Bug 1254003: Don't call tabs.create callback before the tab is ready. r=gabor
authorKris Maglione <maglione.k@gmail.com>
Sun, 03 Apr 2016 19:31:47 -0700
changeset 339404 f42a04d822baa22a4741c4fff1aab2054857e398
parent 339403 0ac19ca034d9097ded455362e46b56017f3a64a4
child 339405 87207eee5e4785e29d405e7f3586d7bde08b91ce
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgabor
bugs1254003
milestone49.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1254003: Don't call tabs.create callback before the tab is ready. r=gabor MozReview-Commit-ID: 3qK6GpTmIFQ
browser/components/extensions/ext-tabs.js
browser/components/extensions/test/browser/browser_ext_pageAction_context.js
browser/components/extensions/test/browser/browser_ext_tabs_create.js
browser/components/extensions/test/browser/browser_ext_tabs_create_invalid_url.js
browser/components/extensions/test/browser/browser_ext_tabs_detectLanguage.js
browser/components/extensions/test/browser/browser_ext_tabs_executeScript.js
browser/components/extensions/test/browser/browser_ext_tabs_update_url.js
--- a/browser/components/extensions/ext-tabs.js
+++ b/browser/components/extensions/ext-tabs.js
@@ -225,16 +225,46 @@ let tabListener = {
   },
 
   emitRemoved(tab, isWindowClosing) {
     let windowId = WindowManager.getId(tab.ownerDocument.defaultView);
     let tabId = TabManager.getId(tab);
 
     this.emit("tab-removed", {tab, tabId, windowId, isWindowClosing});
   },
