Backed out changeset 89dd5f05ca91 (bug 1678255) for causing assertion failures in BasePrincipal.cpp CLOSED TREE
authorNoemi Erli <nerli@mozilla.com>
Thu, 11 Feb 2021 06:15:26 +0200
changeset 566934 0478359566a06f7354b17f583cff93c55bffef42
parent 566933 690c6a094f8a4801f2cd8c152f13028f31e32d4b
child 566935 8afa9c8f3bdc10509d627cc82d200e1ea9ea2159
push id136220
push usernerli@mozilla.com
push dateThu, 11 Feb 2021 04:15:57 +0000
treeherderautoland@0478359566a0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1678255
milestone87.0a1
backs out89dd5f05ca91f99ea872ff499282b40d60c37b86
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
Backed out changeset 89dd5f05ca91 (bug 1678255) for causing assertion failures in BasePrincipal.cpp CLOSED TREE
docshell/base/nsDocShell.cpp
dom/ipc/ContentParent.cpp
dom/ipc/ContentParent.h
dom/ipc/PContent.ipdl
toolkit/mozapps/handling/ContentDispatchChooser.jsm
uriloader/exthandler/nsExternalHelperAppService.cpp
uriloader/exthandler/nsExternalHelperAppService.h
uriloader/exthandler/nsExternalProtocolHandler.cpp
uriloader/exthandler/nsIContentDispatchChooser.idl
uriloader/exthandler/nsIExternalProtocolService.idl
uriloader/exthandler/tests/mochitest/browser.ini
uriloader/exthandler/tests/mochitest/browser_protocol_ask_dialog_external.js
uriloader/exthandler/tests/mochitest/head.js
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -12761,18 +12761,17 @@ nsresult nsDocShell::OnLinkClickSync(nsI
         // if the URL scheme does not correspond to an exposed protocol, then
         // we need to hand this link click over to the external protocol
         // handler.
         bool isExposed;
         nsresult rv =
             extProtService->IsExposedProtocol(scheme.get(), &isExposed);
         if (NS_SUCCEEDED(rv) && !isExposed) {
           return extProtService->LoadURI(aLoadState->URI(), triggeringPrincipal,
-                                         mBrowsingContext,
-                                         /* aTriggeredExternally */ false);
+                                         mBrowsingContext);
         }
       }
     }
   }
   uint32_t triggeringSandboxFlags = 0;
   if (mBrowsingContext) {
     triggeringSandboxFlags = mBrowsingContext->GetSandboxFlags();
   }
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -4468,35 +4468,33 @@ mozilla::ipc::IPCResult ContentParent::R
   }
   nsMixedContentBlocker::AccumulateMixedContentHSTS(aURI, aActive,
                                                     aOriginAttributes);
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult ContentParent::RecvLoadURIExternal(
     nsIURI* uri, nsIPrincipal* aTriggeringPrincipal,
-    const MaybeDiscarded<BrowsingContext>& aContext,
-    bool aWasExternallyTriggered) {
+    const MaybeDiscarded<BrowsingContext>& aContext) {
   if (aContext.IsDiscarded()) {
     return IPC_OK();
   }
 
   nsCOMPtr<nsIExternalProtocolService> extProtService(
       do_GetService(NS_EXTERNALPROTOCOLSERVICE_CONTRACTID));
   if (!extProtService) {
     return IPC_OK();
   }
 
   if (!uri) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   BrowsingContext* bc = aContext.get();
-  extProtService->LoadURI(uri, aTriggeringPrincipal, bc,
-                          aWasExternallyTriggered);
+  extProtService->LoadURI(uri, aTriggeringPrincipal, bc);
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult ContentParent::RecvExtProtocolChannelConnectParent(
     const uint64_t& registrarId) {
   nsresult rv;
 
   // First get the real channel created before redirect on the parent.
--- a/dom/ipc/ContentParent.h
+++ b/dom/ipc/ContentParent.h
@@ -1082,18 +1082,17 @@ class ContentParent final
   mozilla::ipc::IPCResult RecvOpenNotificationSettings(
       const IPC::Principal& aPrincipal);
 
   mozilla::ipc::IPCResult RecvNotificationEvent(
       const nsString& aType, const NotificationEventData& aData);
 
   mozilla::ipc::IPCResult RecvLoadURIExternal(
       nsIURI* uri, nsIPrincipal* triggeringPrincipal,
-      const MaybeDiscarded<BrowsingContext>& aContext,
-      bool aWasExternallyTriggered);
+      const MaybeDiscarded<BrowsingContext>& aContext);
   mozilla::ipc::IPCResult RecvExtProtocolChannelConnectParent(
       const uint64_t& registrarId);
 
   mozilla::ipc::IPCResult RecvSyncMessage(
       const nsString& aMsg, const ClonedMessageData& aData,
       nsTArray<StructuredCloneData>* aRetvals);
 
   mozilla::ipc::IPCResult RecvAsyncMessage(const nsString& aMsg,
--- a/dom/ipc/PContent.ipdl
+++ b/dom/ipc/PContent.ipdl
@@ -1042,20 +1042,17 @@ parent:
 
     async PBenchmarkStorage();
 
     // Services remoting
 
     async StartVisitedQueries(nsIURI[] uri);
     async SetURITitle(nsIURI uri, nsString title);
 
-    async LoadURIExternal(nsIURI uri,
-                          nsIPrincipal triggeringPrincipal,
-                          MaybeDiscardedBrowsingContext browsingContext,
-                          bool wasExternallyTriggered);
+    async LoadURIExternal(nsIURI uri, nsIPrincipal triggeringPrincipal, MaybeDiscardedBrowsingContext browsingContext);
     async ExtProtocolChannelConnectParent(uint64_t registrarId);
 
     // PrefService message
     sync GetGfxVars() returns (GfxVarUpdate[] vars);
 
     sync SyncMessage(nsString aMessage, ClonedMessageData aData)
       returns (StructuredCloneData[] retval);
 
--- a/toolkit/mozapps/handling/ContentDispatchChooser.jsm
+++ b/toolkit/mozapps/handling/ContentDispatchChooser.jsm
@@ -14,24 +14,16 @@ const DIALOG_URL_APP_CHOOSER =
 const DIALOG_URL_PERMISSION =
   "chrome://mozapps/content/handling/permissionDialog.xhtml";
 
 var EXPORTED_SYMBOLS = [
   "nsContentDispatchChooser",
   "ContentDispatchChooserTelemetry",
 ];
 
-const gPrefs = {};
-XPCOMUtils.defineLazyPreferenceGetter(
-  gPrefs,
-  "promptForExternal",
-  "network.protocol-handler.prompt-from-external",
-  true
-);
-
 const PROTOCOL_HANDLER_OPEN_PERM_KEY = "open-protocol-handler";
 const PERMISSION_KEY_DELIMITER = "^";
 
 let ContentDispatchChooserTelemetry = {
   /**
    * Maps protocol scheme to telemetry label.
    */
   SCHEME_TO_LABEL: {
@@ -241,37 +233,23 @@ class nsContentDispatchChooser {
    * If the triggering principal doesn't have permission to open apps for the
    * protocol of aURI, we show a permission prompt first.
    * If the caller has permission and a preferred handler is set, we skip the
    * dialogs and directly open the handler.
    * @param {nsIHandlerInfo} aHandler - Info about protocol and handlers.
    * @param {nsIURI} aURI - URI to be handled.
    * @param {nsIPrincipal} [aPrincipal] - Principal which triggered the load.
    * @param {BrowsingContext} [aBrowsingContext] - Context of the load.
-   * @param {bool} [aTriggeredExternally] - Whether the load came from outside
-   * this application.
    */
-  async handleURI(
-    aHandler,
-    aURI,
-    aPrincipal,
-    aBrowsingContext,
-    aTriggeredExternally = false
-  ) {
+  async handleURI(aHandler, aURI, aPrincipal, aBrowsingContext) {
     let callerHasPermission = this._hasProtocolHandlerPermission(
       aHandler.type,
       aPrincipal
     );
 
-    // Force showing the dialog for links passed from outside the application.
-    // This avoids infinite loops, see bug 1678255, bug 1667468, etc.
-    if (aTriggeredExternally && gPrefs.promptForExternal) {
-      aHandler.alwaysAskBeforeHandling = true;
-    }
-
     // Skip the dialog if a preferred application is set and the caller has
     // permission.
     if (
       callerHasPermission &&
       !aHandler.alwaysAskBeforeHandling &&
       (aHandler.preferredAction == Ci.nsIHandlerInfo.useHelperApp ||
         aHandler.preferredAction == Ci.nsIHandlerInfo.useSystemDefault)
     ) {
--- a/uriloader/exthandler/nsExternalHelperAppService.cpp
+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
@@ -1005,23 +1005,22 @@ NS_IMETHODIMP nsExternalHelperAppService
 static const char kExternalProtocolPrefPrefix[] =
     "network.protocol-handler.external.";
 static const char kExternalProtocolDefaultPref[] =
     "network.protocol-handler.external-default";
 
 NS_IMETHODIMP
 nsExternalHelperAppService::LoadURI(nsIURI* aURI,
                                     nsIPrincipal* aTriggeringPrincipal,
-                                    BrowsingContext* aBrowsingContext,
-                                    bool aTriggeredExternally) {
+                                    BrowsingContext* aBrowsingContext) {
   NS_ENSURE_ARG_POINTER(aURI);
 
   if (XRE_IsContentProcess()) {
     mozilla::dom::ContentChild::GetSingleton()->SendLoadURIExternal(
-        aURI, aTriggeringPrincipal, aBrowsingContext, aTriggeredExternally);
+        aURI, aTriggeringPrincipal, aBrowsingContext);
     return NS_OK;
   }
 
   nsAutoCString spec;
   aURI->GetSpec(spec);
 
   if (spec.Find("%00") != -1) return NS_ERROR_MALFORMED_URI;
 
@@ -1113,17 +1112,17 @@ nsExternalHelperAppService::LoadURI(nsIU
   rv = GetProtocolHandlerInfo(scheme, getter_AddRefs(handler));
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<nsIContentDispatchChooser> chooser =
       do_CreateInstance("@mozilla.org/content-dispatch-chooser;1", &rv);
   NS_ENSURE_SUCCESS(rv, rv);
 
   return chooser->HandleURI(handler, uri, aTriggeringPrincipal,
-                            aBrowsingContext, aTriggeredExternally);
+                            aBrowsingContext);
 }
 
 //////////////////////////////////////////////////////////////////////////////////////////////////////
 // Methods related to deleting temporary files on exit
 //////////////////////////////////////////////////////////////////////////////////////////////////////
 
 /* static */
 nsresult nsExternalHelperAppService::DeleteTemporaryFileHelper(
--- a/uriloader/exthandler/nsExternalHelperAppService.h
+++ b/uriloader/exthandler/nsExternalHelperAppService.h
@@ -77,18 +77,17 @@ class nsExternalHelperAppService : publi
    */
   NS_IMETHOD ExternalProtocolHandlerExists(const char* aProtocolScheme,
                                            bool* aHandlerExists) override;
   NS_IMETHOD IsExposedProtocol(const char* aProtocolScheme,
                                bool* aResult) override;
   NS_IMETHOD GetProtocolHandlerInfo(const nsACString& aScheme,
                                     nsIHandlerInfo** aHandlerInfo) override;
   NS_IMETHOD LoadURI(nsIURI* aURI, nsIPrincipal* aTriggeringPrincipal,
-                     mozilla::dom::BrowsingContext* aBrowsingContext,
-                     bool aWasTriggeredExternally) override;
+                     mozilla::dom::BrowsingContext* aBrowsingContext) override;
   NS_IMETHOD SetProtocolHandlerDefaults(nsIHandlerInfo* aHandlerInfo,
                                         bool aOSHandlerExists) override;
 
   /**
    * Given a string identifying an application, create an nsIFile representing
    * it. This function should look in $PATH for the application.
    * The base class implementation will first try to interpret platformAppPath
    * as an absolute path, and if that fails it will look for a file next to the
--- a/uriloader/exthandler/nsExternalProtocolHandler.cpp
+++ b/uriloader/exthandler/nsExternalProtocolHandler.cpp
@@ -160,18 +160,17 @@ nsresult nsExtProtocolChannel::OpenURL()
 
     RefPtr<mozilla::dom::BrowsingContext> ctx;
     rv = mLoadInfo->GetTargetBrowsingContext(getter_AddRefs(ctx));
     if (NS_FAILED(rv)) {
       goto finish;
     }
 
     RefPtr<nsIPrincipal> principal = mLoadInfo->TriggeringPrincipal();
-    rv = extProtService->LoadURI(mUrl, principal, ctx,
-                                 mLoadInfo->GetLoadTriggeredFromExternal());
+    rv = extProtService->LoadURI(mUrl, principal, ctx);
 
     if (NS_SUCCEEDED(rv) && mListener) {
       mStatus = NS_ERROR_NO_CONTENT;
 
       RefPtr<nsExtProtocolChannel> self = this;
       nsCOMPtr<nsIStreamListener> listener = mListener;
       MessageLoop::current()->PostTask(NS_NewRunnableFunction(
           "nsExtProtocolChannel::OpenURL", [self, listener]() {
--- a/uriloader/exthandler/nsIContentDispatchChooser.idl
+++ b/uriloader/exthandler/nsIContentDispatchChooser.idl
@@ -24,18 +24,15 @@ interface nsIContentDispatchChooser : ns
   *        The interface describing the details of how this content should or
   *        can be handled.
   * @param aURI
   *        The URI of the resource that we are asking about.
   * @param aTriggeringPrincipal
   *        The principal making the request.
   * @param aBrowsingContext
   *        The browsing context where the load should happen.
-  * @param aWasTriggeredExternally
-  *        True if the load was tripped by an external app.
   */
   void handleURI(in nsIHandlerInfo aHandler,
            in nsIURI aURI,
            in nsIPrincipal aTriggeringPrincipal,
-           in BrowsingContext aBrowsingContext,
-           [optional] in bool aWasTriggeredExternally);
+           in BrowsingContext aBrowsingContext);
 };
 
--- a/uriloader/exthandler/nsIExternalProtocolService.idl
+++ b/uriloader/exthandler/nsIExternalProtocolService.idl
@@ -104,27 +104,23 @@ interface nsIExternalProtocolService : n
    * @param aBrowsingContext
    *        The context to parent the dialog against, and, if a web handler
    *        is chosen, it is loaded in this window as well.  This parameter
    *        may be ultimately passed nsIURILoader.openURI in the case of a
    *        web handler, and aWindowContext is null or not present, web
    *        handlers will fail.  We need to do better than that; bug 394483
    *        filed in order to track.
    *
-   * @param aWasTriggeredExternally
-   *        If true, indicates the load was initiated by an external app.
-   *
    * @note  Embedders that do not expose the http protocol should not currently
    *        use web-based protocol handlers, as handoff won't work correctly
    *        (bug 394479).
    */
   void loadURI(in nsIURI aURI,
                [optional] in nsIPrincipal aTriggeringPrincipal,
-               [optional] in BrowsingContext aBrowsingContext,
-               [optional] in bool aWasTriggeredExternally);
+               [optional] in BrowsingContext aBrowsingContext);
 
   /**
    * Gets a human-readable description for the application responsible for
    * handling a specific protocol.
    *
    * @param aScheme The scheme to look up. For example, "mms".
    *
    * @throw NS_ERROR_NOT_IMPLEMENTED
--- a/uriloader/exthandler/tests/mochitest/browser.ini
+++ b/uriloader/exthandler/tests/mochitest/browser.ini
@@ -40,13 +40,12 @@ support-files =
   file_pdf_application_pdf.pdf
   file_pdf_application_pdf.pdf^headers^
 [browser_protocol_ask_dialog.js]
 support-files =
   file_nested_protocol_request.html
 [browser_first_prompt_not_blocked_without_user_interaction.js]
 support-files =
   file_external_protocol_iframe.html
-[browser_protocol_ask_dialog_external.js]
 [browser_protocol_ask_dialog_permission.js]
 [browser_protocolhandler_loop.js]
 [browser_remember_download_option.js]
 [browser_web_protocol_handlers.js]
deleted file mode 100644
--- a/uriloader/exthandler/tests/mochitest/browser_protocol_ask_dialog_external.js
+++ /dev/null
@@ -1,106 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-ChromeUtils.import(
-  "resource://testing-common/HandlerServiceTestUtils.jsm",
-  this
-);
-
-let gHandlerService = Cc["@mozilla.org/uriloader/handler-service;1"].getService(
-  Ci.nsIHandlerService
-);
-
-/**
- * Creates dummy protocol handler
- */
-function initTestHandlers() {
-  let handlerInfo = HandlerServiceTestUtils.getBlankHandlerInfo("yoink");
-
-  let appHandler = Cc[
-    "@mozilla.org/uriloader/local-handler-app;1"
-  ].createInstance(Ci.nsILocalHandlerApp);
-  // This is a dir and not executable, but that's enough for here.
-  appHandler.executable = Services.dirsvc.get("XCurProcD", Ci.nsIFile);
-  handlerInfo.possibleApplicationHandlers.appendElement(appHandler);
-  handlerInfo.preferredApplicationHandler = appHandler;
-  handlerInfo.preferredAction = handlerInfo.useHelperApp;
-  handlerInfo.alwaysAskBeforeHandling = false;
-  gHandlerService.store(handlerInfo);
-  registerCleanupFunction(() => {
-    gHandlerService.remove(handlerInfo);
-  });
-}
-
-/**
- * Check that if we get a direct request from another app / the OS to open a
- * link, we always prompt, even if we think we know what the correct answer
- * is. This is to avoid infinite loops in such situations where the OS and
- * Firefox have conflicting ideas about the default handler, or where our
- * checks with the OS don't work (Linux and/or Snap, at time of this comment).
- */
-add_task(async function test_external_asks_anyway() {
-  await SpecialPowers.pushPrefEnv({
-    set: [["network.protocol-handler.prompt-from-external", true]],
-  });
-  initTestHandlers();
-
-  let cmdLineHandler = Cc["@mozilla.org/browser/final-clh;1"].getService(
-    Ci.nsICommandLineHandler
-  );
-  let fakeCmdLine = {
-    length: 1,
-    _arg: "yoink:yoink",
-
-    getArgument(aIndex) {
-      if (aIndex == 0) {
-        return this._arg;
-      }
-      throw Components.Exception("", Cr.NS_ERROR_INVALID_ARG);
-    },
-
-    findFlag() {
-      return -1;
-    },
-
-    handleFlagWithParam() {
-      if (this._argCount) {
-        this._argCount = 0;
-        return this._arg;
-      }
-
-      return "";
-    },
-
-    state: 2,
-
-    STATE_INITIAL_LAUNCH: 0,
-    STATE_REMOTE_AUTO: 1,
-    STATE_REMOTE_EXPLICIT: 2,
-
-    preventDefault: false,
-
-    resolveURI() {
-      return Services.io.newURI(this._arg);
-    },
-    QueryInterface: ChromeUtils.generateQI(["nsICommandLine"]),
-  };
-  let chooserDialogOpenPromise = waitForProtocolAppChooserDialog(
-    gBrowser,
-    true
-  );
-  cmdLineHandler.handle(fakeCmdLine);
-  let dialog = await chooserDialogOpenPromise;
-  ok(dialog, "Should have prompted.");
-
-  let dialogClosedPromise = waitForProtocolAppChooserDialog(
-    gBrowser.selectedBrowser,
-    false
-  );
-  let dialogEl = dialog._frame.contentDocument.querySelector("dialog");
-  dialogEl.cancelDialog();
-  await dialogClosedPromise;
-  // We will have opened a tab; close it.
-  BrowserTestUtils.removeTab(gBrowser.selectedTab);
-});
--- a/uriloader/exthandler/tests/mochitest/head.js
+++ b/uriloader/exthandler/tests/mochitest/head.js
@@ -120,45 +120,30 @@ async function openHelperAppDialog(launc
     dlg.location.href,
     "chrome://mozapps/content/downloads/unknownContentType.xhtml",
     "Got correct dialog"
   );
 
   return dlg;
 }
 
-/**
- * Wait for a subdialog event indicating a dialog either opened
- * or was closed.
- *
- * First argument is the browser in which to listen. If a tabbrowser,
- * we listen to subdialogs for any tab of that browser.
- */
 async function waitForSubDialog(browser, url, state) {
   let eventStr = state ? "dialogopen" : "dialogclose";
 
-  let eventTarget;
-
-  // Tabbrowser?
-  if (browser.tabContainer) {
-    eventTarget = browser.tabContainer.ownerDocument.documentElement;
-  } else {
-    // Individual browser. Get its box:
-    let tabDialogBox = browser.ownerGlobal.gBrowser.getTabDialogBox(browser);
-    eventTarget = tabDialogBox.getTabDialogManager()._dialogStack;
-  }
+  let tabDialogBox = gBrowser.getTabDialogBox(browser);
+  let dialogStack = tabDialogBox.getTabDialogManager()._dialogStack;
 
   let checkFn;
 
   if (state) {
     checkFn = dialogEvent => dialogEvent.detail.dialog?._openedURL == url;
   }
 
   let event = await BrowserTestUtils.waitForEvent(
-    eventTarget,
+    dialogStack,
     eventStr,
     true,
     checkFn
   );
 
   let { dialog } = event.detail;
 
   // If the dialog is closing wait for it to be fully closed before resolving