Closed Bug 1603598 Opened 1 year ago Closed 1 year ago

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

Categories

(Core :: DOM: Content Processes, defect)

72 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla73
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)

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.

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.

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.

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.

Flags: needinfo?(jdai)

Er... as long as the window.

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

Assignee: nobody → jdai
Status: NEW → ASSIGNED
Pushed by jdai@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8cb11ce383f4
Purge pending queries at JSWindowActor destroy; r=mccr8
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

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

Flags: needinfo?(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
Flags: needinfo?(jdai)
Attachment #9116106 - Flags: approval-mozilla-beta?

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

hopfully fix a crash regression, approved for 72.0b8

Attachment #9116106 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.