Bug 1225215: [webext] Raise the expected errors when given invalid tab IDs. r?aswan draft
authorKris Maglione <maglione.k@gmail.com>
Fri, 26 Feb 2016 17:56:30 -0800
changeset 375405 6548055ec739e2aeff656d0078a92801834426b0
parent 375401 0ac19ca034d9097ded455362e46b56017f3a64a4
child 522864 1d28276cfefffc88fe5fcf75cacb229999db4483
push id20269
push usermaglione.k@gmail.com
push dateSun, 05 Jun 2016 06:52:36 +0000
reviewersaswan
bugs1225215
milestone49.0a1
Bug 1225215: [webext] Raise the expected errors when given invalid tab IDs. r?aswan MozReview-Commit-ID: E5G0GmVhzLh
browser/components/extensions/ext-browserAction.js
browser/components/extensions/ext-pageAction.js
browser/components/extensions/ext-tabs.js
browser/components/extensions/ext-utils.js
browser/components/extensions/ext-windows.js
browser/components/extensions/test/browser/browser_ext_browserAction_context.js
browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js
toolkit/components/extensions/ext-webNavigation.js
--- a/browser/components/extensions/ext-browserAction.js
+++ b/browser/components/extensions/ext-browserAction.js
@@ -236,82 +236,90 @@ extensions.registerSchemaAPI("browserAct
         };
         BrowserAction.for(extension).on("click", listener);
         return () => {
           BrowserAction.for(extension).off("click", listener);
         };
       }).api(),
 
       enable: function(tabId) {
-        let tab = tabId !== null ? TabManager.getTab(tabId) : null;
+        let tab = tabId !== null ? TabManager.getTab(tabId, context) : null;
         BrowserAction.for(extension).setProperty(tab, "enabled", true);
       },
 
       disable: function(tabId) {
-        let tab = tabId !== null ? TabManager.getTab(tabId) : null;
+        let tab = tabId !== null ? TabManager.getTab(tabId, context) : null;
         BrowserAction.for(extension).setProperty(tab, "enabled", false);
       },
 
       setTitle: function(details) {
-        let tab = details.tabId !== null ? TabManager.getTab(details.tabId) : null;
+        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;
 
         let title = details.title;
         // Clear the tab-specific title when given a null string.
         if (tab && title == "") {
           title = null;
         }
         BrowserAction.for(extension).setProperty(tab, "title", title);
       },
 
       getTitle: function(details) {
-        let tab = details.tabId !== null ? TabManager.getTab(details.tabId) : null;
+        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;
+
         let title = BrowserAction.for(extension).getProperty(tab, "title");
         return Promise.resolve(title);
       },
 
       setIcon: function(details) {
-        let tab = details.tabId !== null ? TabManager.getTab(details.tabId) : null;
+        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;
+
         let icon = IconDetails.normalize(details, extension, context);
         BrowserAction.for(extension).setProperty(tab, "icon", icon);
         return Promise.resolve();
       },
 
       setBadgeText: function(details) {
-        let tab = details.tabId !== null ? TabManager.getTab(details.tabId) : null;
+        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;
+
         BrowserAction.for(extension).setProperty(tab, "badgeText", details.text);
       },
 
       getBadgeText: function(details) {
-        let tab = details.tabId !== null ? TabManager.getTab(details.tabId) : null;
+        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;
+
         let text = BrowserAction.for(extension).getProperty(tab, "badgeText");
         return Promise.resolve(text);
       },
 
       setPopup: function(details) {
-        let tab = details.tabId !== null ? TabManager.getTab(details.tabId) : null;
+        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;
+
         // Note: Chrome resolves arguments to setIcon relative to the calling
         // context, but resolves arguments to setPopup relative to the extension
         // root.
         // For internal consistency, we currently resolve both relative to the
         // calling context.
         let url = details.popup && context.uri.resolve(details.popup);
         BrowserAction.for(extension).setProperty(tab, "popup", url);
       },
 
       getPopup: function(details) {
-        let tab = details.tabId !== null ? TabManager.getTab(details.tabId) : null;
+        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;
+
         let popup = BrowserAction.for(extension).getProperty(tab, "popup");
         return Promise.resolve(popup);
       },
 
       setBadgeBackgroundColor: function(details) {
-        let tab = details.tabId !== null ? TabManager.getTab(details.tabId) : null;
+        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;
+
         BrowserAction.for(extension).setProperty(tab, "badgeBackgroundColor", details.color);
       },
 
       getBadgeBackgroundColor: function(details, callback) {
-        let tab = details.tabId !== null ? TabManager.getTab(details.tabId) : null;
+        let tab = details.tabId !== null ? TabManager.getTab(details.tabId, context) : null;
+
         let color = BrowserAction.for(extension).getProperty(tab, "badgeBackgroundColor");
         return Promise.resolve(color);
       },
     },
   };
 });
