Closed Bug 1678255 Opened 4 years ago Closed 3 years ago

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


(Firefox :: File Handling, defect, P2)




88 Branch
Tracking Status
firefox85 --- wontfix
firefox88 --- fixed


(Reporter: Gijs, Assigned: Gijs)


(Regressed 1 open bug)



(3 files)


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

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

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.

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(gijskruitbosch+bugs)
Severity: -- → S2
Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P2
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1682285
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → gijskruitbosch+bugs

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

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.

Attachment #9201022 - Attachment description: Bug 1678255 - track whether links have been opened externally and prompt instead of looping forever, r?pbz!,nika! → Bug 1678255 - prompt for external protocol links whose loads were also triggered externally, instead of looping forever, r?pbz!,nika!
Pushed by
prompt for external protocol links whose loads were also triggered externally, instead of looping forever, r=pbz,nika

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 ''.

It seems we end up with the bogus remote type because we pass the bogus URI to 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?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(nika)
Attachment #9202667 - Attachment description: Bug 1678255 - avoid crashing if trying to initialize a fission frameloader with a nullprincipal, ckerschb!,nika! → Bug 1678255 - avoid crashing if trying to initialize a fission frameloader with a nullprincipal, r=ckerschb!,nika!

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

Flags: needinfo?(nika)
Pushed by
prompt for external protocol links whose loads were also triggered externally, instead of looping forever, r=pbz,nika
avoid crashing if trying to initialize a fission frameloader with a nullprincipal, r=ckerschb,nika
Pushed by
fix eslint warning for HandlerServiceTestUtils import. a=lint-fix
Pushed by
Port bug 1678255 - Add new argument to nsIExternalProtocolService.LoadURI calls. rs=bustage-fix
Blocks: 1667468
Regressed by: 1700976
No longer regressed by: 1700976
Regressions: 1700976
Regressions: 1717314
Regressions: 1732132
You need to log in before you can comment on or make changes to this bug.