Bug 1250784: Part 2 - [webext] Fix some issues that cause noisy tests. r=Mossop draft
authorKris Maglione <maglione.k@gmail.com>
Wed, 09 Mar 2016 17:26:27 -0800
changeset 338834 16fa88807d9048657d968ccb99e5a6b3a7f45f76
parent 338833 f9f9c3b81b4579181b55780fb9f84d81dd6436e3
child 338835 daa4356fc0b0a97a08833753381c4a1cf128166e
push id12585
push usermaglione.k@gmail.com
push dateThu, 10 Mar 2016 01:42:05 +0000
reviewersMossop
bugs1250784
milestone48.0a1
Bug 1250784: Part 2 - [webext] Fix some issues that cause noisy tests. r=Mossop MozReview-Commit-ID: 93NMinJpWRo
browser/base/content/browser.js
browser/components/extensions/ext-bookmarks.js
browser/components/extensions/test/browser/head.js
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/ExtensionUtils.jsm
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -4634,28 +4634,37 @@ var CombinedStopReload = {
     if (this._timer) {
       clearTimeout(this._timer);
       this._timer = 0;
     }
   }
 };
 
 var TabsProgressListener = {
+  // Keep track of which browsers we've started load timers for, since
+  // we won't see STATE_START events for pre-rendered tabs.
+  _startedLoadTimer: new WeakSet(),
+
   onStateChange: function (aBrowser, aWebProgress, aRequest, aStateFlags, aStatus) {
     // Collect telemetry data about tab load times.
     if (aWebProgress.isTopLevel && (!aRequest.originalURI || aRequest.originalURI.spec.scheme != "about")) {
       if (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW) {
         if (aStateFlags & Ci.nsIWebProgressListener.STATE_START) {
+          this._startedLoadTimer.add(aBrowser);
           TelemetryStopwatch.start("FX_PAGE_LOAD_MS", aBrowser);
           Services.telemetry.getHistogramById("FX_TOTAL_TOP_VISITS").add(true);
-        } else if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
+        } else if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
+                   this._startedLoadTimer.has(aBrowser)) {
+          this._startedLoadTimer.delete(aBrowser);
           TelemetryStopwatch.finish("FX_PAGE_LOAD_MS", aBrowser);
         }
       } else if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
-                 aStatus == Cr.NS_BINDING_ABORTED) {
+                 aStatus == Cr.NS_BINDING_ABORTED &&
+                 this._startedLoadTimer.has(aBrowser)) {
+        this._startedLoadTimer.delete(aBrowser);
         TelemetryStopwatch.cancel("FX_PAGE_LOAD_MS", aBrowser);
       }
     }
 
     // Attach a listener to watch for "click" events bubbling up from error
     // pages and other similar pages (like about:newtab). This lets us fix bugs
     // like 401575 which require error page UI to do privileged things, without
     // letting error pages have any privilege themselves.
