Bug 1369705 - avoid starting the search service or calling the search-one-offs XBL constructor before first paint, r=adw.
authorFlorian Quèze <florian@queze.net>
Fri, 09 Jun 2017 15:11:03 +0200
changeset 363186 56ac88ccb3c6b15d0f551451897134b7d66e1fa8
parent 363185 944c5cc643a642b399ae6dc7ec29ba2a0e42b562
child 363187 d1bdd265b00d8bebb2e73fdf75da45ad300088ff
push id91264
push userflorian@queze.net
push dateFri, 09 Jun 2017 13:12:39 +0000
treeherdermozilla-inbound@c76e85ca482f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1369705
milestone55.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 1369705 - avoid starting the search service or calling the search-one-offs XBL constructor before first paint, r=adw.
browser/base/content/test/performance/browser_startup.js
browser/base/content/urlbarBindings.xml
browser/components/search/content/search.xml
browser/components/search/test/browser_426329.js
browser/components/tests/startupRecorder.js
--- a/browser/base/content/test/performance/browser_startup.js
+++ b/browser/base/content/test/performance/browser_startup.js
@@ -42,32 +42,32 @@ const startupPhases = {
       "resource://gre/modules/RemotePageManager.jsm", // bug 1369466
       "resource://gre/modules/Promise.jsm" // bug 1368456
     ])
   }},
 
   // For the following phases of startup we have only a black list for now
 
   // We are at this phase after creating the first browser window (ie. after final-ui-startup).
-  "before opening first browser window": {blacklist: {
+  "before opening first browser window": {},
+
+  // We reach this phase right after showing the first browser window.
+  // This means that anything already loaded at this point has been loaded
+  // before first paint and delayed it.
+  "before first paint": {blacklist: {
     components: new Set([
       "nsSearchService.js",
     ])
   }},
 
-  // We reach this phase right after showing the first browser window.
-  // This means that anything already loaded at this point has been loaded
-  // before first paint and delayed it.
-  "before first paint": {},
-
   // We are at this phase once we are ready to handle user events.
   // Anything loaded at this phase or before gets in the way of the user
   // interacting with the first browser window.
   "before handling user events": {},
-}
+};
 
 function test() {
   if (!AppConstants.NIGHTLY_BUILD && !AppConstants.DEBUG) {
     ok(!("@mozilla.org/test/startuprecorder;1" in Cc),
        "the startup recorder component shouldn't exist in this non-nightly non-debug build.");
     return;
   }
 
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -116,17 +116,19 @@ file, You can obtain one at http://mozil
                                    GetStringFromName("pasteAndGo.label");
           pasteAndGo.setAttribute("label", label);
           pasteAndGo.setAttribute("anonid", "paste-and-go");
           pasteAndGo.setAttribute("oncommand",
               "gURLBar.select(); goDoCommand('cmd_paste'); gURLBar.handleCommand();");
           cxmenu.insertBefore(pasteAndGo, insertLocation.nextSibling);
         }
 
