Bug 1547693 - do not bother looking up protocol information with the OS just to store the default shipped options, r=florian

Differential Revision: https://phabricator.services.mozilla.com/D59788

--HG--
extra : moz-landing-system : lando

browser/base/content/test/performance/browser_startup_mainthreadio.js

@@ -357,13 +357,6 @@ const startupPhases = {
       condition: WIN,
       stat: 1,
     },
-    // Bug 1547693
-    {
-      path: "*WindowsApps/microsoft.windowscommunicationsapps*",
-      condition: WIN,
-      ignoreIfUnused: true,
-      stat: 3,
-    },
     // Bug 1545167
     {
       path: "*Microsoft.MicrosoftEdge*",

uriloader/exthandler/HandlerService.js

@@ -116,6 +116,8 @@ HandlerService.prototype = {
         this._store.data.defaultHandlersVersion[
           locale
         ] = prefsDefaultHandlersVersion;
+        // Now save the result:
+        this._store.saveSoon();
       }
     } catch (ex) {
       Cu.reportError(ex);
@@ -151,22 +153,41 @@ HandlerService.prototype = {
       } catch (ex) {}
     }
 
+    // Now, we're going to cheat. Terribly. The idiologically correct way
+    // of implementing the following bit of code would be to fetch the
+    // handler info objects from the protocol service, manipulate those,
+    // and then store each of them.
+    // However, that's expensive. It causes us to talk to the OS about
+    // default apps, which causes the OS to go hit the disk.
+    // All we're trying to do is insert some web apps into the list. We
+    // don't care what's already in the file, we just want to do the
+    // equivalent of appending into the database. So let's just go do that:
     for (let scheme of Object.keys(schemes)) {
-      let protoInfo = gExternalProtocolService.getProtocolHandlerInfo(scheme);
-
-      // cache the possible handlers to avoid extra xpconnect traversals.
-      let possibleHandlers = protoInfo.possibleApplicationHandlers;
-
-      for (let handlerNumber of Object.keys(schemes[scheme])) {
-        let handlerApp = this.handlerAppFromSerializable(
-          schemes[scheme][handlerNumber]
-        );
-        // If there is already a handler registered with the same template
-        // URL, the newly added one will be ignored when saving.
-        possibleHandlers.appendElement(handlerApp);
+      let existingSchemeInfo = this._store.data.schemes[scheme];
+      if (!this._store.data.schemes[scheme]) {
+        // Haven't seen this scheme before. Default to asking which app the
+        // user wants to use:
+        existingSchemeInfo = {
+          // Signal to future readers that we didn't ask the OS anything.
+          // When the entry is first used, get the info from the OS.
+          stubEntry: true,
+          // The first item in the list is the preferred handler, and
+          // there isn't one, so we fill in null:
+          handlers: [null],
+        };
+        this._store.data.schemes[scheme] = existingSchemeInfo;
+      }
+      let { handlers } = existingSchemeInfo;
+      for (let handlerNumber of Object.keys(schemes[scheme])) {
+        let newHandler = schemes[scheme][handlerNumber];
+        // If there is already a handler registered with the same template
+        // URL, ignore the new one:
+        let matchingTemplate = handler =>
+          handler && handler.uriTemplate == newHandler.uriTemplate;
+        if (!handlers.some(matchingTemplate)) {
+          handlers.push(newHandler);
+        }
       }
-
-      this.store(protoInfo);
     }
   },
 
@@ -396,6 +417,9 @@ HandlerService.prototype = {
       }
     }
 
+    // If we're saving *anything*, it stops being a stub:
+    delete storedHandlerInfo.stubEntry;
+
     this._store.saveSoon();
   },
 
@@ -412,22 +436,33 @@ HandlerService.prototype = {
       );
     }
 
