Bug 1462121 - Improve reliability of contentWindow/active around inner window changes draft
authorRob Wu <rob@robwu.nl>
Thu, 17 May 2018 16:47:57 +0200
changeset 796416 c88caa1a5b72ea7331e16efd52f392191f5bdf4a
parent 796415 7ca348cacee283c7c001fb2927b3ff4501fcf02b
push id110248
push userbmo:rob@robwu.nl
push dateThu, 17 May 2018 17:12:40 +0000
bugs1462121
milestone62.0a1
Bug 1462121 - Improve reliability of contentWindow/active around inner window changes Previously, the context.contentWindow and context.active were unreliable around navigations from/to the bfcache, as they were not set during the pageshow event, and neither after pagehide (including the unload event). This commit fixes the issue by using getters. Note that there is no memory leak because these properties are explicitly nulled in the context.unload method. MozReview-Commit-ID: JqUR636FIaz
toolkit/components/extensions/ExtensionCommon.jsm
toolkit/components/extensions/test/xpcshell/test_ext_contentscript_context.js
toolkit/components/extensions/test/xpcshell/test_ext_contentscript_context_isolation.js
--- a/toolkit/components/extensions/ExtensionCommon.jsm
+++ b/toolkit/components/extensions/ExtensionCommon.jsm
@@ -152,31 +152,50 @@ class BaseContext {
 
     MessageChannel.setupMessageManagers([this.messageManager]);
 
     let isValidInnerWindow = () => {
       return !Cu.isDeadWrapper(contentWindow) &&
         getInnerWindowID(contentWindow) === this.innerWindowID;
     };
 
+    function defineWritableGetter(object, prop, getter) {
+      Object.defineProperty(object, prop, {
+        configurable: true,
+        enumerable: true,
+        get: getter,
+        set(value) {
+          Object.defineProperty(this, prop, {
+            configurable: true,
+            enumerable: true,
+            writable: true,
+            value,
+          });
+        },
+      });
+    }
+
     // Use an event target object (instead of a function) so that the closure
     // can be released upon closing this context, even if the document is in the
     // bfcache.
     let handler = {
       handleEvent: (event) => {
         if (event.target !== document) {
           return;
         }
         if (event.type === "pageshow") {
           this.contentWindow = contentWindow;
           this.active = true;
         }
         if (event.type === "pagehide") {
-          this.contentWindow = null;
-          this.active = false;
+          // Define getters instead of properties, so that the "contentWindow"
+          // and "active" properties are accurate during the "pageshow" event
+          // and during "unload".
+          defineWritableGetter(this, "contentWindow", () => isValidInnerWindow() ? contentWindow : null);
+          defineWritableGetter(this, "active", isValidInnerWindow);
         }
       },
     };
 
     this.contentWindow = contentWindow;
     this.active = true;
     contentWindow.addEventListener("pagehide", handler, {mozSystemGroup: true}, false);
     contentWindow.addEventListener("pageshow", handler, {mozSystemGroup: true}, false);
--- a/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_context.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_context.js
@@ -124,8 +124,86 @@ add_task(async function test_contentscri
 
     await this.pageshownPromise;
 
     Assert.equal(this.context.contentWindow, null, "Context's contentWindow property is null after restore from bfcache");
   });
 
   await contentPage.close();
 });
+
+add_task(async function test_contentscript_context_valid_during_execution() {
+  // This test does the following:
+  // - Load page
+  // - Load extension; inject content script.
+  // - Navigate page; pagehide triggered.
+  // - Navigate back; pageshow triggered.
+  // - Close page; pagehide, unload triggered.
+  // At each of these last four events, the validity of the context is checked.
+
+  function contentScript() {
+    browser.test.sendMessage("content-script-ready");
+    window.wrappedJSObject.checkContextIsValid("Context is valid on execution");
+
+    window.addEventListener("pagehide", () => {
+      window.wrappedJSObject.checkContextIsValid("Context is valid on pagehide");
+      browser.test.sendMessage("content-script-hide");
+    }, true);
+    window.addEventListener("pageshow", () => {
+      window.wrappedJSObject.checkContextIsValid("Context is valid on pageshow");
+
+      // This unload listener is registered after pageshow, to ensure that the
+      // page can be stored in the bfcache at the previous pagehide.
+      window.addEventListener("unload", () => {
+        window.wrappedJSObject.checkContextIsValid("Context is valid on unload");
+        browser.test.sendMessage("content-script-unload");
+      });
+
+      browser.test.sendMessage("content-script-show");
+    });
+  }
+
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      content_scripts: [{
+        "matches": ["http://example.com/dummy"],
+        "js": ["content_script.js"],
+      }],
+    },
+
+    files: {
+      "content_script.js": contentScript,
+    },
+  });
+
+  let contentPage = await ExtensionTestUtils.loadContentPage("http://example.com/dummy");
+  await contentPage.spawn(extension.id, async extensionId => {
+    let context;
+    let checkContextIsValid = (description) => {
+      if (!context) {
+        let {DocumentManager} = ChromeUtils.import("resource://gre/modules/ExtensionContent.jsm", {});
+        context = DocumentManager.getContext(extensionId, this.content);
+      }
+      Assert.equal(context.contentWindow, this.content, `${description}: contentWindow`);
+      Assert.equal(context.active, true, `${description}: contentWindow`);
+    };
+    Cu.exportFunction(checkContextIsValid, this.content, {defineAs: "checkContextIsValid"});
+  });
+  await extension.startup();
+  await extension.awaitMessage("content-script-ready");
+
+  await contentPage.spawn(extension.id, async extensionId => {
+    // Navigate so that the content page is hidden in the bfcache.
+    this.content.location = "http://example.org/dummy";
+  });
+
+  await extension.awaitMessage("content-script-hide");
+  await contentPage.spawn(null, async () => {
+    // Navigate back so the content page is resurrected from the bfcache.
+    this.content.history.back();
+  });
+
+  await extension.awaitMessage("content-script-show");
+  await contentPage.close();
+  await extension.awaitMessage("content-script-hide");
+  await extension.awaitMessage("content-script-unload");
+  await extension.unload();
+});
--- a/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_context_isolation.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_context_isolation.js
@@ -79,33 +79,39 @@ add_task(async function test_contentscri
 
     this.content.location = "http://example.org/dummy";
   });
 
   await extension.awaitMessage("content-script-hide");
 
   await contentPage.spawn(null, async () => {
     Assert.equal(this.context.contentWindow, null, "Context's contentWindow property is null");
+    Assert.ok(this.context.sandbox, "Context's sandbox exists");
 
     // Navigate back so the content page is resurrected from the bfcache.
     this.content.history.back();
   });
 
   await extension.awaitMessage("content-script-show");
 
   await contentPage.spawn(null, async () => {
     Assert.equal(this.context.contentWindow, this.content, "Context's contentWindow property is correct");
+    Assert.ok(this.context.sandbox, "Context's sandbox exists before unload");
 
     // Now add an "unload" event listener, which should prevent a page from entering the bfcache.
     await new Promise((resolve) => {
       this.content.addEventListener("unload", () => {
-        Assert.equal(this.context.contentWindow, null, "Context's contentWindow property is null");
+        Assert.equal(this.context.contentWindow, this.content, "Context's contentWindow property is null");
         resolve();
       });
       this.content.location = "http://example.org/dummy";
     });
   });
 
   await extension.awaitMessage("content-script-unload");
 
+  await contentPage.spawn(null, async () => {
+    Assert.equal(this.context.sandbox, null, "Context's sandbox has been destroyed after unload");
+  });
+
   await contentPage.close();
   await extension.unload();
 });