1663315 - Crash in [@ nsJARChannel::Open] during shutdown

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[[:philipp]]

[Tracking Requested - why for this release]:

Crash report: https://crash-stats.mozilla.org/report/index/bb0b6571-e8b6-4018-99a4-0e1e50200906

Top 10 frames of crashing thread:

0 xul.dll nsJARChannel::Open modules/libjar/nsJARChannel.cpp:844
1 xul.dll NS_MaybeOpenChannelUsingOpen netwerk/base/nsNetUtil.cpp:2543
2 xul.dll ReadScript js/xpconnect/loader/mozJSComponentLoader.cpp:677
3 xul.dll mozJSComponentLoader::ObjectForLocation js/xpconnect/loader/mozJSComponentLoader.cpp:792
4 xul.dll mozJSComponentLoader::Import js/xpconnect/loader/mozJSComponentLoader.cpp:1263
5 xul.dll mozilla::dom::module_getter::ModuleGetter dom/base/ChromeUtils.cpp:576
6 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:577
7 xul.dll js::CallGetter js/src/vm/Interpreter.cpp:781
8 xul.dll js::NativeGetExistingProperty js/src/vm/NativeObject.cpp
9 xul.dll Interpret js/src/vm/Interpreter.cpp:3471

this seems to be a one-time crash during a firefox update. the signature started to spike up from the 80.0 -> 80.0.1 version update.

Comment 1

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

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Andrew McCreight [:mccr8]]

nsJARChannel is indeed usually associated with networking, but it looks like we might be passing in a null pointer to NS_MaybeOpenChannelUsingOpen, so this looks like more of an XPConnect issue.

Comment 3

[Andrew McCreight [:mccr8]]

These all have shutdown progress equal to xpcom-shutdown, so this is yet another crash where we're running JS unwholesomely late.

Comment 4

[Andrew McCreight [:mccr8]]

Looking at this some more, the proximate cause of this crash is that gJarHandler in nsJARChannel is null, because it is ClearOnShutdown and we're late in shutdown. I could add a null check, but I think it would be better to detect this issue and bail out earlier in the component loader, or we may be chasing lots of weird null deref crashes like this.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1663315 - Don't load new JSMs during shutdown.

Under unknown circumstances, we can end up running chrome
JS during thread manager shutdown. Sometimes this ends up
trying to load new JSMs, but gJarHandler has already been
cleared, leading to a crash.

To avoid this and other issues, this patch forbids the
importing of new JSMs after we're late enough in shutdown
to have cleared the ClearOnShutdown pointers. I allow the
importing of JSMs that have already been loaded, as that
seems like it should be okay.

Comment 6

[Andrew McCreight [:mccr8]]

I'm not sure how to really test this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86ef5a4111d8
Don't load new JSMs during shutdown. r=kmag

Comment 8

[Jens Stutte [:jstutte]]

Hi Andrew, how severe do you rate this? Should we consider this for uplift?

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/86ef5a4111d8

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 9174441 [details]
Bug 1663315 - Don't load new JSMs during shutdown.

Beta/Release Uplift Approval Request

• User impact if declined: This is yet another crash we're seeing due to chrome JS running oddly late in shutdown. I'm not sure how much user impact there really is from a crash this late, but not shutting down cleanly could cause some issues.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We still don't know why JS is running so late in shutdown, or why these only show up on beta. That's concerning, but this patch should be low risk because it should only change behavior in a case where we're definitely going to crash anyways.
• String changes made/needed: none

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9174441 [details]
Bug 1663315 - Don't load new JSMs during shutdown.

Helps with a pretty frequent crash. Approved for 81.0b9.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6096c156d0f2