1678255 - Can still end up in infinite protocol handling loop if the user-selected handler hands the URL back to us

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P2)

Description

[:Gijs (he/him)]

STR:

0. install outlook (unclear which versions do this) or some other app that consults the default OS mailto handler if passed a mailto URL
1. set Firefox as default mail client
2. try to email someone in Firefox
3. pick outlook.exe as a handler manually

ER:
you get the same dialog again, or a warning of some kind

AR:
infinite tabs open

We should work around this by checking somewhere along the lines whether we're the OS default handler and the URL was opened by an external app, and in that case, override the "always do X" bit and prompt the user.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:Gijs, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1678255 - prompt for external protocol links whose loads were also triggered externally, instead of looping forever, r?pbz!,nika!

This passes around the "are we external" bit of load information a bunch,
such that the external protocol handling code has access to it.

In this bug and bug 1667468, I think ideally I would have used a check
if we're the OS default for a given protocol before continuing. However,
this information is currently unavailable on Linux (bug 1599713), and
worse, I believe is likely to remain unavailable in flatpak and other
such restricted environments (cf. bug 1618094 - we aren't able to find
out anything about protocol handlers from the OS).

So we could be skeptical about any external URIs passed to Firefox,
and always prompt for those. I was worried about Breaking People's
Workflows, where I don't know whether anyone relies on Firefox happily
passing these URIs along to the relevant application (more convenient
than doing all the registry/API work yourself in scripts!) or anything
like that. I also figured it might be surprising if it suddenly starts
prompting.

So I compromised and added a recency based map of URIs we get. This
way, if we pass a URI to an external app, and within 10s get the same
URI back a second time, we force a prompt.

I'm open to other ideas, and yes this feels yucky, but I think it will
solve the issue in this bug and mitigate bug 1667468 and that issue
on Linux in general, so it seemed worth a shot.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89dd5f05ca91
prompt for external protocol links whose loads were also triggered externally, instead of looping forever, r=pbz,nika

Comment 5

[Noemi Erli[:noemi_erli]]

Backed out changeset 89dd5f05ca91 (Bug 1678255) for causing assertion failures in BasePrincipal.cpp CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Aggad12ARvu9Iyspu4iiOA.1&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux1804-64%2Fdebug-mochitest-browser-chrome-fis-e10s%2Cbc9&revision=89dd5f05ca91f99ea872ff499282b40d60c37b86

Failure log: https://treeherder.mozilla.org/logviewer?job_id=329582821&repo=autoland&lineNumber=13465

Backout: https://hg.mozilla.org/integration/autoland/rev/0478359566a06f7354b17f583cff93c55bffef42

Comment 6

[:Gijs (he/him)]