--- a/browser/components/extensions/ext-pageAction.js
+++ b/browser/components/extensions/ext-pageAction.js
@@ -214,56 +214,60 @@ extensions.registerSchemaAPI("pageAction
 
         pageAction.on("click", listener);
         return () => {
           pageAction.off("click", listener);
         };
       }).api(),
 
       show(tabId) {
-        let tab = TabManager.getTab(tabId);
+        let tab = TabManager.getTab(tabId, context);
         PageAction.for(extension).setProperty(tab, "show", true);
       },
 
       hide(tabId) {
-        let tab = TabManager.getTab(tabId);
+        let tab = TabManager.getTab(tabId, context);
         PageAction.for(extension).setProperty(tab, "show", false);
       },
 
       setTitle(details) {
-        let tab = TabManager.getTab(details.tabId);
+        let tab = TabManager.getTab(details.tabId, context);
 
         // Clear the tab-specific title when given a null string.
         PageAction.for(extension).setProperty(tab, "title", details.title || null);
       },
 
       getTitle(details) {
-        let tab = TabManager.getTab(details.tabId);
+        let tab = TabManager.getTab(details.tabId, context);
+
         let title = PageAction.for(extension).getProperty(tab, "title");
         return Promise.resolve(title);
       },
 
       setIcon(details) {
-        let tab = TabManager.getTab(details.tabId);
+        let tab = TabManager.getTab(details.tabId, context);
+
         let icon = IconDetails.normalize(details, extension, context);
         PageAction.for(extension).setProperty(tab, "icon", icon);
         return Promise.resolve();
       },
 
       setPopup(details) {
-        let tab = TabManager.getTab(details.tabId);
+        let tab = TabManager.getTab(details.tabId, context);
+
         // Note: Chrome resolves arguments to setIcon relative to the calling
         // context, but resolves arguments to setPopup relative to the extension
         // root.
         // For internal consistency, we currently resolve both relative to the
         // calling context.
         let url = details.popup && context.uri.resolve(details.popup);
         PageAction.for(extension).setProperty(tab, "popup", url);
       },
 
       getPopup(details) {
-        let tab = TabManager.getTab(details.tabId);
+        let tab = TabManager.getTab(details.tabId, context);
+
         let popup = PageAction.for(extension).getProperty(tab, "popup");
         return Promise.resolve(popup);
       },
     },
   };
 });