-        this._enableOrDisableOneOffSearches();
+        this.popup.addEventListener("popupshowing", () => {
+          this._enableOrDisableOneOffSearches();
+        }, {capturing: true, once: true});
 
         // The autocomplete controller uses heuristic on some internal caches
         // to handle cases like backspace, autofill or repeated searches.
         // Ensure to clear those internal caches when switching tabs.
         gBrowser.tabContainer.addEventListener("TabSelect", this);
       ]]></constructor>
 
       <destructor><![CDATA[
--- a/browser/components/search/content/search.xml
+++ b/browser/components/search/content/search.xml
@@ -65,44 +65,55 @@
                      onclick="handleSearchCommand(event);"
                      tooltiptext="&searchEndCap.label;"/>
         </xul:hbox>
       </xul:textbox>
     </content>
 
     <implementation implements="nsIObserver">
       <constructor><![CDATA[
+        this._textbox.placeholder = this._stringBundle.getString("searchPlaceholder");
+
         if (this.parentNode.parentNode.localName == "toolbarpaletteitem")
           return;
 
         Services.obs.addObserver(this, "browser-search-engine-modified");
 
         this._initialized = true;
 
-        Services.search.init(aStatus => {
-          // Bail out if the binding's been destroyed
-          if (!this._initialized)
-            return;
+        (window.delayedStartupPromise || Promise.resolve()).then(() => {
+          window.requestIdleCallback(() => {
+            Services.search.init(aStatus => {
+              // Bail out if the binding's been destroyed
+              if (!this._initialized)
+                return;
 
-          if (Components.isSuccessCode(aStatus)) {
-            // Refresh the display (updating icon, etc)
-            this.updateDisplay();
-            BrowserSearch.updateOpenSearchBadge();
-          } else {
-            Components.utils.reportError("Cannot initialize search service, bailing out: " + aStatus);
-          }
+              if (Components.isSuccessCode(aStatus)) {
+                // Refresh the display (updating icon, etc)
+                this.updateDisplay();
+                BrowserSearch.updateOpenSearchBadge();
+              } else {
+                Components.utils.reportError("Cannot initialize search service, bailing out: " + aStatus);
+              }
+            });
+          });
         });
 
-        // Some accessibility tests create their own <searchbar> that doesn't
-        // use the popup binding below, so null-check oneOffButtons.
-        if (this.textbox.popup.oneOffButtons) {
-          this.textbox.popup.oneOffButtons.telemetryOrigin = "searchbar";
-          this.textbox.popup.oneOffButtons.popup = this.textbox.popup;
-          this.textbox.popup.oneOffButtons.textbox = this.textbox;
-        }
+        // Wait until the popupshowing event to avoid forcing immediate
+        // attachment of the search-one-offs binding.
+        this.textbox.popup.addEventListener("popupshowing", () => {
+          let oneOffButtons = this.textbox.popup.oneOffButtons;
+          // Some accessibility tests create their own <searchbar> that doesn't
+          // use the popup binding below, so null-check oneOffButtons.
+          if (oneOffButtons) {
+            oneOffButtons.telemetryOrigin = "searchbar";
+            oneOffButtons.popup = this.textbox.popup;
+            oneOffButtons.textbox = this.textbox;
+          }
+        }, {capturing: true, once: true});
       ]]></constructor>
 
       <destructor><![CDATA[
         this.destroy();
       ]]></destructor>
 
       <method name="destroy">
         <body><![CDATA[
@@ -281,18 +292,16 @@
 
       <method name="updateDisplay">
         <body><![CDATA[
           var uri = this.currentEngine.iconURI;
           this.setIcon(this, uri ? uri.spec : "");
 
           var name = this.currentEngine.name;
           var text = this._stringBundle.getFormattedString("searchtip", [name]);
-
-          this._textbox.placeholder = this._stringBundle.getString("searchPlaceholder");
           this._textbox.label = text;
           this._textbox.tooltipText = text;
         ]]></body>
       </method>
 
       <method name="updateGoButtonVisibility">
         <body><![CDATA[
           document.getAnonymousElementByAttribute(this, "anonid",
@@ -562,118 +571,125 @@
         }
       ]]></handler>
 
     </handlers>
   </binding>
 
   <binding id="searchbar-textbox"
       extends="chrome://global/content/bindings/autocomplete.xml#autocomplete">
-    <implementation implements="nsIObserver">
+    <implementation>
       <constructor><![CDATA[
-        const kXULNS =
-          "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
-
         if (document.getBindingParent(this).parentNode.parentNode.localName ==
             "toolbarpaletteitem")
           return;
 
-        // Initialize fields
-        this._stringBundle = document.getBindingParent(this)._stringBundle;
-        this._suggestEnabled =
-          Services.prefs.getBoolPref("browser.search.suggest.enabled");
-
         if (Services.prefs.getBoolPref("browser.urlbar.clickSelectsAll"))
           this.setAttribute("clickSelectsAll", true);
 
-        // Add items to context menu and attach controller to handle them
         var textBox = document.getAnonymousElementByAttribute(this,
                                               "anonid", "textbox-input-box");
         var cxmenu = document.getAnonymousElementByAttribute(textBox,
                                           "anonid", "input-box-contextmenu");
-        var pasteAndSearch;
-        cxmenu.addEventListener("popupshowing", function() {
-          BrowserSearch.searchBar._textbox.closePopup();
-          if (!pasteAndSearch)
-            return;
-          var controller = document.commandDispatcher.getControllerForCommand("cmd_paste");
-          var enabled = controller.isCommandEnabled("cmd_paste");
-          if (enabled)
-            pasteAndSearch.removeAttribute("disabled");
-          else
-            pasteAndSearch.setAttribute("disabled", "true");
-        });
-
-        var element, label, akey;
-
-        element = document.createElementNS(kXULNS, "menuseparator");
-        cxmenu.appendChild(element);
+        cxmenu.addEventListener("popupshowing",
+                                () => { this.initContextMenu(cxmenu); },
+                                {capturing: true, once: true});
 
         this.setAttribute("aria-owns", this.popup.id);
-
-        var insertLocation = cxmenu.firstChild;
-        while (insertLocation.nextSibling &&
-               insertLocation.getAttribute("cmd") != "cmd_paste")
-          insertLocation = insertLocation.nextSibling;
-        if (insertLocation) {
-          element = document.createElementNS(kXULNS, "menuitem");
-          label = this._stringBundle.getString("cmd_pasteAndSearch");
-          element.setAttribute("label", label);
-          element.setAttribute("anonid", "paste-and-search");
-          element.setAttribute("oncommand", "BrowserSearch.pasteAndSearch(event)");
-          cxmenu.insertBefore(element, insertLocation.nextSibling);
-          pasteAndSearch = element;
-        }
-
-        element = document.createElementNS(kXULNS, "menuitem");
-        label = this._stringBundle.getString("cmd_clearHistory");
-        akey = this._stringBundle.getString("cmd_clearHistory_accesskey");
-        element.setAttribute("label", label);
-        element.setAttribute("accesskey", akey);
-        element.setAttribute("cmd", "cmd_clearhistory");
-        cxmenu.appendChild(element);
-
-        element = document.createElementNS(kXULNS, "menuitem");
-        label = this._stringBundle.getString("cmd_showSuggestions");
-        akey = this._stringBundle.getString("cmd_showSuggestions_accesskey");
-        element.setAttribute("anonid", "toggle-suggest-item");
-        element.setAttribute("label", label);
-        element.setAttribute("accesskey", akey);
-        element.setAttribute("cmd", "cmd_togglesuggest");
-        element.setAttribute("type", "checkbox");
-        element.setAttribute("checked", this._suggestEnabled);
-        element.setAttribute("autocheck", "false");
-        this._suggestMenuItem = element;
-        cxmenu.appendChild(element);
-
-        this.addEventListener("keypress", aEvent => {
-          if (navigator.platform.startsWith("Mac") && aEvent.keyCode == KeyEvent.VK_F4)
-            this.openSearch()
-        }, true);
-
-        this.controllers.appendController(this.searchbarController);
         document.getBindingParent(this)._textboxInitialized = true;
-
-        // Add observer for suggest preference
-        Services.prefs.addObserver("browser.search.suggest.enabled", this);
       ]]></constructor>
 
       <destructor><![CDATA[
-        Services.prefs.removeObserver("browser.search.suggest.enabled", this);
-
-        // Because XBL and the customize toolbar code interacts poorly,
-        // there may not be anything to remove here
+        // If the context menu has never been opened, there won't be anything
+        // to remove here.
+        // Also, XBL and the customize toolbar code sometimes interact poorly.
         try {
           this.controllers.removeController(this.searchbarController);
         } catch (ex) { }
       ]]></destructor>
 
-      <field name="_stringBundle"/>
-      <field name="_suggestMenuItem"/>
-      <field name="_suggestEnabled"/>
+      // Add items to context menu and attach controller to handle them the
+      // first time the context menu is opened.
+      <method name="initContextMenu">
+        <parameter name="aMenu"/>
+        <body><![CDATA[
+          const kXULNS =
+            "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
+          let stringBundle = document.getBindingParent(this)._stringBundle;
+
+          let pasteAndSearch, suggestMenuItem;
+          let element, label, akey;
+
+          element = document.createElementNS(kXULNS, "menuseparator");
+          aMenu.appendChild(element);
+
+          let insertLocation = aMenu.firstChild;
+          while (insertLocation.nextSibling &&
+                 insertLocation.getAttribute("cmd") != "cmd_paste")
+            insertLocation = insertLocation.nextSibling;
+          if (insertLocation) {
+            element = document.createElementNS(kXULNS, "menuitem");
+            label = stringBundle.getString("cmd_pasteAndSearch");
+            element.setAttribute("label", label);
+            element.setAttribute("anonid", "paste-and-search");
+            element.setAttribute("oncommand", "BrowserSearch.pasteAndSearch(event)");
+            aMenu.insertBefore(element, insertLocation.nextSibling);
+            pasteAndSearch = element;
+          }
+
+          element = document.createElementNS(kXULNS, "menuitem");
+          label = stringBundle.getString("cmd_clearHistory");
+          akey = stringBundle.getString("cmd_clearHistory_accesskey");
+          element.setAttribute("label", label);
+          element.setAttribute("accesskey", akey);
+          element.setAttribute("cmd", "cmd_clearhistory");
+          aMenu.appendChild(element);
+
+          element = document.createElementNS(kXULNS, "menuitem");
+          label = stringBundle.getString("cmd_showSuggestions");
+          akey = stringBundle.getString("cmd_showSuggestions_accesskey");
+          element.setAttribute("anonid", "toggle-suggest-item");
+          element.setAttribute("label", label);
+          element.setAttribute("accesskey", akey);
+          element.setAttribute("cmd", "cmd_togglesuggest");
+          element.setAttribute("type", "checkbox");
+          element.setAttribute("autocheck", "false");
+          suggestMenuItem = element;
+          aMenu.appendChild(element);
+
+          if (AppConstants.platform == "macosx") {
+            this.addEventListener("keypress", aEvent => {
+              if (aEvent.keyCode == KeyEvent.DOM_VK_F4)
+                this.openSearch()
+            }, true);
+          }
+
+          this.controllers.appendController(this.searchbarController);
+
+          let onpopupshowing = function() {
+            BrowserSearch.searchBar._textbox.closePopup();
+            if (suggestMenuItem) {
+              let enabled =
+                Services.prefs.getBoolPref("browser.search.suggest.enabled");
+              suggestMenuItem.setAttribute("checked", enabled);
+            }
+
+            if (!pasteAndSearch)
+              return;
+            let controller = document.commandDispatcher.getControllerForCommand("cmd_paste");
+            let enabled = controller.isCommandEnabled("cmd_paste");
+            if (enabled)
+              pasteAndSearch.removeAttribute("disabled");
+            else
+              pasteAndSearch.setAttribute("disabled", "true");
+          };
+          aMenu.addEventListener("popupshowing", onpopupshowing);
+          onpopupshowing();
+        ]]></body>
+      </method>
 
       <!--
         This overrides the searchParam property in autocomplete.xml.  We're
         hijacking this property as a vehicle for delivering the privacy
         information about the window into the guts of nsSearchSuggestions.
 
         Note that the setter is the same as the parent.  We were not sure whether
         we can override just the getter.  If that proves to be the case, the setter
@@ -744,29 +760,16 @@
             popup.setAttribute("width", width > 100 ? width : 100);
 
             var yOffset = outerRect.bottom - innerRect.bottom;
             popup.openPopup(this.inputField, "after_start", 0, yOffset, false, false);
           }
         ]]></body>
       </method>
 
-      <method name="observe">
-        <parameter name="aSubject"/>
-        <parameter name="aTopic"/>
-        <parameter name="aData"/>
-        <body><![CDATA[
-          if (aTopic == "nsPref:changed") {
-            this._suggestEnabled =
-              Services.prefs.getBoolPref("browser.search.suggest.enabled");
-            this._suggestMenuItem.setAttribute("checked", this._suggestEnabled);
-          }
-        ]]></body>
-      </method>
-
       <method name="openSearch">
         <body>
           <![CDATA[
             if (!this.popupOpen) {
               document.getBindingParent(this).openSuggestionsPanel();
               return false;
             }
             return true;
@@ -858,20 +861,20 @@
           switch (aCommand) {
             case "cmd_clearhistory":
               var param = this._self.getAttribute("autocompletesearchparam");
 
               BrowserSearch.searchBar.FormHistory.update({ op: "remove", fieldname: param }, null);
               this._self.value = "";
               break;
             case "cmd_togglesuggest":
-              // The pref observer will update _suggestEnabled and the menu
-              // checkmark.
+              let enabled =
+                Services.prefs.getBoolPref("browser.search.suggest.enabled");
               Services.prefs.setBoolPref("browser.search.suggest.enabled",
-                                         !this._self._suggestEnabled);
+                                         !enabled);
               break;
             default:
               // do nothing with unrecognized command
           }
         }
       })]]></field>
     </implementation>
 