This seems a fission-only debug-only assertion failure that also breaks external link opens in fission-enabled browsers - we just haven't realized because it's fission-only as well as a debug assert (so normal users don't see it in testing), and because local debug builds won't normally get handed external URIs to load from the commandline, never mind ones that we can't handle ourselves (ie URIs we then have to handle externally). There's very limited testing of our commandline handling infrastructure which is how I won this particular lottery. The assert stack here is:

[task 2021-02-11T04:12:16.970Z] 04:12:16     INFO - GECKO(6921) | Assertion failure: !StringBeginsWith(aOrigin, nsLiteralCString("moz-nullprincipal" ":")) (CreateContentPrincipal does not support NullPrincipal), at /builds/worker/checkouts/gecko/caps/BasePrincipal.cpp:1139
[task 2021-02-11T04:12:16.972Z] 04:12:16     INFO - GECKO(6921) | #01: mozilla::dom::CreateRemoteTypeIsolationPrincipal(nsTSubstring<char> const&) [dom/ipc/ContentParent.cpp:904]
[task 2021-02-11T04:12:16.973Z] 04:12:16     INFO - GECKO(6921) | #02: mozilla::dom::ContentParent::GetUsedBrowserProcess(nsTSubstring<char> const&, nsTArray<mozilla::dom::ContentParent*>&, unsigned int, bool) [dom/ipc/ContentParent.cpp:1014]
[task 2021-02-11T04:12:16.974Z] 04:12:16     INFO - GECKO(6921) | #03: mozilla::dom::ContentParent::GetNewOrUsedLaunchingBrowserProcess(nsTSubstring<char> const&, mozilla::dom::BrowsingContextGroup*, mozilla::hal::ProcessPriority, bool) [dom/ipc/ContentParent.cpp:1070]
[task 2021-02-11T04:12:16.974Z] 04:12:16     INFO - GECKO(6921) | #04: mozilla::dom::ContentParent::GetNewOrUsedBrowserProcess(nsTSubstring<char> const&, mozilla::dom::BrowsingContextGroup*, mozilla::hal::ProcessPriority, bool) [dom/ipc/ContentParent.cpp:1139]
[task 2021-02-11T04:12:16.976Z] 04:12:16     INFO - GECKO(6921) | #05: mozilla::dom::ContentParent::CreateBrowser(mozilla::dom::TabContext const&, mozilla::dom::Element*, nsTSubstring<char> const&, mozilla::dom::BrowsingContext*, mozilla::dom::ContentParent*) [dom/ipc/ContentParent.cpp:1523]
[task 2021-02-11T04:12:16.977Z] 04:12:16     INFO - GECKO(6921) | #06: nsFrameLoader::TryRemoteBrowserInternal() [dom/base/nsFrameLoader.cpp:2629]
[task 2021-02-11T04:12:16.977Z] 04:12:16     INFO - GECKO(6921) | #07: nsFrameLoader::TryRemoteBrowser() [dom/base/nsFrameLoader.cpp:2692]
[task 2021-02-11T04:12:16.978Z] 04:12:16     INFO - GECKO(6921) | #08: nsFrameLoader::ShowRemoteFrame(mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const&, nsSubDocumentFrame*) [dom/base/nsFrameLoader.cpp:1067]
[task 2021-02-11T04:12:16.980Z] 04:12:16     INFO - GECKO(6921) | #09: nsFrameLoader::Show(nsSubDocumentFrame*) [dom/base/nsFrameLoader.cpp:944]
[task 2021-02-11T04:12:16.980Z] 04:12:16     INFO - GECKO(6921) | #10: nsSubDocumentFrame::ShowViewer() [layout/generic/nsSubDocumentFrame.cpp:196]
[task 2021-02-11T04:12:16.981Z] 04:12:16     INFO - GECKO(6921) | #11: AsyncFrameInit::Run() [layout/generic/nsSubDocumentFrame.cpp:98]
[task 2021-02-11T04:12:16.981Z] 04:12:16     INFO - GECKO(6921) | #12: nsContentUtils::RemoveScriptBlocker() [dom/base/nsContentUtils.cpp:5569]
[task 2021-02-11T04:12:16.983Z] 04:12:16     INFO - GECKO(6921) | #13: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [layout/base/PresShell.cpp:4224]
[task 2021-02-11T04:12:16.984Z] 04:12:16     INFO - GECKO(6921) | #14: mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) [dom/base/Document.cpp:10330]
[task 2021-02-11T04:12:16.985Z] 04:12:16     INFO - GECKO(6921) | #15: nsFrameLoader::TryRemoteBrowserInternal() [dom/base/nsFrameLoader.cpp:2508]
[task 2021-02-11T04:12:16.985Z] 04:12:16     INFO - GECKO(6921) | #16: nsFrameLoader::TryRemoteBrowser() [dom/base/nsFrameLoader.cpp:2692]
[task 2021-02-11T04:12:16.986Z] 04:12:16     INFO - GECKO(6921) | #17: nsFrameLoader::GetBrowsingContext() [dom/base/nsFrameLoader.cpp:3453]
[task 2021-02-11T04:12:16.987Z] 04:12:16     INFO - GECKO(6921) | #18: nsFrameLoader::LoadContext() [dom/base/nsFrameLoader.cpp:3442]

This is peculiar because the actual load from the commandline handler is happening with a system principal as triggering principal, but the frameloader's mRemoteType is webIsolated + a null principal, which breaks on this assert. You can reproduce this before my patch by just running your local build with ./mach run --enable-fission --remote &, setting your mailto handler to "always ask" in about:preferences, then (with the browser still open) running ./mach run --remote 'mailto:[e-mail]'.

It seems we end up with the bogus remote type because we pass the bogus URI to https://searchfox.org/mozilla-central/rev/3ff133d19f87da2ba01ade55d6e7fdf0fc28b08c/toolkit/modules/E10SUtils.jsm#237 which returns a null principal. Nika, can you help figure out what to do? Am I OK to land this patch and disable the test in fission + debug, and file a follow-up for the failing assert and how to resolve that conundrum?

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1678255 - avoid crashing if trying to initialize a fission frameloader with a nullprincipal, r=ckerschb!,nika!

Depends on D103967

Comment 8

[:Gijs (he/him)]

Nika answered on matrix and I put up a second patch to try to address the issue.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72cde81e9b21
prompt for external protocol links whose loads were also triggered externally, instead of looping forever, r=pbz,nika
https://hg.mozilla.org/integration/autoland/rev/a77ce089e179
avoid crashing if trying to initialize a fission frameloader with a nullprincipal, r=ckerschb,nika

Comment 10

[:Gijs (he/him)]

Attachment: Bug 1678255 - fix eslint warning for HandlerServiceTestUtils import

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a143c4e876f
fix eslint warning for HandlerServiceTestUtils import. a=lint-fix

Comment 12

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/72cde81e9b21
https://hg.mozilla.org/mozilla-central/rev/a77ce089e179
https://hg.mozilla.org/mozilla-central/rev/4a143c4e876f

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fc2e9e8fc3b8
Port bug 1678255 - Add new argument to nsIExternalProtocolService.LoadURI calls. rs=bustage-fix