+
+  tabReadyInitialized: false,
+  tabReadyPromises: new WeakMap(),
+
+  initTabReady() {
+    if (!this.tabReadyInitialized) {
+      AllWindowEvents.addListener("progress", this);
+
+      this.tabReadyInitialized = true;
+    }
+  },
+
+  onLocationChange(browser, webProgress, request, locationURI, flags) {
+    if (webProgress.isTopLevel) {
+      let gBrowser = browser.ownerDocument.defaultView.gBrowser;
+      let tab = gBrowser.getTabForBrowser(browser);
+
+      let deferred = this.tabReadyPromises.get(tab);
+      if (deferred) {
+        deferred.resolve(tab);
+        this.tabReadyPromises.delete(tab);
+      }
+    }
+  },
+
+  awaitTabReady(tab) {
+    return new Promise((resolve, reject) => {
+      this.tabReadyPromises.set(tab, {resolve, reject});
+    });
+  },
 };
 
 extensions.registerSchemaAPI("tabs", null, (extension, context) => {
   let self = {
     tabs: {
       onActivated: new WindowEventManager(context, "tabs.onActivated", "TabSelect", (fire, event) => {
         let tab = event.originalTarget;
         let tabId = TabManager.getId(tab);
@@ -448,64 +478,74 @@ extensions.registerSchemaAPI("tabs", nul
           AllWindowEvents.removeListener("TabAttrModified", listener);
           AllWindowEvents.removeListener("TabPinned", listener);
           AllWindowEvents.removeListener("TabUnpinned", listener);
         };
       }).api(),
 
       create: function(createProperties) {
         return new Promise((resolve, reject) => {
-          function createInWindow(window) {
-            let url;
-
-            if (createProperties.url !== null) {
-              url = context.uri.resolve(createProperties.url);
-
-              if (!context.checkLoadURL(url, {dontReportErrors: true})) {
-                reject({message: `URL not allowed: ${url}`});
-                return;
-              }
-            }
-
-            let tab = window.gBrowser.addTab(url || window.BROWSER_NEW_TAB_URL);
-
-            let active = true;
-            if (createProperties.active !== null) {
-              active = createProperties.active;
-            }
-            if (active) {
-              window.gBrowser.selectedTab = tab;
-            }
-
-            if (createProperties.index !== null) {
-              window.gBrowser.moveTabTo(tab, createProperties.index);
-            }
-
-            if (createProperties.pinned) {
-              window.gBrowser.pinTab(tab);
-            }
-
-            resolve(TabManager.convert(extension, tab));
-          }
-
           let window = createProperties.windowId !== null ?
             WindowManager.getWindow(createProperties.windowId, context) :
             WindowManager.topWindow;
           if (!window.gBrowser) {
             let obs = (finishedWindow, topic, data) => {
               if (finishedWindow != window) {
                 return;
               }
               Services.obs.removeObserver(obs, "browser-delayed-startup-finished");
-              createInWindow(window);
+              resolve(window);
             };
             Services.obs.addObserver(obs, "browser-delayed-startup-finished", false);
           } else {
-            createInWindow(window);
+            resolve(window);
+          }
+        }).then(window => {
+          let url;
+
+          if (createProperties.url !== null) {
+            url = context.uri.resolve(createProperties.url);
+
+            if (!context.checkLoadURL(url, {dontReportErrors: true})) {
+              return Promise.reject({message: `Illegal URL: ${url}`});
+            }
+          }
+
+          tabListener.initTabReady();
+          let tab = window.gBrowser.addTab(url || window.BROWSER_NEW_TAB_URL);
+
+          let active = true;
+          if (createProperties.active !== null) {
+            active = createProperties.active;
+          }
+          if (active) {
+            window.gBrowser.selectedTab = tab;
           }
+
+          if (createProperties.index !== null) {
+            window.gBrowser.moveTabTo(tab, createProperties.index);
+          }
+
+          if (createProperties.pinned) {
+            window.gBrowser.pinTab(tab);
+          }
+
+          if (!createProperties.url || createProperties.url.startsWith("about:")) {
+            // We can't wait for a location change event for about:newtab,
+            // since it may be pre-rendered, in which case its initial
+            // location change event has already fired.
+            return tab;
+          }
+
+          // Wait for the first location change event, so that operations
+          // like `executeScript` are dispatched to the inner window that
+          // contains the URL we're attempting to load.
+          return tabListener.awaitTabReady(tab);
+        }).then(tab => {
+          return TabManager.convert(extension, tab);
         });
       },
 
       remove: function(tabs) {
         if (!Array.isArray(tabs)) {
           tabs = [tabs];
         }
 
@@ -525,17 +565,17 @@ extensions.registerSchemaAPI("tabs", nul
         }
 
         let tabbrowser = tab.ownerDocument.defaultView.gBrowser;
 
         if (updateProperties.url !== null) {
           let url = context.uri.resolve(updateProperties.url);
 
           if (!context.checkLoadURL(url, {dontReportErrors: true})) {
-            return Promise.reject({message: `URL not allowed: ${url}`});
+            return Promise.reject({message: `Illegal URL: ${url}`});
           }
 
           tab.linkedBrowser.loadURI(url);
         }
 
         if (updateProperties.active !== null) {
           if (updateProperties.active) {
             tabbrowser.selectedTab = tab;
--- a/browser/components/extensions/test/browser/browser_ext_pageAction_context.js
+++ b/browser/components/extensions/test/browser/browser_ext_pageAction_context.js
@@ -200,18 +200,16 @@ add_task(function* testTabSwitchContext(
           browser.tabs.onUpdated.addListener(function listener(tabId, changed) {
             if (tabId == details.id && changed.url == details.url) {
               browser.tabs.onUpdated.removeListener(listener);
               resolve();
             }
           });
         });
       };
-      let tabLoadPromise;
-
       return [
         expect => {
           browser.test.log("Initial state. No icon visible.");
           expect(null);
         },
         expect => {
           browser.test.log("Show the icon on the first tab, expect default properties.");
           browser.pageAction.show(tabs[0]);
@@ -220,26 +218,23 @@ add_task(function* testTabSwitchContext(
         expect => {
           browser.test.log("Change the icon. Expect default properties excluding the icon.");
           browser.pageAction.setIcon({tabId: tabs[0], path: "1.png"});
           expect(details[1]);
         },
         expect => {
           browser.test.log("Create a new tab. No icon visible.");
           browser.tabs.create({active: true, url: "about:blank?0"}, tab => {
-            tabLoadPromise = promiseTabLoad({url: "about:blank?0", id: tab.id});
             tabs.push(tab.id);
             expect(null);
           });
         },
         expect => {
           browser.test.log("Await tab load. No icon visible.");
-          tabLoadPromise.then(() => {
-            expect(null);
-          });
+          expect(null);
         },
         expect => {
           browser.test.log("Change properties. Expect new properties.");
           let tabId = tabs[1];
           browser.pageAction.show(tabId);
           browser.pageAction.setIcon({tabId, path: "2.png"});
           browser.pageAction.setPopup({tabId, popup: "2.html"});
           browser.pageAction.setTitle({tabId, title: "Title 2"});
--- a/browser/components/extensions/test/browser/browser_ext_tabs_create.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_create.js
@@ -96,53 +96,54 @@ add_task(function* () {
               return;
             }
 
             let test = tests.shift();
             let expected = Object.assign({}, DEFAULTS, test.result);
 
             browser.test.log(`Testing tabs.create(${JSON.stringify(test.create)}), expecting ${JSON.stringify(test.result)}`);
 
-            let tabId;
             let updatedPromise = new Promise(resolve => {
               let onUpdated = (changedTabId, changed) => {
-                if (changedTabId === tabId && changed.url) {
+                if (changed.url) {
                   browser.tabs.onUpdated.removeListener(onUpdated);
-                  resolve(changed.url);
+                  resolve({tabId: changedTabId, url: changed.url});
                 }
               };
               browser.tabs.onUpdated.addListener(onUpdated);
             });
 
             let createdPromise = new Promise(resolve => {
               let onCreated = tab => {
                 browser.test.assertTrue("id" in tab, `Expected tabs.onCreated callback to receive tab object`);
                 resolve();
               };
               browser.tabs.onCreated.addListener(onCreated);
             });
 
+            let tabId;
             Promise.all([
               browser.tabs.create(test.create),
               createdPromise,
             ]).then(([tab]) => {
               tabId = tab.id;
 
               for (let key of Object.keys(expected)) {
                 if (key === "url") {
                   // FIXME: This doesn't get updated until later in the load cycle.
                   continue;
                 }
 
                 browser.test.assertEq(expected[key], tab[key], `Expected value for tab.${key}`);
               }
 
               return updatedPromise;
-            }).then(url => {
-              browser.test.assertEq(expected.url, url, `Expected value for tab.url`);
+            }).then(updated => {
+              browser.test.assertEq(tabId, updated.tabId, `Expected value for tab.id`);
+              browser.test.assertEq(expected.url, updated.url, `Expected value for tab.url`);
 
               return browser.tabs.remove(tabId);
             }).then(() => {
               return browser.tabs.update(activeTab, {active: true});
             }).then(() => {
               nextTest();
             });
           }
--- a/browser/components/extensions/test/browser/browser_ext_tabs_create_invalid_url.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_create_invalid_url.js
@@ -8,17 +8,17 @@ function* testTabsCreateInvalidURL(tabsC
       "permissions": ["tabs"],
     },
 
     background: function() {
       browser.test.sendMessage("ready");
       browser.test.onMessage.addListener((msg, tabsCreateURL) => {
         browser.tabs.create({url: tabsCreateURL}, (tab) => {
           browser.test.assertEq(undefined, tab, "on error tab should be undefined");
-          browser.test.assertTrue(/URL not allowed/.test(browser.runtime.lastError.message),
+          browser.test.assertTrue(/Illegal URL/.test(browser.runtime.lastError.message),
                                   "runtime.lastError should report the expected error message");
 
           // Remove the opened tab is any.
           if (tab) {
             browser.tabs.remove(tab.id);
           }
           browser.test.sendMessage("done");
         });
--- a/browser/components/extensions/test/browser/browser_ext_tabs_detectLanguage.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_detectLanguage.js
@@ -7,30 +7,17 @@ add_task(function* testDetectLanguage() 
     manifest: {
       "permissions": ["tabs"],
     },
 
     background() {
       const BASE_PATH = "browser/browser/components/extensions/test/browser";
 
       function loadTab(url) {
-        let tabId;
-        let awaitUpdated = new Promise(resolve => {
-          browser.tabs.onUpdated.addListener(function onUpdated(changedTabId, changed, tab) {
-            if (changedTabId === tabId && changed.url) {
-              browser.tabs.onUpdated.removeListener(onUpdated);
-              resolve(tab);
-            }
-          });
-        });
-
-        return browser.tabs.create({url}).then(tab => {
-          tabId = tab.id;
-          return awaitUpdated;
-        });
+        return browser.tabs.create({url});
       }
 
       loadTab(`http://example.co.jp/${BASE_PATH}/file_language_ja.html`).then(tab => {
         return browser.tabs.detectLanguage(tab.id).then(lang => {
           browser.test.assertEq("ja", lang, "Japanese document should be detected as Japanese");
           return browser.tabs.remove(tab.id);
         });
       }).then(() => {
--- a/browser/components/extensions/test/browser/browser_ext_tabs_executeScript.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_executeScript.js
@@ -113,16 +113,24 @@ add_task(function* testExecuteScript() {
 
         browser.tabs.executeScript({
           code: "location.href;",
           frameId: frames[1].frameId,
         }).then(result => {
           browser.test.assertEq("http://mochi.test:8888/", result, "Result for frameId[1] is correct");
         }),
 
+        browser.tabs.create({url: "http://example.com/"}).then(tab => {
+          return browser.tabs.executeScript(tab.id, {code: "location.href"}).then(result => {
+            browser.test.assertEq("http://example.com/", result, "Script executed correctly in new tab");
+
+            return browser.tabs.remove(tab.id);
+          });
+        }),
+
         new Promise(resolve => {
           browser.runtime.onMessage.addListener(message => {
             browser.test.assertEq("script ran", message, "Expected runtime message");
             resolve();
           });
         }),
       ]);
     }).then(() => {
@@ -130,17 +138,17 @@ add_task(function* testExecuteScript() {
     }).catch(e => {
       browser.test.fail(`Error: ${e} :: ${e.stack}`);
       browser.test.notifyFail("executeScript");
     });
   }
 
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
-      "permissions": ["http://mochi.test/", "webNavigation"],
+      "permissions": ["http://mochi.test/", "http://example.com/", "webNavigation"],
     },
 
     background,
 
     files: {
       "script.js": function() {
         browser.runtime.sendMessage("script ran");
       },
--- a/browser/components/extensions/test/browser/browser_ext_tabs_update_url.js
+++ b/browser/components/extensions/test/browser/browser_ext_tabs_update_url.js
@@ -32,17 +32,17 @@ function* testTabsUpdateURL(existentTabU
             browser.test.assertTrue(tab, "on success the tab should be defined");
           }
         };
 
         let onTabsUpdateError = (error) => {
           if (!isErrorExpected) {
             browser.test.fails(`tabs.update with URL ${tabsUpdateURL} should not be rejected`);
           } else {
-            browser.test.assertTrue(/^URL not allowed/.test(error.message),
+            browser.test.assertTrue(/^Illegal URL/.test(error.message),
                                     "tabs.update should be rejected with the expected error message");
           }
         };
 
         let onTabsUpdateDone = () => browser.test.sendMessage("done");
 
         browser.tabs.query({lastFocusedWindow: true}, (tabs) => {
           browser.tabs.update(tabs[1].id, {url: tabsUpdateURL})