Crash in [@ mozilla::dom::AutoEntryScript::AutoEntryScript]
Categories
(Core :: DOM: Content Processes, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox71 | --- | unaffected |
firefox72 | --- | fixed |
firefox73 | --- | fixed |
People
(Reporter: philipp, Assigned: jdai)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
|
Details | Review |
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•1 year ago
|
||
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•1 year ago
|
||
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•1 year ago
|
||
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•1 year ago
|
||
Er... as long as the window.
Assignee | ||
Comment 5•1 year ago
|
||
Yes, I will take a look. Keep NI for tracking. :)
Assignee | ||
Comment 6•1 year ago
|
||
Assignee | ||
Comment 7•1 year ago
|
||
Updated•1 year ago
|
Pushed by jdai@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8cb11ce383f4 Purge pending queries at JSWindowActor destroy; r=mccr8
Comment 9•1 year ago
|
||
bugherder |
Reporter | ||
Comment 10•1 year ago
|
||
could you request an uplift to beta please, if you deem fit to do so?
Assignee | ||
Comment 11•1 year ago
|
||
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•1 year ago
|
||
Comment on attachment 9116106 [details]
Bug 1603598 - Purge pending queries at JSWindowActor destroy;
hopfully fix a crash regression, approved for 72.0b8
Comment 13•1 year ago
|
||
bugherderuplift |
Description
•