Bug 1254003: Don't call tabs.create callback before the tab is ready. r=gabor
☠☠ backed out by 0225961ad7ad ☠ ☠
authorKris Maglione <maglione.k@gmail.com>
Sun, 03 Apr 2016 19:31:47 -0700
changeset 332396 a57938cd29d2ff6c3d6bc6efa853f01a800bb326
parent 332395 215083cf8511e3350400199c35b76b2268fd720a
child 332397 5655a04459af6ab8a11a030beb36496198fb00bc
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgabor
bugs1254003
milestone48.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
@@ -448,64 +448,84 @@ 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}`});
+            }
+          }
+
+          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 == tab.linkedBrowser.currentURI.spec) {
+            // 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 new Promise(resolve => {
+            let webProgress = tab.linkedBrowser.webProgress;
+            let listener = {
+              QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener, Ci.nsISupportsWeakReference]),
+
+              onLocationChange() {
+                webProgress.removeProgressListener(listener);
+                resolve(tab);
+              },
+            };
+            webProgress.addProgressListener(listener, Ci.nsIWebProgress.NOTIFY_LOCATION);
+          });
+        }).then(tab => {
+          return TabManager.convert(extension, tab);
         });
       },
 
       remove: function(tabs) {
         if (!Array.isArray(tabs)) {
           tabs = [tabs];
         }
 
@@ -525,17 +545,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})