1712275 - Crash in [@ nsExternalAppHandler::GetBrowsingContextId]

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

6 Nightly crashes so far, oldest with 90.0a1 20210519083222. The crash reports have local files as current page, is bug 1555637 the regressing change?

Crash report: https://crash-stats.mozilla.org/report/index/46689c91-8de1-473b-8c37-f26750210520

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll nsExternalAppHandler::GetBrowsingContextId 
1 xul.dll static XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1143
2 xul.dll XPC_WN_GetterSetter js/xpconnect/src/XPCWrappedNativeJSOps.cpp:965
3 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:512
4 xul.dll js::NativeGetProperty js/src/vm/NativeObject.cpp:2292
5 xul.dll Interpret js/src/vm/Interpreter.cpp:2929
6 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:544
7 xul.dll JS::Call js/src/jsapi.cpp:2796
8 xul.dll mozilla::dom::EventHandlerNonNull::Call dom/bindings/EventHandlerBinding.cpp:279
9 xul.dll mozilla::JSEventHandler::HandleEvent dom/events/JSEventHandler.cpp:201

Comment 1

[:Gijs (he/him)]

Yeah, we'd hit this if the mLauncher object (ie the this in https://searchfox.org/mozilla-central/rev/6e630edb09c3ab06d0103665b16c9ea7dce782c5/uriloader/exthandler/nsExternalHelperAppService.cpp#1880 ) has a nullptr mBrowsingContext, when the dialog code attempts to get the browsing context ID, because that getter doesn't nullcheck mBrowsingContext

What I'm less sure about is how users end up in this situation, or what the right solution is. I think the lack of browsing context means either the return value of MaybeCloseWindow overwrote it with null, which I think is not possible, or either the construction of the handler/launcher object at nsExternalHelperAppService::DoContentContentProcessHelper or nsExternalHelperAppService::CreateListener gets no browsing context. I don't know when/how that'd be the case.

Nika, do you know, and relatedly, is there a convention for "there's no browsing context" responses to ID requests? Should it return 0?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Gijs (he/him) from comment #1)

What I'm less sure about is how users end up in this situation, or what the right solution is. I think the lack of browsing context means either the return value of MaybeCloseWindow overwrote it with null, which I think is not possible, or either the construction of the handler/launcher object at nsExternalHelperAppService::DoContentContentProcessHelper or nsExternalHelperAppService::CreateListener gets no browsing context. I don't know when/how that'd be the case.

It seems quite possible to end up with a null mBrowsingContext in nsExternalHelperApp in various ways, such as if a load ends up going through PExternalHelperApp but your BC is discarded while the constructor message is in flight. There may also be other more-reliable ways to hit it :-). There's actually an old patch from :mattwoodrow which had to add some of these if (mBrowsingContext) checks to handle the situation last year because we were hitting crashes in the wild (bug 1611588).

You can actually see that we seem to hit this in infrastructure by looking at the ccov report, as the lines around the if (mBrowsingContext) line in nsExternalAppHandler::OnStartRequest have 433 hits, whereas the body of the if statement has only 363 ccov hits. Probably wouldn't be too hard to do a mochitest-browser-chrome test run with an assert there to find the specific codepaths if you wanted. (https://coverage.moz.tools/#revision=latest&path=uriloader/exthandler/nsExternalHelperAppService.cpp&view=file&line=1706 for now, though this link will probably break quickly)

Nika, do you know, and relatedly, is there a convention for "there's no browsing context" responses to ID requests? Should it return 0?

0 is never a valid BrowsingContext ID, so it's a good thing to return if there is no BrowsingContext. I think the easiest fix will be returning 0 here, which will also make the call which immediately tries to re-get the BC return nullptr as there is no BC with ID 0.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1712275 - avoid crashing on browsingcontext-less downloads for which we ask the user what to do with the file, r?nika

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5eab654b0eb
avoid crashing on browsingcontext-less downloads for which we ask the user what to do with the file, r=nika

Comment 5

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/a5eab654b0eb