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

--HG--
extra : rebase_source : 2b53c4e58f4cb8cbd4ea10741f3f609693989010

browser/base/content/browser.js

@@ -1452,6 +1452,11 @@ var gBrowserInit = {
       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) {

browser/components/newtab/aboutNewTabService.js

@@ -10,8 +10,8 @@ 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";
 
@@ -19,12 +19,18 @@ const ACTIVITY_STREAM_URL = "resource://activity-stream/data/content/activity-st
 
 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();
+  }
 }
 
 /*
@@ -68,14 +74,28 @@ AboutNewTabService.prototype = {
   _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;
     }
   },
 
@@ -93,7 +113,8 @@ AboutNewTabService.prototype = {
    * @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
@@ -158,7 +179,7 @@ AboutNewTabService.prototype = {
   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);
   }
 };

browser/components/nsBrowserGlue.js

@@ -25,7 +25,7 @@ XPCOMUtils.defineLazyModuleGetter(this, "SafeBrowsing",
 
 // 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,
@@ -54,7 +54,6 @@ 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"],
@@ -969,7 +968,6 @@ BrowserGlue.prototype = {
     DirectoryLinksProvider.init();
     NewTabUtils.init();
     NewTabUtils.links.addProvider(DirectoryLinksProvider);
-    AboutNewTab.init();
 
     PageActions.init();
 
@@ -1030,7 +1028,6 @@ BrowserGlue.prototype = {
     BrowserUsageTelemetry.uninit();
 
     PageThumbs.uninit();
-    AboutNewTab.uninit();
     NewTabUtils.uninit();
     AutoCompletePopup.uninit();
     DateTimePickerHelper.uninit();

browser/extensions/activity-stream/lib/ActivityStreamMessageChannel.jsm

@@ -133,6 +133,13 @@ this.ActivityStreamMessageChannel = class ActivityStreamMessageChannel {
     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}});
+      }
+    }
   }
 
   /**

toolkit/modules/RemotePageManager.jsm

@@ -93,6 +93,8 @@ RemotePages.prototype = {
   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()) {
@@ -104,8 +106,15 @@ RemotePages.prototype = {
 
   // 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);
   },
@@ -116,6 +125,7 @@ RemotePages.prototype = {
       port.removeMessageListener(name, this.portMessageReceived);
     }
 
+    port.removeMessageListener("RemotePage:Load", this.portMessageReceived);
     port.removeMessageListener("RemotePage:Unload", this.portMessageReceived);
     this.messagePorts.delete(port);
   },
@@ -177,6 +187,7 @@ function publicMessagePort(port) {
   }
 
   Object.defineProperty(clean, "portID", {
+    enumerable: true,
     get() {
       return port.portID;
     }
@@ -184,6 +195,7 @@ function publicMessagePort(port) {
 
   if (port instanceof ChromeMessagePort) {
     Object.defineProperty(clean, "browser", {
+      enumerable: true,
       get() {
         return port.browser;
       }

toolkit/modules/tests/browser/browser_RemotePageManager.js

@@ -305,6 +305,59 @@ add_task(async function remote_pages_basic() {
   }
 });
 
+// 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);
@@ -389,5 +442,7 @@ add_task(async function get_ports_for_browser() {
   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();
 });