--- a/browser/components/extensions/ext-bookmarks.js
+++ b/browser/components/extensions/ext-bookmarks.js
@@ -90,17 +90,17 @@ extensions.registerSchemaAPI("bookmarks"
           for (let id of list) {
             let bookmark = yield Bookmarks.fetch({guid: id});
             if (!bookmark) {
               throw new Error("Bookmark not found");
             }
             bookmarks.push(convert(bookmark));
           }
           return bookmarks;
-        });
+        }).catch(error => Promise.reject({message: error.message}));
       },
 
       getChildren: function(id) {
         // TODO: We should optimize this.
         return getTree(id, true);
       },
 
       getTree: function() {
@@ -138,17 +138,18 @@ extensions.registerSchemaAPI("bookmarks"
 
         if (bookmark.parentId !== null) {
           info.parentGuid = bookmark.parentId;
         } else {
           info.parentGuid = Bookmarks.unfiledGuid;
         }
 
         try {
-          return Bookmarks.insert(info).then(convert);
+          return Bookmarks.insert(info).then(convert)
+                          .catch(error => Promise.reject({message: error.message}));
         } catch (e) {
           return Promise.reject({message: `Invalid bookmark: ${JSON.stringify(info)}`});
         }
       },
 
       move: function(id, destination) {
         let info = {
           guid: id,
@@ -157,17 +158,18 @@ extensions.registerSchemaAPI("bookmarks"
         if (destination.parentId !== null) {
           info.parentGuid = destination.parentId;
         }
         if (destination.index !== null) {
           info.index = destination.index;
         }
 
         try {
-          return Bookmarks.update(info).then(convert);
+          return Bookmarks.update(info).then(convert)
+                          .catch(error => Promise.reject({message: error.message}));
         } catch (e) {
           return Promise.reject({message: `Invalid bookmark: ${JSON.stringify(info)}`});
         }
       },
 
       update: function(id, changes) {
         let info = {
           guid: id,
@@ -176,42 +178,45 @@ extensions.registerSchemaAPI("bookmarks"
         if (changes.title !== null) {
           info.title = changes.title;
         }
         if (changes.url !== null) {
           info.url = changes.url;
         }
 
         try {
-          return Bookmarks.update(info).then(convert);
+          return Bookmarks.update(info).then(convert)
+                          .catch(error => Promise.reject({message: error.message}));
         } catch (e) {
           return Promise.reject({message: `Invalid bookmark: ${JSON.stringify(info)}`});
         }
       },
 
       remove: function(id) {
         let info = {
           guid: id,
         };
 
         // The API doesn't give you the old bookmark at the moment
         try {
-          return Bookmarks.remove(info, {preventRemovalOfNonEmptyFolders: true}).then(result => {});
+          return Bookmarks.remove(info, {preventRemovalOfNonEmptyFolders: true}).then(result => {})
+                          .catch(error => Promise.reject({message: error.message}));
         } catch (e) {
           return Promise.reject({message: `Invalid bookmark: ${JSON.stringify(info)}`});
         }
       },
 
       removeTree: function(id) {
         let info = {
           guid: id,
         };
 
         try {
-          return Bookmarks.remove(info).then(result => {});
+          return Bookmarks.remove(info).then(result => {})
+                          .catch(error => Promise.reject({message: error.message}));
         } catch (e) {
           return Promise.reject({message: `Invalid bookmark: ${JSON.stringify(info)}`});
         }
       },
     },
   };
 });
 
--- a/browser/components/extensions/test/browser/head.js
+++ b/browser/components/extensions/test/browser/head.js
@@ -126,14 +126,16 @@ function clickPageAction(extension, win 
 
   EventUtils.synthesizeMouseAtCenter(elem, {}, win);
   return new Promise(SimpleTest.executeSoon);
 }
 
 function closePageAction(extension, win = window) {
   let node = getPageActionPopup(extension, win);
   if (node) {
-    node.hidePopup();
+    return promisePopupShown(node).then(() => {
+      node.hidePopup();
+    });
   }
 
   return Promise.resolve();
 }
 
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -392,22 +392,17 @@ GlobalManager = {
             if (callback === null) {
               callback = defaultCallback;
             }
 
             let promise;
             try {
               promise = findPath(path)[name](...args);
             } catch (e) {
-              if (e instanceof context.cloneScope.Error) {
-                promise = Promise.reject(e);
-              } else {
-                Cu.reportError(e);
-                promise = Promise.reject({message: "An unexpected error occurred"});
-              }
+              promise = Promise.reject(e);
             }
 
             return context.wrapPromise(promise || Promise.resolve(), callback);
           },
 
           shouldInject(path, name) {
             return findPath(path) != null;
           },
--- a/toolkit/components/extensions/ExtensionUtils.jsm
+++ b/toolkit/components/extensions/ExtensionUtils.jsm
@@ -197,35 +197,52 @@ class BaseContext {
   }
 
   set lastError(val) {
     this.checkedLastError = false;
     this._lastError = val;
   }
 
   /**
+   * Normalizes the given error object for use by the target scope. If
+   * the target is an error object which belongs to that scope, it is
+   * returned as-is. If it is an ordinary object with a `message`
+   * property, it is converted into an error belonging to the target
+   * scope. If it is an Error object which does *not* belong to the
+   * clone scope, it is reported, and converted to an unexpected
+   * exception error.
+   */
+  normalizeError(error) {
+    if (error instanceof this.cloneScope.Error) {
+      return error;
+    }
+    if (!instanceOf(error, "Object")) {
+      Cu.reportError(error);
+      error = {message: "An unexpected error occurred"};
+    }
+    return new this.cloneScope.Error(error.message);
+  }
+
+  /**
    * Sets the value of `.lastError` to `error`, calls the given
    * callback, and reports an error if the value has not been checked
    * when the callback returns.
    *
    * @param {object} error An object with a `message` property. May
    *     optionally be an `Error` object belonging to the target scope.
    * @param {function} callback The callback to call.
    * @returns {*} The return value of callback.
    */
   withLastError(error, callback) {
-    if (!(error instanceof this.cloneScope.Error)) {
-      error = new this.cloneScope.Error(error.message);
-    }
-    this.lastError = error;
+    this.lastError = this.normalizeError(error);
     try {
       return callback();
     } finally {
       if (!this.checkedLastError) {
-        Cu.reportError(`Unchecked lastError value: ${error}`);
+        Cu.reportError(`Unchecked lastError value: ${this.lastError}`);
       }
       this.lastError = null;
     }
   }
 
   /**
    * Wraps the given promise so it can be safely returned to extension
    * code in this context.
@@ -273,20 +290,17 @@ class BaseContext {
             runSafeSyncWithoutClone(callback);
           });
         });
     } else {
       return new this.cloneScope.Promise((resolve, reject) => {
         promise.then(
           value => { runSafe(resolve, value); },
           value => {
-            if (!(value instanceof this.cloneScope.Error)) {
-              value = new this.cloneScope.Error(value.message);
-            }
-            runSafeSyncWithoutClone(reject, value);
+            runSafeSyncWithoutClone(reject, this.normalizeError(value));
           });
       });
     }
   }
 
   unload() {
     MessageChannel.abortResponses({
       extensionId: this.extension.id,