Closed Bug 1581548 Opened 3 months ago Closed 3 months ago

Crash in [@ nsWindowWatcher::OpenWindowInternal]

Categories

(Core :: DOM: Content Processes, defect, P1, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla71
Fission Milestone M4
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 + fixed
firefox71 --- fixed

People

(Reporter: philipp, Assigned: kmag)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug is for crash report bp-d7b91b49-8901-455b-84f8-c658e0190916.

Top 10 frames of crashing thread:

0 xul.dll nsWindowWatcher::OpenWindowInternal toolkit/components/windowwatcher/nsWindowWatcher.cpp:1042
1 xul.dll nsWindowWatcher::OpenWindow2 toolkit/components/windowwatcher/nsWindowWatcher.cpp:377
2 xul.dll nsGlobalWindowOuter::OpenInternal dom/base/nsGlobalWindowOuter.cpp:7243
3 xul.dll nsGlobalWindowOuter::OpenOuter dom/base/nsGlobalWindowOuter.cpp:5698
4 xul.dll nsGlobalWindowInner::Open dom/base/nsGlobalWindowInner.cpp:3750
5 xul.dll static bool mozilla::dom::Window_Binding::open dom/bindings/WindowBinding.cpp:2868
6 xul.dll mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3163
7 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:539
8 xul.dll Interpret js/src/vm/Interpreter.cpp:3084
9 xul.dll js::RunScript js/src/vm/Interpreter.cpp:424

this crash signature started showing up in 71.0a1 build 20190910095613 and then in 70.0b6 in higher volume - timing-wise this would point to the changes in bug 1579437.

on the recent beta build the signature is currently accounting for 2.9% of all tab crashes.

Flags: needinfo?(nika)

:kmag, it looks like the crash that I fixed in bug 1579437 allowed crashy cases to make it further down within the discarded window.open codepath.

I'm vaguely guessing that the BrowsingContext has a nullptr nsDocShell on it (probably due to being destroyed during the nested event loop), and the assertions are coming from the check of the ItemType() field from it, but I don't know for sure. The crash addresses are all either 0x268 (on x86), or 0x404 (on x86-64), so it at least seems consistent.

I'm guessing the easiest fix here would probably be to add a || newBC->IsDiscarded() here, and probably also make the asserts below it into proper checks which return with an error on release here.

Flags: needinfo?(nika) → needinfo?(kmaglione+bmo)
Fission Milestone: --- → M4
Priority: -- → P2
Assignee: nobody → kmaglione+bmo
Status: NEW → ASSIGNED
Priority: P2 → P1
Flags: needinfo?(kmaglione+bmo)
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dc7ef690b396
Handle new BrowsingContexts becoming discarded during nested event loop. r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb32dac62cccbdcc9206894efb7230371c01ab81
Bug 1581548: Handle new BrowsingContexts becoming discarded during nested event loop. r=bzbarsky
Flags: needinfo?(kmaglione+bmo)
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Comment on attachment 9093169 [details]
Bug 1581548: Handle new BrowsingContexts becoming discarded during nested event loop. r=nika

Beta/Release Uplift Approval Request

  • User impact if declined: Fixes a crash in beta.
  • Is this code covered by automated tests?: Yes
  • 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): The non-test portion of this patch is a trivial change which amounts to a null check.
  • String changes made/needed: None.
Attachment #9093169 - Flags: approval-mozilla-beta?

Comment on attachment 9093169 [details]
Bug 1581548: Handle new BrowsingContexts becoming discarded during nested event loop. r=nika

Crash fix, approved for 70.0b8.

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