1603598 - Crash in [@ mozilla::dom::AutoEntryScript::AutoEntryScript]

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[[:philipp]]

This bug is for crash report bp-cd66445e-fba8-4004-a585-66f150191212.

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::AutoEntryScript::AutoEntryScript dom/script/ScriptSettings.cpp:582
1 xul.dll void mozilla::dom::Promise::MaybeSomething<nsresult&> dom/promise/Promise.h:300
2 xul.dll mozilla::dom::JSWindowActor::RejectPendingQueries dom/ipc/JSWindowActor.cpp:103
3 xul.dll void mozilla::dom::JSWindowActorChild::cycleCollection::Unlink dom/ipc/JSWindowActorChild.cpp:145
4 xul.dll bool nsCycleCollector::Collect xpcom/base/nsCycleCollector.cpp:3436
5 xul.dll nsCycleCollector_collectSlice xpcom/base/nsCycleCollector.cpp:3926
6 xul.dll static void nsJSContext::RunCycleCollectorSlice dom/base/nsJSEnvironment.cpp:1600
7 xul.dll static bool ICCRunnerFired dom/base/nsJSEnvironment.cpp:1650
8 xul.dll std::_Func_impl_no_alloc<bool  
9 xul.dll nsresult mozilla::IdleTaskRunner::Run xpcom/threads/IdleTaskRunner.cpp:58

this crash signature is spiking up considerably during the 72.0b cycle.

Comment 1

[Andrew McCreight [:mccr8]]

The unlink of JSWindowActor is calling RejectPendingQueries, which ends up doing AutoEntryScript aes(mGlobal, ...);, where mGlobal is the a field of the promise. The crash is a null deref. I suspect what is happening is that the JSWindow actor and one of its promises are getting selected as garbage in the same cycle collection, and we are unlinking the promise before we unlink the JSWindow actor, which would clear the mGlobal field.

Comment 2

[Andrew McCreight [:mccr8]]

It looks like this was added in bug 1544936, but maybe we're just hitting a critical mass of conversions to window actors that this is showing up as a stability problem now.

Comment 3

[Andrew McCreight [:mccr8]]

John, could you take a look at this? Fundamentally it isn't really okay to run code like this in the unlink method. Maybe these pending queries can get purged at some earlier point, like if the window of the window actor is going away or the actor gets unregistered? I would assume at the window actor is going to live as long as the actor.

Comment 4

[Andrew McCreight [:mccr8]]

Er... as long as the window.

Comment 5

[John Dai[:jdai]]

Yes, I will take a look. Keep NI for tracking. :)

Comment 6

[John Dai[:jdai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c4e97f00ef21bc74359c00eb5c91330fe96b7a0

Comment 7

[John Dai[:jdai]]

Attachment: Bug 1603598 - Purge pending queries at JSWindowActor destroy;

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cb11ce383f4
Purge pending queries at JSWindowActor destroy; r=mccr8

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/8cb11ce383f4

Comment 10

[[:philipp]]

could you request an uplift to beta please, if you deem fit to do so?

Comment 11

[John Dai[:jdai]]

Comment on attachment 9116106 [details]
Bug 1603598 - Purge pending queries at JSWindowActor destroy;

Beta/Release Uplift Approval Request

• User impact if declined: Potential crash.
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: N/A
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It only move pending queries out of unlink to avoid crashes in known dangerous places. No tests explicitly cover the crashing cases, however, the other cases for the code are decently tested.
• String changes made/needed: none

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 9116106 [details]
Bug 1603598 - Purge pending queries at JSWindowActor destroy;

hopfully fix a crash regression, approved for 72.0b8

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1b5e8e9241d0