1670557 - Crash in [@ RtlAcquireSRWLockExclusive | mozilla::ipc::MessageChannel::NotifyImpendingShutdown]

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/9d0283e7-464b-42ff-8ffd-358600201011

Top 10 frames of crashing thread:

0 ntdll.dll RtlAcquireSRWLockExclusive 
1 xul.dll mozilla::ipc::MessageChannel::NotifyImpendingShutdown ipc/glue/MessageChannel.cpp:2656
2 xul.dll mozilla::dom::ContentParent::MaybeBeginShutDown dom/ipc/ContentParent.cpp:1573
3 xul.dll mozilla::dom::CanonicalBrowsingContext::PendingRemotenessChange::Clear docshell/base/CanonicalBrowsingContext.cpp:1150
4 xul.dll mozilla::dom::CanonicalBrowsingContext::ChangeRemoteness docshell/base/CanonicalBrowsingContext.cpp:1221
5 xul.dll mozilla::net::DocumentLoadListener::TriggerProcessSwitch netwerk/ipc/DocumentLoadListener.cpp:1718
6 xul.dll mozilla::net::DocumentLoadListener::OnStartRequest netwerk/ipc/DocumentLoadListener.cpp:2090
7 xul.dll mozilla::net::ParentChannelListener::OnStartRequest netwerk/protocol/http/ParentChannelListener.cpp:90
8 xul.dll nsDocumentOpenInfo::OnStartRequest uriloader/base/nsURILoader.cpp:166
9 xul.dll mozilla::net::ParentProcessDocumentOpenInfo::OnStartRequest netwerk/ipc/DocumentLoadListener.cpp:320

These crashes are writes to a NULL pointer, presumably the underlying lock. They seem to have started with buildid 20200911093056 and from what I can tell are only happening on nightly.

Comment 1

[Andrew McCreight [:mccr8]]

This is the set of changes in that build: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e995e42082ee5432935c3db1d5e1bc502d8c20ec&tochange=3618085622203ef418c62c3d99199485c99e0e51

Maybe bug 1661364 is related?

Comment 2

[Andrew McCreight [:mccr8]]

Only one of these crashes has Fission enabled, FWIW.

Comment 3

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

The only time when mMonitor can be nullptr is before the IPC connection to the remote process has been opened, when it is still in the launching phase (i.e. https://searchfox.org/mozilla-central/rev/d25eb00ab4e90cc0130cd18f303a04cc2a2f8409/dom/ipc/ContentParent.cpp#2428 hasn't been called yet). I think the call to NotifyImpendingShutdown should probably be guarded by a check of !IsLaunching() to make sure we're not in that phase of content process startup.

Comment 5

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

Attachment: Bug 1670557 - Check for open channel in NotifyImpendingShutdown,

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb084006d41f
Check for open channel in NotifyImpendingShutdown, r=annyG

Comment 7

[Dorel Luca [:dluca]]

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

Comment 8

[Pascal Chevrel:pascalc]

Nika, can we uplift this patch to beta? Thanks

Comment 9

[Gabriele Svelto [:gsvelto]]

Added a new signature that was briefly visible on macOS just before the fix landed.

Comment 10

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

Comment on attachment 9183471 [details]
Bug 1670557 - Check for open channel in NotifyImpendingShutdown,

Beta/Release Uplift Approval Request

• User impact if declined: Crashes
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• 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): Simple patch which should avoid the underlying issue without much risk.
• String changes made/needed: None

Comment 11

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

It looks like bug 1673711 is another failure with a new signature which used to be hidden by this assertion failure. Given that there are still issues after this is fixed, it may not be worthwhile to uplift this to beta right now.

Comment 12

[Pascal Chevrel:pascalc]

Given the volume on beta, I think we need to uplift at least a mitigation to reduce the volume before we ship 83.

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9183471 [details]
Bug 1670557 - Check for open channel in NotifyImpendingShutdown,

We are taking part 1 of bug 1673711 to mitigate the issue instead.

Comment 14

[[:philipp]]

the uplift taken for this topcrasher in b9 doesn't seem to have had the intended success unfortunately.

Comment 15

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

Re-opening as this clearly hasn't been fixed yet based on comment 14.

Comment 16

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

I've done more analysis and I think this will be fixed by also uplifting part 2 of bug 1673711. I've made an uplift request in bug 1673711 comment 17 with more details.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

No crashes with the RC build so far. Calling 83 fixed based on comment 16.