Closed Bug 1678255 Opened 1 year ago Closed 9 months ago

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

Categories

(Firefox :: File Handling, defect, P2)

Desktop
All
defect

Tracking

()

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

People

(Reporter: Gijs, Assigned: Gijs)

References

(Regressed 1 open bug)

Details

Attachments

(3 files)

STR:

  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

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.

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)
Duplicate of this bug: 1678249
Severity: -- → S2
Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P2
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1682285
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

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.

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 gijskruitbosch@gmail.com:
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

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:a@example.com'.

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?

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 gijskruitbosch@gmail.com:
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
Pushed by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a143c4e876f
fix eslint warning for HandlerServiceTestUtils import. a=lint-fix
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/fc2e9e8fc3b8
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
See Also: → 1707302
Regressions: 1717314
Regressions: 1732132
You need to log in before you can comment on or make changes to this bug.