--- a/browser/components/extensions/ext-tabs.js
+++ b/browser/components/extensions/ext-tabs.js
@@ -505,29 +505,25 @@ extensions.registerSchemaAPI("tabs", nul
       },
 
       remove: function(tabs) {
         if (!Array.isArray(tabs)) {
           tabs = [tabs];
         }
 
         for (let tabId of tabs) {
-          let tab = TabManager.getTab(tabId);
+          let tab = TabManager.getTab(tabId, context);
           tab.ownerDocument.defaultView.gBrowser.removeTab(tab);
         }
 
         return Promise.resolve();
       },
 
       update: function(tabId, updateProperties) {
-        let tab = tabId !== null ? TabManager.getTab(tabId) : TabManager.activeTab;
-
-        if (!tab) {
-          return Promise.reject({message: `No tab found with tabId: ${tabId}`});
-        }
+        let tab = tabId !== null ? TabManager.getTab(tabId, context) : TabManager.activeTab;
 
         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}`});
@@ -556,28 +552,30 @@ extensions.registerSchemaAPI("tabs", nul
           }
         }
         // FIXME: highlighted/selected, openerTabId
 
         return Promise.resolve(TabManager.convert(extension, tab));
       },
 
       reload: function(tabId, reloadProperties) {
-        let tab = tabId !== null ? TabManager.getTab(tabId) : TabManager.activeTab;
+        let tab = tabId !== null ? TabManager.getTab(tabId, context) : TabManager.activeTab;
+
         let flags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
         if (reloadProperties && reloadProperties.bypassCache) {
           flags |= Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;
         }
         tab.linkedBrowser.reloadWithFlags(flags);
 
         return Promise.resolve();
       },
 
       get: function(tabId) {
-        let tab = TabManager.getTab(tabId);
+        let tab = TabManager.getTab(tabId, context);
+
         return Promise.resolve(TabManager.convert(extension, tab));
       },
 
       getCurrent() {
         let tab;
         if (context.tabId) {
           tab = TabManager.convert(extension, TabManager.getTab(context.tabId));
         }
@@ -704,17 +702,17 @@ extensions.registerSchemaAPI("tabs", nul
         let recipient = {innerWindowID: browser.innerWindowID};
 
         return context.sendMessage(browser.messageManager, "Extension:DetectLanguage",
                                    {}, {recipient});
       },
 
       // Used to executeScript, insertCSS and removeCSS.
       _execute: function(tabId, details, kind, method) {
-        let tab = tabId !== null ? TabManager.getTab(tabId) : TabManager.activeTab;
+        let tab = tabId !== null ? TabManager.getTab(tabId, context) : TabManager.activeTab;
         let mm = tab.linkedBrowser.messageManager;
 
         let options = {
           js: [],
           css: [],
           remove_css: method == "removeCSS",
         };
 
@@ -775,32 +773,32 @@ extensions.registerSchemaAPI("tabs", nul
         return self.tabs._execute(tabId, details, "css", "insertCSS");
       },
 
       removeCSS: function(tabId, details) {
         return self.tabs._execute(tabId, details, "css", "removeCSS");
       },
 
       connect: function(tabId, connectInfo) {
-        let tab = TabManager.getTab(tabId);
+        let tab = TabManager.getTab(tabId, context);
         let mm = tab.linkedBrowser.messageManager;
 
         let name = "";
         if (connectInfo && connectInfo.name !== null) {
           name = connectInfo.name;
         }
         let recipient = {extensionId: extension.id};
         if (connectInfo && connectInfo.frameId !== null) {
           recipient.frameId = connectInfo.frameId;
         }
         return context.messenger.connect(mm, name, recipient);
       },
 
       sendMessage: function(tabId, message, options, responseCallback) {
-        let tab = TabManager.getTab(tabId);
+        let tab = TabManager.getTab(tabId, context, null);
         if (!tab) {
           // ignore sendMessage to non existent tab id
           return;
         }
         let mm = tab.linkedBrowser.messageManager;
 
         let recipient = {extensionId: extension.id};
         if (options && options.frameId !== null) {
@@ -829,23 +827,18 @@ extensions.registerSchemaAPI("tabs", nul
           Indexes are maintained on a per window basis so that a call to
             move([tabA, tabB], {index: 0})
               -> tabA to 0, tabB to 1 if tabA and tabB are in the same window
             move([tabA, tabB], {index: 0})
               -> tabA to 0, tabB to 0 if tabA and tabB are in different windows
         */
         let indexMap = new Map();
 
-        for (let tabId of tabIds) {
-          let tab = TabManager.getTab(tabId);
-          // Ignore invalid tab ids.
-          if (!tab) {
-            continue;
-          }
-
+        let tabs = tabIds.map(tabId => TabManager.getTab(tabId, context));
+        for (let tab of tabs) {
           // If the window is not specified, use the window from the tab.
           let window = destinationWindow || tab.ownerDocument.defaultView;
           let gBrowser = window.gBrowser;
 
           let insertionPoint = indexMap.get(window) || index;
           // If the index is -1 it should go to the end of the tabs.
           if (insertionPoint == -1) {
             insertionPoint = gBrowser.tabs.length;
@@ -873,20 +866,17 @@ extensions.registerSchemaAPI("tabs", nul
           }
           tabsMoved.push(tab);
         }
 
         return Promise.resolve(tabsMoved.map(tab => TabManager.convert(extension, tab)));
       },
 
       duplicate: function(tabId) {
-        let tab = TabManager.getTab(tabId);
-        if (!tab) {
-          return Promise.reject({message: `Invalid tab ID: ${tabId}`});
-        }
+        let tab = TabManager.getTab(tabId, context);
 
         let gBrowser = tab.ownerDocument.defaultView.gBrowser;
         let newTab = gBrowser.duplicateTab(tab);
 
         return new Promise(resolve => {
           // We need to use SSTabRestoring because any attributes set before
           // are ignored. SSTabRestored is too late and results in a jump in
           // the UI. See http://bit.ly/session-store-api for more information.
--- a/browser/components/extensions/ext-utils.js
+++ b/browser/components/extensions/ext-utils.js
@@ -587,29 +587,37 @@ global.TabManager = {
       let tab = gBrowser.getTabForBrowser(browser);
       if (tab) {
         return this.getId(tab);
       }
     }
     return -1;
   },
 
-  getTab(tabId) {
+  /**
+   * Returns the XUL <tab> element associated with the given tab ID. If no tab
+   * with the given ID exists, and no default value is provided, an error is
+   * raised, belonging to the scope of the given context.
+   */
+  getTab(tabId, context, default_ = undefined) {
     // FIXME: Speed this up without leaking memory somehow.
     for (let window of WindowListManager.browserWindows()) {
       if (!window.gBrowser) {
         continue;
       }
       for (let tab of window.gBrowser.tabs) {
         if (this.getId(tab) == tabId) {
           return tab;
         }
       }
     }
-    return null;
+    if (default_ !== undefined) {
+      return default_;
+    }
+    throw new context.cloneScope.Error(`Invalid tab ID: ${tabId}`);
   },
 
   get activeTab() {
     let window = WindowManager.topWindow;
     if (window && window.gBrowser) {
       return window.gBrowser.selectedTab;
     }
     return null;
--- a/browser/components/extensions/ext-windows.js
+++ b/browser/components/extensions/ext-windows.js
@@ -88,20 +88,17 @@ extensions.registerSchemaAPI("windows", 
 
         let args = Cc["@mozilla.org/supports-array;1"].createInstance(Ci.nsISupportsArray);
 
         if (createData.tabId !== null) {
           if (createData.url !== null) {
             return Promise.reject({message: "`tabId` may not be used in conjunction with `url`"});
           }
 
-          let tab = TabManager.getTab(createData.tabId);
-          if (tab == null) {
-            return Promise.reject({message: `Invalid tab ID: ${createData.tabId}`});
-          }
+          let tab = TabManager.getTab(createData.tabId, context);
 
           // Private browsing tabs can only be moved to private browsing
           // windows.
           let incognito = PrivateBrowsingUtils.isBrowserPrivate(tab.linkedBrowser);
           if (createData.incognito !== null && createData.incognito != incognito) {
             return Promise.reject({message: "`incognito` property must match the incognito state of tab"});
           }
           createData.incognito = incognito;
--- a/browser/components/extensions/test/browser/browser_ext_browserAction_context.js
+++ b/browser/components/extensions/test/browser/browser_ext_browserAction_context.js
@@ -39,16 +39,45 @@ function* runTests(options) {
 
     let expectDefaults = expecting => {
       return checkDetails(expecting);
     };
 
     let tabs = [];
     let tests = getTests(tabs, expectDefaults);
 
+    {
+      let tabId = 0xdeadbeef;
+      let calls = [
+        () => browser.browserAction.enable(tabId),
+        () => browser.browserAction.disable(tabId),
+        () => browser.browserAction.setTitle({tabId, title: "foo"}),
+        () => browser.browserAction.setIcon({tabId, path: "foo.png"}),
+        () => browser.browserAction.setPopup({tabId, popup: "foo.html"}),
+        () => browser.browserAction.setBadgeText({tabId, text: "foo"}),
+        () => browser.browserAction.setBadgeBackgroundColor({tabId, color: [0xff, 0, 0, 0xff]}),
+      ];
+
+      for (let call of calls) {
+        let checkError = e => {
+          browser.test.assertTrue(e.message.includes(`Invalid tab ID: ${tabId}`),
+                                  `Expected invalid tab ID error, got ${e}`);
+        }
+        try {
+          call().then(() => {
+            browser.test.fail(`Expected call to fail: ${call}`);
+          }, e => {
+            checkError(e);
+          });
+        } catch (e) {
+          checkError(e);
+        }
+      }
+    }
+
     // Runs the next test in the `tests` array, checks the results,
     // and passes control back to the outer test scope.
     function nextTest() {
       let test = tests.shift();
 
       test(expecting => {
         // Check that the API returns the expected values, and then
         // run the next test.
--- a/browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js
+++ b/browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js
@@ -6,25 +6,25 @@ add_task(function* testWebNavigationGetN
   let extension = ExtensionTestUtils.loadExtension({
     background: "(" + function() {
       let results = [
         // There is no "tabId = 0" because the id assigned by TabManager (defined in ext-utils.js)
         // starts from 1.
         browser.webNavigation.getAllFrames({tabId: 0}).then(() => {
           browser.test.fail("getAllFrames Promise should be rejected on error");
         }, (error) => {
-          browser.test.assertEq("No tab found with tabId: 0", error.message,
+          browser.test.assertEq("Invalid tab ID: 0", error.message,
                                 "getAllFrames rejected Promise should pass the expected error");
         }),
         // There is no "tabId = 0" because the id assigned by TabManager (defined in ext-utils.js)
         // starts from 1, processId is currently marked as optional and it is ignored.
         browser.webNavigation.getFrame({tabId: 0, frameId: 15, processId: 20}).then(() => {
           browser.test.fail("getFrame Promise should be rejected on error");
         }, (error) => {
-          browser.test.assertEq("No tab found with tabId: 0", error.message,
+          browser.test.assertEq("Invalid tab ID: 0", error.message,
                                 "getFrame rejected Promise should pass the expected error");
         }),
       ];
 
       Promise.all(results).then(() => {
         browser.test.sendMessage("getNonExistentTab.done");
       });
     } + ")();",
--- a/toolkit/components/extensions/ext-webNavigation.js
+++ b/toolkit/components/extensions/ext-webNavigation.js
@@ -157,29 +157,29 @@ extensions.registerSchemaAPI("webNavigat
       onCommitted: new WebNavigationEventManager(context, "onCommitted").api(),
       onDOMContentLoaded: new WebNavigationEventManager(context, "onDOMContentLoaded").api(),
       onCompleted: new WebNavigationEventManager(context, "onCompleted").api(),
       onErrorOccurred: new WebNavigationEventManager(context, "onErrorOccurred").api(),
       onReferenceFragmentUpdated: new WebNavigationEventManager(context, "onReferenceFragmentUpdated").api(),
       onHistoryStateUpdated: new WebNavigationEventManager(context, "onHistoryStateUpdated").api(),
       onCreatedNavigationTarget: ignoreEvent(context, "webNavigation.onCreatedNavigationTarget"),
       getAllFrames(details) {
-        let tab = TabManager.getTab(details.tabId);
+        let tab = TabManager.getTab(details.tabId, context);
         if (!tab) {
           return Promise.reject({message: `No tab found with tabId: ${details.tabId}`});
         }
 
         let {innerWindowID, messageManager} = tab.linkedBrowser;
         let recipient = {innerWindowID};
 
         return context.sendMessage(messageManager, "WebNavigation:GetAllFrames", {}, {recipient})
                       .then((results) => results.map(convertGetFrameResult.bind(null, details.tabId)));
       },
       getFrame(details) {
-        let tab = TabManager.getTab(details.tabId);
+        let tab = TabManager.getTab(details.tabId, context);
         if (!tab) {
           return Promise.reject({message: `No tab found with tabId: ${details.tabId}`});
         }
 
         let recipient = {
           innerWindowID: tab.linkedBrowser.innerWindowID,
         };