Bug 1386445 - Early about:newtab pages are missing message APIs because RemotePages registers too late. r?Mossop draft
authorEd Lee <edilee@mozilla.com>
Tue, 01 Aug 2017 12:30:33 -0700
changeset 621360 e7ac3b3f663f6d760854b37b7b8ba11590ba6e80
parent 620862 32083f24a1bb2c33050b4c972783f066432194eb
child 640984 3da5330b95d6a57dd8efacc759ab54312175b3dd
push id72346
push userbmo:edilee@mozilla.com
push dateFri, 04 Aug 2017 18:19:06 +0000
reviewersMossop
bugs1386445
milestone57.0a1
Bug 1386445 - Early about:newtab pages are missing message APIs because RemotePages registers too late. r?Mossop This moves AboutNewTab.init from nsBrowserGlue.js handling of "browser-delayed-startup-finished" into aboutNewTabService.js so that when the service is loaded once from the main thread probably by browser.js towards the beginning of _delayedStartup just before potentially calling gBrowser.loadTabs, the service triggers the attaching of RemotePages(about:newtab) before any about:newtab pages load. Additionally even when RemotePages starts early enough, Activity Stream might not borrow the RemotePages instance early enough to catch the RemotePage:Load message, so to simulate that, RemotePages now remembers when a port has been loaded for consumers to check. Adds tests to confirm the expected properties on the port and value of loaded at the various RemotePage:* messages. MozReview-Commit-ID: IXJLvFCgbEH
browser/base/content/browser.js
browser/components/newtab/aboutNewTabService.js
browser/components/nsBrowserGlue.js
browser/extensions/activity-stream/lib/ActivityStreamMessageChannel.jsm
toolkit/modules/RemotePageManager.jsm
toolkit/modules/tests/browser/browser_RemotePageManager.js
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -1450,16 +1450,21 @@ var gBrowserInit = {
     gBrowser.addEventListener("InsecureLoginFormsStateChange", function() {
       gIdentityHandler.refreshForInsecureLoginForms();
     });
 
     gBrowser.addEventListener("PermissionStateChange", function() {
       gIdentityHandler.refreshIdentityBlock();
     });
 
+    // Get the service so that it initializes and registers listeners for new
+    // tab pages in order to be ready for any early-loading about:newtab pages,
+    // e.g., start/home page, command line / startup uris to load, sessionstore
+    gAboutNewTabService.QueryInterface(Ci.nsISupports);
+
     let uriToLoad = this._getUriToLoad();
     if (uriToLoad && uriToLoad != "about:blank") {
       if (uriToLoad instanceof Ci.nsIArray) {
         let count = uriToLoad.length;
         let specs = [];
         for (let i = 0; i < count; i++) {
           let urisstring = uriToLoad.queryElementAt(i, Ci.nsISupportsString);
           specs.push(urisstring.data);
--- a/browser/components/newtab/aboutNewTabService.js
+++ b/browser/components/newtab/aboutNewTabService.js
@@ -5,31 +5,37 @@
 */
 
 "use strict";
 
 const {utils: Cu, interfaces: Ci} = Components;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
-XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
-                                  "resource://gre/modules/Preferences.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "AboutNewTab",
+                                  "resource:///modules/AboutNewTab.jsm");
 
 const LOCAL_NEWTAB_URL = "chrome://browser/content/newtab/newTab.xhtml";
 
 const ACTIVITY_STREAM_URL = "resource://activity-stream/data/content/activity-stream.html";
 
 const ABOUT_URL = "about:newtab";
 
+const IS_MAIN_PROCESS = Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_DEFAULT;
+
 // Pref that tells if activity stream is enabled
 const PREF_ACTIVITY_STREAM_ENABLED = "browser.newtabpage.activity-stream.enabled";
 
 function AboutNewTabService() {
-  Preferences.observe(PREF_ACTIVITY_STREAM_ENABLED, this._handleToggleEvent.bind(this));
-  this.toggleActivityStream(Services.prefs.getBoolPref(PREF_ACTIVITY_STREAM_ENABLED));
+  Services.obs.addObserver(this, "quit-application-granted");
+  Services.prefs.addObserver(PREF_ACTIVITY_STREAM_ENABLED, this);
+  this.toggleActivityStream();
+  if (IS_MAIN_PROCESS) {
+    AboutNewTab.init();
+  }
 }
 
 /*
  * A service that allows for the overriding, at runtime, of the newtab page's url.
  * Additionally, the service manages pref state between a activity stream, or the regular
  * about:newtab page.
  *
  * There is tight coupling with browser/about/AboutRedirector.cpp.
@@ -63,24 +69,38 @@ function AboutNewTabService() {
  */
 AboutNewTabService.prototype = {
 
   _newTabURL: ABOUT_URL,
   _activityStreamEnabled: false,
   _overridden: false,
 
   classID: Components.ID("{dfcd2adc-7867-4d3a-ba70-17501f208142}"),
-  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAboutNewTabService]),
+  QueryInterface: XPCOMUtils.generateQI([
+    Ci.nsIAboutNewTabService,
+    Ci.nsIObserver
+  ]),
   _xpcom_categories: [{
     service: true
   }],
 
-  _handleToggleEvent(stateEnabled) {
-    if (this.toggleActivityStream(stateEnabled)) {
-      Services.obs.notifyObservers(null, "newtab-url-changed", ABOUT_URL);
+  observe(subject, topic, data) {
+    switch (topic) {
+      case "nsPref:changed":
+        if (this.toggleActivityStream()) {
+          Services.obs.notifyObservers(null, "newtab-url-changed", ABOUT_URL);
+        }
+        break;
+      case "quit-application-granted":
+        Services.obs.removeObserver(this, "quit-application-granted");
+        Services.prefs.removeObserver(PREF_ACTIVITY_STREAM_ENABLED, this);
+        if (IS_MAIN_PROCESS) {
+          AboutNewTab.uninit();
+        }
+        break;
     }
   },
 
   /**
    * React to changes to the activity stream pref.
    *
    * If browser.newtabpage.activity-stream.enabled is true, this will change the default URL to the
    * activity stream page URL. If browser.newtabpage.activity-stream.enabled is false, the default URL
@@ -88,17 +108,18 @@ AboutNewTabService.prototype = {
    *
    * This will only act if there is a change of state and if not overridden.
    *
    * @returns {Boolean} Returns if there has been a state change
    *
    * @param {Boolean}   stateEnabled    activity stream enabled state to set to
    * @param {Boolean}   forceState      force state change
    */
-  toggleActivityStream(stateEnabled, forceState = false) {
+  toggleActivityStream(stateEnabled = Services.prefs.getBoolPref(PREF_ACTIVITY_STREAM_ENABLED),
+                       forceState = false) {
 
     if (!forceState && (this.overridden || stateEnabled === this.activityStreamEnabled)) {
       // exit there is no change of state
       return false;
     }
     if (stateEnabled) {
       this._activityStreamEnabled = true;
     } else {
@@ -153,14 +174,14 @@ AboutNewTabService.prototype = {
 
   get activityStreamURL() {
     return ACTIVITY_STREAM_URL;
   },
 
   resetNewTabURL() {
     this._overridden = false;
     this._newTabURL = ABOUT_URL;
-    this.toggleActivityStream(Services.prefs.getBoolPref(PREF_ACTIVITY_STREAM_ENABLED), true);
+    this.toggleActivityStream(undefined, true);
     Services.obs.notifyObservers(null, "newtab-url-changed", this._newTabURL);
   }
 };
 
 this.NSGetFactory = XPCOMUtils.generateNSGetFactory([AboutNewTabService]);
--- a/browser/components/nsBrowserGlue.js
+++ b/browser/components/nsBrowserGlue.js
@@ -20,17 +20,17 @@ XPCOMUtils.defineLazyGetter(this, "Weave
 XPCOMUtils.defineLazyModuleGetter(this, "ContextualIdentityService",
                                   "resource://gre/modules/ContextualIdentityService.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "SafeBrowsing",
                                   "resource://gre/modules/SafeBrowsing.jsm");
 
 // lazy module getters
 
-/* global AboutHome:false, AboutNewTab:false, AddonManager:false, AppMenuNotifications:false,
+/* global AboutHome:false, AddonManager:false, AppMenuNotifications:false,
           AsyncPrefs: false, AsyncShutdown:false, AutoCompletePopup:false, BookmarkHTMLUtils:false,
           BookmarkJSONUtils:false, BrowserUITelemetry:false, BrowserUsageTelemetry:false,
           ContentClick:false, ContentPrefServiceParent:false, ContentSearch:false,
           DateTimePickerHelper:false, DirectoryLinksProvider:false,
           ExtensionsUI:false, Feeds:false,
           FileUtils:false, FormValidationHandler:false, Integration:false,
           LightweightThemeManager:false, LoginHelper:false, LoginManagerParent:false,
           NetUtil:false, NewTabUtils:false, OS:false,
@@ -49,17 +49,16 @@ XPCOMUtils.defineLazyModuleGetter(this, 
  * IF YOU ADD OR REMOVE FROM THIS LIST, PLEASE UPDATE THE LIST ABOVE AS WELL.
  * XXX Bug 1325373 is for making eslint detect these automatically.
  */
 
 let initializedModules = {};
 
 [
   ["AboutHome", "resource:///modules/AboutHome.jsm", "init"],
-  ["AboutNewTab", "resource:///modules/AboutNewTab.jsm"],
   ["AddonManager", "resource://gre/modules/AddonManager.jsm"],
   ["AppMenuNotifications", "resource://gre/modules/AppMenuNotifications.jsm"],
   ["AsyncPrefs", "resource://gre/modules/AsyncPrefs.jsm"],
   ["AsyncShutdown", "resource://gre/modules/AsyncShutdown.jsm"],
   ["AutoCompletePopup", "resource://gre/modules/AutoCompletePopup.jsm"],
   ["BookmarkHTMLUtils", "resource://gre/modules/BookmarkHTMLUtils.jsm"],
   ["BookmarkJSONUtils", "resource://gre/modules/BookmarkJSONUtils.jsm"],
   ["BrowserUITelemetry", "resource:///modules/BrowserUITelemetry.jsm"],
@@ -964,17 +963,16 @@ BrowserGlue.prototype = {
       WeaveService.init();
     }
 
     PageThumbs.init();
 
     DirectoryLinksProvider.init();
     NewTabUtils.init();
     NewTabUtils.links.addProvider(DirectoryLinksProvider);
-    AboutNewTab.init();
 
     PageActions.init();
 
     this._firstWindowTelemetry(aWindow);
     this._firstWindowLoaded();
 
     this._mediaTelemetryIdleObserver = {
       browserGlue: this,
@@ -1025,17 +1023,16 @@ BrowserGlue.prototype = {
       if (mod.uninit) {
         mod.uninit();
       }
     }
 
     BrowserUsageTelemetry.uninit();
 
     PageThumbs.uninit();
-    AboutNewTab.uninit();
     NewTabUtils.uninit();
     AutoCompletePopup.uninit();
     DateTimePickerHelper.uninit();
   },
 
   _initServiceDiscovery() {
     if (!Services.prefs.getBoolPref("browser.casting.enabled")) {
       return;
--- a/browser/extensions/activity-stream/lib/ActivityStreamMessageChannel.jsm
+++ b/browser/extensions/activity-stream/lib/ActivityStreamMessageChannel.jsm
@@ -128,16 +128,23 @@ this.ActivityStreamMessageChannel = clas
   createChannel() {
     //  Receive AboutNewTab's Remote Pages instance, if it exists, on override
     const channel = this.pageURL === ABOUT_NEW_TAB_URL && AboutNewTab.override(true);
     this.channel = channel || new RemotePages(this.pageURL);
     this.channel.addMessageListener("RemotePage:Init", this.onNewTabInit);
     this.channel.addMessageListener("RemotePage:Load", this.onNewTabLoad);
     this.channel.addMessageListener("RemotePage:Unload", this.onNewTabUnload);
     this.channel.addMessageListener(this.incomingMessageName, this.onMessage);
+
+    // Some pages might have already loaded, so we won't get the usual message
+    for (const {loaded, portID} of this.channel.messagePorts) {
+      if (loaded) {
+        this.onNewTabLoad({target: {portID}});
+      }
+    }
   }
 
   /**
    * destroyChannel - Destroys the RemotePages channel
    */
   destroyChannel() {
     this.channel.removeMessageListener("RemotePage:Init", this.onNewTabInit);
     this.channel.removeMessageListener("RemotePage:Load", this.onNewTabLoad);
--- a/toolkit/modules/RemotePageManager.jsm
+++ b/toolkit/modules/RemotePageManager.jsm
@@ -88,39 +88,49 @@ RemotePages.prototype = {
     this.listener = null;
     this.destroyed = true;
   },
 
   // Called when a page matching the url has loaded in a frame.
   portCreated(port) {
     this.messagePorts.add(port);
 
+    port.loaded = false;
+    port.addMessageListener("RemotePage:Load", this.portMessageReceived);
     port.addMessageListener("RemotePage:Unload", this.portMessageReceived);
 
     for (let name of this.listener.keys()) {
       this.registerPortListener(port, name);
     }
 
     this.listener.callListeners({ target: port, name: "RemotePage:Init" });
   },
 
   // A message has been received from one of the pages
   portMessageReceived(message) {
-    if (message.name == "RemotePage:Unload")
-      this.removeMessagePort(message.target);
+    switch (message.name) {
+      case "RemotePage:Load":
+        message.target.loaded = true;
+        break;
+      case "RemotePage:Unload":
+        message.target.loaded = false;
+        this.removeMessagePort(message.target);
+        break;
+    }
 
     this.listener.callListeners(message);
   },
 
   // A page has closed
   removeMessagePort(port) {
     for (let name of this.listener.keys()) {
       port.removeMessageListener(name, this.portMessageReceived);
     }
 
+    port.removeMessageListener("RemotePage:Load", this.portMessageReceived);
     port.removeMessageListener("RemotePage:Unload", this.portMessageReceived);
     this.messagePorts.delete(port);
   },
 
   registerPortListener(port, name) {
     port.addMessageListener(name, this.portMessageReceived);
   },
 
@@ -172,23 +182,25 @@ function publicMessagePort(port) {
                     "sendAsyncMessage", "destroy"];
 
   let clean = {};
   for (let property of properties) {
     clean[property] = port[property].bind(port);
   }
 
   Object.defineProperty(clean, "portID", {
+    enumerable: true,
     get() {
       return port.portID;
     }
   });
 
   if (port instanceof ChromeMessagePort) {
     Object.defineProperty(clean, "browser", {
+      enumerable: true,
       get() {
         return port.browser;
       }
     });
   }
 
   return clean;
 }
--- a/toolkit/modules/tests/browser/browser_RemotePageManager.js
+++ b/toolkit/modules/tests/browser/browser_RemotePageManager.js
@@ -300,16 +300,69 @@ add_task(async function remote_pages_bas
   try {
     pages.sendAsyncMessage("Foo");
     ok(false, "Should have seen exception");
   } catch (e) {
     ok(true, "Should have seen exception");
   }
 });
 
+// Test that properties exist on the target port provided to listeners
+add_task(async function check_port_properties() {
+  let pages = new RemotePages(TEST_URL);
+
+  const expectedProperties = [
+    "addMessageListener",
+    "browser",
+    "destroy",
+    "loaded",
+    "portID",
+    "removeMessageListener",
+    "sendAsyncMessage"
+  ];
+  function checkProperties(port, description) {
+    const expected = [];
+    const unexpected = [];
+    for (const key in port) {
+      (expectedProperties.includes(key) ? expected : unexpected).push(key);
+    }
+    is(`${expected.sort()}`, expectedProperties, `${description} has expected keys`);
+    is(`${unexpected.sort()}`, "", `${description} should not have unexpected keys`);
+  }
+
+  function portFrom(message, extraFn = () => {}) {
+    return new Promise(resolve => {
+      function onMessage({target}) {
+        pages.removeMessageListener(message, onMessage);
+        resolve(target);
+      }
+      pages.addMessageListener(message, onMessage);
+      extraFn();
+    });
+  }
+
+  let portFromInit = await portFrom("RemotePage:Init", () =>
+    (gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser, TEST_URL)));
+  checkProperties(portFromInit, "inited port");
+  is(portFromInit.loaded, false, "inited port has not been loaded yet");
+
+  let portFromLoad = await portFrom("RemotePage:Load");
+  is(portFromLoad, portFromInit, "got the same port from init and load");
+  checkProperties(portFromLoad, "loaded port");
+  is(portFromInit.loaded, true, "loaded port is now loaded");
+
+  let portFromUnload = await portFrom("RemotePage:Unload", () =>
+    BrowserTestUtils.removeTab(gBrowser.selectedTab));
+  is(portFromUnload, portFromInit, "got the same port from init and unload");
+  checkProperties(portFromUnload, "unloaded port");
+  is(portFromInit.loaded, false, "unloaded port is now not loaded");
+
+  pages.destroy();
+});
+
 // Test sending messages to all remote pages works
 add_task(async function remote_pages_multiple() {
   let pages = new RemotePages(TEST_URL);
   let port1 = await waitForPage(pages);
   let port2 = await waitForPage(pages);
 
   let pongPorts = [];
   await new Promise((resolve) => {
@@ -384,10 +437,12 @@ add_task(async function get_ports_for_br
   let pages = new RemotePages(TEST_URL);
   let port = await waitForPage(pages);
   // waitForPage creates a new tab and selects it by default, so
   // the selected tab should be the one hosting this port.
   let browser = gBrowser.selectedBrowser;
   let foundPorts = pages.portsForBrowser(browser);
   is(foundPorts.length, 1, "There should only be one port for this simple page");
   is(foundPorts[0], port, "Should find the port");
+
+  pages.destroy();
   gBrowser.removeCurrentTab();
 });