1614259 - Crash in [@ mozilla::dom::Location::CallerSubsumes]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Julien Cristau [:jcristau]]

Crashes in nightly on all platforms starting with buildid 20200207035551, on MOZ_DIAGNOSTIC_ASSERT(outer).

This bug is for crash report bp-c5da9903-1745-43d8-b0fd-02c6c0200210.

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::Location::CallerSubsumes dom/base/Location.cpp:622
1 xul.dll mozilla::dom::Location::GetProtocol dom/base/Location.cpp:414
2 xul.dll mozilla::dom::Location_Binding::get_protocol dom/bindings/LocationBinding.cpp:236
3 xul.dll mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::MaybeCrossOriginObjectThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3052
4 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:562
5 xul.dll js::CallGetter js/src/vm/Interpreter.cpp:766
6 xul.dll js::NativeGetProperty js/src/vm/NativeObject.cpp:2631
7 xul.dll JS_ForwardGetPropertyTo js/src/jsapi.cpp:2501
8 xul.dll mozilla::dom::Location_Binding::DOMProxyHandler::get const dom/bindings/LocationBinding.cpp:1556
9 xul.dll static js::Proxy::get js/src/proxy/Proxy.cpp:341

Comment 1

[Calixte Denizet (:calixte)]

The crash started in build 20200207035551 with pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b372743705c9&tochange=7dcafc398876

So it could be related to patches for bug 1582832.
:kmag, could you investigate please ?

Comment 2

[Julien Cristau [:jcristau]]

Neha/Chris, we should get this fixed or backed out soon.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1614259 - Ensure BrowisngContexts are detached when nsDocShell is destroyed,

After bug 1582832, DocShell destruction and BrowsingContext detaching happen
in separate operations, leaving a gap where a DocShell has been destroyed, but
its BrowsingContext is still considered attached. During this gap, the usual
invariant that an in-process, attached BrowsingContext always has an
associated DOM window doesn't hold, nor do the usual invariants for outer
window forwarding security checks.

This patch fixes the detach timing so that a child BrowsingContext for a frame
which has been removed is always marked detached at the same time its DocShell
is destroyed.

Comment 4

[[:philipp]]

since it's a MOZ_DIAGNOSTIC_ASSERT it shouldn't affect beta and release users.

Comment 5

[Andrew Overholt [:overholt]]

Thanks Philipp! Also note that the changes that caused this in bug 1582832 have been backed out from beta.

Comment 6

[Julien Cristau [:jcristau]]

Kris, any progress here?

Comment 7

[Julien Cristau [:jcristau]]

Do we need to back bug 1582832 out of beta (again) and trunk at this point?

Comment 8

[Andrew Overholt [:overholt]]

(In reply to Julien Cristau [:jcristau] from comment #7)

Do we need to back bug 1582832 out of beta (again) and trunk at this point?

I'm not sure but I've started an email thread with kmag and Nika and will keep you posted (or they will) here.

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to Andrew Overholt [:overholt] from comment #8)

(In reply to Julien Cristau [:jcristau] from comment #7)

Do we need to back bug 1582832 out of beta (again) and trunk at this point?

I'm not sure but I've started an email thread with kmag and Nika and will keep you posted (or they will) here.

kmag says he will have an unliftable fix soon. Also, the MOZ_DIAGNOSTIC_ASSERT crash should not affect Beta users (because MOZ_DIAGNOSTIC_ASSERTs are enabled in Nightly and DevEdition, but not Beta).

Comment 11

[Julien Cristau [:jcristau]]

(In reply to Chris Peterson [:cpeterson] from comment #9)

kmag says he will have an unliftable fix soon. Also, the MOZ_DIAGNOSTIC_ASSERT crash should not affect Beta users (because MOZ_DIAGNOSTIC_ASSERTs are enabled in Nightly and DevEdition, but not Beta).

Can we at least change the assert to debug-only? Only crashing nightly and devedition is no excuse to leave a top crasher unaddressed for several weeks IMO.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c49115748677
Ensure BrowisngContexts are detached when nsDocShell is destroyed, r=farre

Comment 13

[Chris Peterson [:cpeterson]]

(In reply to Pulsebot from comment #12)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c49115748677
Ensure BrowisngContexts are detached when nsDocShell is destroyed, r=farre

Nika, should we uplift your nsDocShell fix to 75 Beta? MOZ_DIAGNOSTIC_ASSERTs don't abort in Beta builds, but they do abort in DevEdition builds.

Comment 14

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

Comment on attachment 9126505 [details]
Bug 1614259 - Ensure BrowisngContexts are detached when nsDocShell is destroyed,

Beta/Release Uplift Approval Request

• User impact if declined: DevEdition-only top-crasher
• Is this code covered by automated tests?: No
• 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: Medium
• Why is the change risky/not risky? (and alternatives if risky): This change impacts important parts of the BrowsingContext lifecycle, and could potentially cause regressions due to these changes. This patch has not been thoroughly tested on Nightly.

An alternative to performing the uplift would be to relax the MOZ_DIAGNOSTIC_ASSERT on the beta branch to a MOZ_ASSERT, which would reduce the crash rate without fixing the underlying issue.

• String changes made/needed:

Comment 15

[Stefan Hindli [:stefan_hindli]]

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

Comment 16

[Julien Cristau [:jcristau]]

What's the effect of this bug on a release or beta build, so without the crash? From the alternative you suggest (of making it a MOZ_ASSERT), I'm guessing it's not a huge deal?

Comment 17

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

The particular crash is one which would be safe to relax the assertion on, but there may be other, smaller, bugs which are caused by the same underlying issue which could crop-up.

Comment 18

[Julien Cristau [:jcristau]]

Thanks. I'd opt to relax the assert then, so DevEdition users don't keep crashing for 2 more weeks until 76.0b1.

Comment 19

[Julien Cristau [:jcristau]]

Attachment: Bug 1614259 - make a diagnostic assert debug-only to stop crashes on DevEdition. r?nika

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 9126505 [details]
Bug 1614259 - Ensure BrowisngContexts are detached when nsDocShell is destroyed,

let's go with the other patch for 75.

Comment 21

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/78214bf19fb8

Comment 23

[Firefox Bug Husbandry Bot]

Please specify a root cause for this bug. See :tmaity for more information.