--- a/browser/components/search/test/browser_426329.js
+++ b/browser/components/search/test/browser_426329.js
@@ -227,16 +227,24 @@ add_task(async function testAutocomplete
   var popup = searchBar.textbox.popup;
   let popupShownPromise = BrowserTestUtils.waitForEvent(popup, "popupshown");
   searchBar.textbox.showHistoryPopup();
   await popupShownPromise;
   checkMenuEntries(searchEntries);
 });
 
 add_task(async function testClearHistory() {
+  // Open the textbox context menu to trigger controller attachment.
+  let textbox = searchBar.textbox;
+  let popupShownPromise = BrowserTestUtils.waitForEvent(textbox, "popupshown");
+  EventUtils.synthesizeMouseAtCenter(textbox, { type: "contextmenu", button: 2 });
+  await popupShownPromise;
+  // Close the context menu.
+  EventUtils.synthesizeKey("VK_ESCAPE", {});
+
   let controller = searchBar.textbox.controllers.getControllerForCommand("cmd_clearhistory")
   ok(controller.isCommandEnabled("cmd_clearhistory"), "Clear history command enabled");
   controller.doCommand("cmd_clearhistory");
   let count = await countEntries();
   ok(count == 0, "History cleared");
 });
 
 add_task(async function asyncCleanup() {
--- a/browser/components/tests/startupRecorder.js
+++ b/browser/components/tests/startupRecorder.js
@@ -1,16 +1,22 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const {classes: Cc, utils: Cu, interfaces: Ci} = Components;
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
+Cu.import("resource://gre/modules/AppConstants.jsm");
+
+let firstPaintNotification = "widget-first-paint";
+// widget-first-paint fires much later than expected on Linux.
+if (AppConstants.platform == "linux")
+  firstPaintNotification = "xul-window-visible";
 
 /**
   * The startupRecorder component observes notifications at various stages of
   * startup and records the set of JS components and modules that were already
   * loaded at each of these points.
   * The records are meant to be used by startup tests in
   * browser/base/content/test/performance
   * This component only exists in nightly and debug builds, it doesn't ship in
@@ -37,17 +43,17 @@ startupRecorder.prototype = {
 
     if (topic == "app-startup") {
       // We can't ensure our observer will be called first or last, so the list of
       // topics we observe here should avoid the topics used to trigger things
       // during startup (eg. the topics observed by nsBrowserGlue.js).
       let topics = [
         "profile-do-change", // This catches stuff loaded during app-startup
         "toplevel-window-ready", // Catches stuff from final-ui-startup
-        "widget-first-paint",
+        firstPaintNotification,
         "sessionstore-windows-restored",
       ];
       for (let t of topics)
         Services.obs.addObserver(this, t);
       return;
     }
 
     Services.obs.removeObserver(this, topic);
@@ -56,16 +62,16 @@ startupRecorder.prototype = {
       // We use idleDispatch here to record the set of loaded scripts after we
       // are fully done with startup and ready to react to user events.
       Services.tm.mainThread.idleDispatch(
         this.record.bind(this, "before handling user events"));
     } else {
       const topicsToNames = {
         "profile-do-change": "before profile selection",
         "toplevel-window-ready": "before opening first browser window",
-        "widget-first-paint": "before first paint",
       };
+      topicsToNames[firstPaintNotification] = "before first paint";
       this.record(topicsToNames[topic]);
     }
   }
 };
 
 this.NSGetFactory = XPCOMUtils.generateNSGetFactory([startupRecorder]);