1675820 - Crash in [@ IPCError-browser | RecvCreateBrowsingContext Parent has different group object]

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/c630dda1-b3ad-4f36-926d-9cd370201106

Reason: EXCEPTION_BREAKPOINT

Top 9 frames of crashing thread:

0 ntdll.dll ntdll.dll@0x9c7a4 
1 kernelbase.dll VirtualProtect 
2 xul.dll js::jit::Linker::newCode js/src/jit/Linker.cpp:70
3 xul.dll js::jit::BaselineCompiler::compile js/src/jit/BaselineCodeGen.cpp:253
4 xul.dll js::jit::BaselineCompile js/src/jit/BaselineJIT.cpp:250
5 xul.dll js::jit::BaselineCompileFromBaselineInterpreter js/src/jit/BaselineJIT.cpp:446
6  @0x1caa60624c3 
7 xul.dll js::jit::DoNewArrayFallback js/src/jit/BaselineIC.cpp:3922
8  @0x933b1faa57 

Comment 1

[Andrew McCreight [:mccr8]]

Bug 1656854 fixed the big spike of this crash in August, but it is still happening at a lower level.

Comment 2

[Chris Peterson [:cpeterson]]

M7 because the crash volume is not that high.

A subframe is being created after its parent frame or group has already been discarded.

Comment 3

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

This seems to still be happening. Some notes from my quick scan during triage: Maybe being caused by a message sent over PContent even though the PBrowser is already dead which the BrowsingContext is embedded in? Should we consider having the message be sent over PBrowser instead?

Comment 4

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

Attachment: Bug 1675820 - Part 1: Slightly simplify Document::GetParentFeaturePolicy,

The previous implementation which used WindowContext was unncessarially
complicated, and can be simplified.

Comment 5

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

Attachment: Bug 1675820 - Part 1: Upgrade destroyed asserts in BrowsingContextGroup,

The issues with BrowsingContextGroup identity may be related to using a
destroyed BrowsingContextGroup in some situations when we shouldn't be. By
upgrading the intensity of these assertions, we should be able to catch the
issues more readily.

Comment 6

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

Attachment: Bug 1675820 - Part 2: Trace inner window mBrowsingContext and mWindowGlobalChild fields,

When these fields were added, it appears I completely forgot to add them to the
cycle collection unlink logic. This somehow didn't cause leaks due to existing
code which would break the cycles between the inner window and the outer window
/ BrowsingContext.

Comment 7

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

Attachment: Bug 1675820 - Part 3: Ensure WindowContext is discarded when WindowGlobalChild is destroyed,

Previously it appears that WindowContext::Discard was never invoked in the
content process which hosts the window in question, which may have lead to the
crashes mentioned in this issue, among other problems.

Comment 8

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

Attachment: Bug 1675820 - Part 4: Track WindowGlobalChild in WindowContext,

This allows for the WindowGlobalChild getter in WindowContext to be acquired
more efficiently without performing hashtable lookups, and should generally
simplify things.

The patch also removes the unnecessary XRE_IsContentProcess assertions, and
removes the global hashtable for tracking WindowGlobalChild instances which is
no longer necessary.

Comment 9

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

Attachment: Bug 1675820 - Part 5: Keep BCGs alive while waiting for WindowContext discards to be acked,

This should further reduce the chance that a BrowsingContextGroup is mentioned
in a message which has already ben destroyed by a remote process.

Comment 10

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

Attachment: Bug 1675820 - Part 6: Make DocGroup cycle-collected,

Previously, the DocGroup type was not cycle-collected, as it needed to have
references from other threads for Quantum DOM. Nowadays the only off-main-thread
use of DocGroup is for dispatching runnables to the main thread which should be
tracked using a performance counter for about:performance. This means we can
remove the DocGroup references from these dispatching callsites, only storing
the Performance Counter we're interested in, and simplify make DocGroup be
cycle-collected itself.

This fixes a leak caused by adding the WindowGlobalChild getter to
WindowContext, by allowing cycles between the document and its BrowsingContext
to be broken by the cycle-collector.

Comment 11

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

Attachment: Bug 1675820 - Part 7: Fix marionette BrowsingContext replace detection logic,

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5fd9d265e2c
Part 1: Upgrade destroyed asserts in BrowsingContextGroup, r=kmag
https://hg.mozilla.org/integration/autoland/rev/83789537d242
Part 2: Trace inner window mBrowsingContext and mWindowGlobalChild fields, r=smaug
https://hg.mozilla.org/integration/autoland/rev/c49f76a036ac
Part 3: Ensure WindowContext is discarded when WindowGlobalChild is destroyed, r=kmag
https://hg.mozilla.org/integration/autoland/rev/1a9778d2215c
Part 4: Track WindowGlobalChild in WindowContext, r=kmag
https://hg.mozilla.org/integration/autoland/rev/6417dd804869
Part 5: Keep BCGs alive while waiting for WindowContext discards to be acked, r=kmag
https://hg.mozilla.org/integration/autoland/rev/e6d84dd114ed
Part 6: Make DocGroup cycle-collected, r=farre,smaug
https://hg.mozilla.org/integration/autoland/rev/f2ed6497335f
Part 7: Fix marionette BrowsingContext replace detection logic, r=whimboo,marionette-reviewers,farre

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d5fd9d265e2c
https://hg.mozilla.org/mozilla-central/rev/83789537d242
https://hg.mozilla.org/mozilla-central/rev/c49f76a036ac
https://hg.mozilla.org/mozilla-central/rev/1a9778d2215c
https://hg.mozilla.org/mozilla-central/rev/6417dd804869
https://hg.mozilla.org/mozilla-central/rev/e6d84dd114ed
https://hg.mozilla.org/mozilla-central/rev/f2ed6497335f