-    handlerInfo.preferredAction = storedHandlerInfo.action;
-    handlerInfo.alwaysAskBeforeHandling = !!storedHandlerInfo.ask;
-
-    // If the first item is not null, it is also the preferred handler. Since
-    // we cannot modify the stored array, use a boolean to keep track of this.
-    let isFirstItem = true;
-    for (let handler of storedHandlerInfo.handlers || [null]) {
-      let handlerApp = this.handlerAppFromSerializable(handler || {});
-      if (isFirstItem) {
-        isFirstItem = false;
-        handlerInfo.preferredApplicationHandler = handlerApp;
-      }
-      if (handlerApp) {
-        handlerInfo.possibleApplicationHandlers.appendElement(handlerApp);
+    let isStub = !!storedHandlerInfo.stubEntry;
+    // In the normal case, this is not a stub, so we can just read stored info
+    // and write to the handlerInfo object we were passed.
+    if (!isStub) {
+      handlerInfo.preferredAction = storedHandlerInfo.action;
+      handlerInfo.alwaysAskBeforeHandling = !!storedHandlerInfo.ask;
+    } else {
+      // If we've got a stub, ensure the defaults are still set:
+      gExternalProtocolService.setProtocolHandlerDefaults(
+        handlerInfo,
+        handlerInfo.hasDefaultHandler
+      );
+      if (
+        handlerInfo.preferredAction == Ci.nsIHandlerInfo.alwaysAsk &&
+        handlerInfo.alwaysAskBeforeHandling
+      ) {
+        // `store` will default to `useHelperApp` because `alwaysAsk` is
+        // not one of the 3 recognized options; for compatibility, do
+        // the same here.
+        handlerInfo.preferredAction = Ci.nsIHandlerInfo.useHelperApp;
       }
     }
+    // If it *is* a stub, don't override alwaysAskBeforeHandling or the
+    // preferred actions. Instead, just append the stored handlers, without
+    // overriding the preferred app, and then schedule a task to store proper
+    // info for this handler.
+    this._appendStoredHandlers(handlerInfo, storedHandlerInfo.handlers, isStub);
 
     if (this._isMIMEInfo(handlerInfo) && storedHandlerInfo.extensions) {
       for (let extension of storedHandlerInfo.extensions) {
@@ -436,6 +471,34 @@ HandlerService.prototype = {
     }
   },
 
+  /**
+   * Private method to inject stored handler information into an nsIHandlerInfo
+   * instance.
+   * @param handlerInfo           the nsIHandlerInfo instance to write to
+   * @param storedHandlers        the stored handlers
+   * @param keepPreferredApp      whether to keep the handlerInfo's
+   *                              preferredApplicationHandler or override it
+   *                              (default: false, ie override it)
+   */
+  _appendStoredHandlers(handlerInfo, storedHandlers, keepPreferredApp) {
+    // If the first item is not null, it is also the preferred handler. Since
+    // we cannot modify the stored array, use a boolean to keep track of this.
+    let isFirstItem = true;
+    for (let handler of storedHandlers || [null]) {
+      let handlerApp = this.handlerAppFromSerializable(handler || {});
+      if (isFirstItem) {
+        isFirstItem = false;
+        // Do not overwrite the preferred app unless that's allowed
+        if (!keepPreferredApp) {
+          handlerInfo.preferredApplicationHandler = handlerApp;
+        }
+      }
+      if (handlerApp) {
+        handlerInfo.possibleApplicationHandlers.appendElement(handlerApp);
+      }
+    }
+  },
+
   /**
    * @param handler
    *        A nsIHandlerApp handler app

uriloader/exthandler/tests/unit/test_defaults_handlerService.js

@@ -0,0 +1,106 @@
+/* Any copyright is dedicated to the Public Domain.
+http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+XPCOMUtils.defineLazyServiceGetter(
+  this,
+  "gExternalProtocolService",
+  "@mozilla.org/uriloader/external-protocol-service;1",
+  "nsIExternalProtocolService"
+);
+
+const kDefaultHandlerList = Services.prefs
+  .getChildList("gecko.handlerService.schemes")
+  .filter(p => {
+    try {
+      let val = Services.prefs.getComplexValue(p, Ci.nsIPrefLocalizedString)
+        .data;
+      return !!val;
+    } catch (ex) {
+      return false;
+    }
+  });
+
+add_task(async function test_check_defaults_get_added() {
+  let protocols = new Set(
+    kDefaultHandlerList.map(p => p.match(/schemes\.(\w+)/)[1])
+  );
+  for (let protocol of protocols) {
+    const kPrefStr = `schemes.${protocol}.`;
+    let matchingPrefs = kDefaultHandlerList.filter(p => p.includes(kPrefStr));
+    let protocolHandlerCount = matchingPrefs.length / 2;
+    Assert.ok(
+      protocolHandlerCount,
+      `Prefs for ${protocol} have at least 1 protocol handler`
+    );
+    Assert.ok(
+      gHandlerService.wrappedJSObject._store.data.schemes[protocol].stubEntry,
+      `Expect stub for ${protocol}`
+    );
+    let info = gExternalProtocolService.getProtocolHandlerInfo(protocol, {});
+    Assert.ok(
+      info,
+      `Should be able to get protocol handler info for ${protocol}`
+    );
+    let handlers = Array.from(
+      info.possibleApplicationHandlers.enumerate(Ci.nsIHandlerApp)
+    );
+    handlers = handlers.filter(h => h instanceof Ci.nsIWebHandlerApp);
+    Assert.equal(
+      handlers.length,
+      protocolHandlerCount,
+      `Default web handlers for ${protocol} should match`
+    );
+    let { alwaysAskBeforeHandling, preferredAction } = info;
+    // Actually store something, pretending there was a change:
+    let infoToWrite = gExternalProtocolService.getProtocolHandlerInfo(
+      protocol,
+      {}
+    );
+    gHandlerService.store(infoToWrite);
+    ok(
+      !gHandlerService.wrappedJSObject._store.data.schemes[protocol].stubEntry,
+      "Expect stub entry info to go away"
+    );
+
+    let newInfo = gExternalProtocolService.getProtocolHandlerInfo(protocol, {});
+    Assert.equal(
+      alwaysAskBeforeHandling,
+      newInfo.alwaysAskBeforeHandling,
+      protocol + " - always ask shouldn't change"
+    );
+    Assert.equal(
+      preferredAction,
+      newInfo.preferredAction,
+      protocol + " - preferred action shouldn't change"
+    );
+    await deleteHandlerStore();
+  }
+});
+
+add_task(async function test_check_default_modification() {
+  let mailtoHandlerCount =
+    kDefaultHandlerList.filter(p => p.includes("mailto")).length / 2;
+  Assert.ok(mailtoHandlerCount, "Prefs have at least 1 mailto handler");
+  Assert.ok(
+    true,
+    JSON.stringify(gHandlerService.wrappedJSObject._store.data.schemes.mailto)
+  );
+  Assert.ok(
+    gHandlerService.wrappedJSObject._store.data.schemes.mailto.stubEntry,
+    "Expect stub for mailto"
+  );
+  let mailInfo = gExternalProtocolService.getProtocolHandlerInfo("mailto", {});
+  mailInfo.alwaysAskBeforeHandling = false;
+  mailInfo.preferredAction = Ci.nsIHandlerInfo.useSystemDefault;
+  gHandlerService.store(mailInfo);
+  Assert.ok(
+    !gHandlerService.wrappedJSObject._store.data.schemes.mailto.stubEntry,
+    "Stub entry should be removed immediately."
+  );
+  let newMail = gExternalProtocolService.getProtocolHandlerInfo("mailto", {});
+  Assert.equal(newMail.preferredAction, Ci.nsIHandlerInfo.useSystemDefault);
+  Assert.equal(newMail.alwaysAskBeforeHandling, false);
+  await deleteHandlerStore();
+});

uriloader/exthandler/tests/unit/test_handlerService.js

@@ -175,16 +175,14 @@ function run_test() {
   protoInfo = protoSvc.getProtocolHandlerInfo("mailto");
   if (haveDefaultHandlersVersion) {
     Assert.equal(2, protoInfo.possibleApplicationHandlers.length);
-    // Win7+ or Linux's GIO may have no default mailto: handler. Otherwise
-    // alwaysAskBeforeHandling is expected to be false here, because although
-    // the pref is true, the value in RDF is false. The injected mailto handler
-    // carried over the default pref value, and so when we set the pref above
-    // to true it's ignored.
-    if (noMailto) {
-      Assert.ok(protoInfo.alwaysAskBeforeHandling);
-    } else {
-      Assert.ok(!protoInfo.alwaysAskBeforeHandling);
-    }
+    // Win7+ or Linux's GIO may have no default mailto: handler, so we'd ask
+    // anyway. Otherwise, the default handlers will not have stored preferred
+    // actions etc., so re-requesting them after the warning pref has changed
+    // will use the updated pref value. So both when we have and do not have
+    // a default mailto: handler, we'll ask:
+    Assert.ok(protoInfo.alwaysAskBeforeHandling);
+    // As soon as anyone actually stores updated defaults into the profile
+    // database, that default will stop tracking the warning pref.
   } else {
     Assert.equal(0, protoInfo.possibleApplicationHandlers.length);
     Assert.ok(protoInfo.alwaysAskBeforeHandling);

uriloader/exthandler/tests/unit/xpcshell.ini

@@ -3,6 +3,8 @@ head = head.js
 run-sequentially = Bug 912235 - Intermittent failures
 firefox-appdir = browser
 
+[test_defaults_handlerService.js]
+skip-if = os == "android" # No default stored handlers on android given lack of support.
 [test_getMIMEInfo_unknown_mime_type.js]
 run-if = os == "win" # Windows only test
 [test_getTypeFromExtension_ext_to_type_mapping.js]