1563542 - Crash in [@ mozilla::dom::ContentParent::RecvAttachBrowsingContext]

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[[:philipp]]

This bug is for crash report bp-bd690590-ec87-41d2-90c0-f9a610190704.

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::ContentParent::RecvAttachBrowsingContext dom/ipc/ContentParent.cpp:5755
1 xul.dll mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:10538
2 xul.dll mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2082
3 xul.dll mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:1970
4 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1225
5 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
6 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:110
7 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308
8 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:290
9 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:137

this browser crash signature with MOZ_RELEASE_ASSERT(parent) (Parent doesn't exist in parent process) is spiking up since in nightly again since 69.0a1 build 20190703094734.
the crashes happen cross platform and account for 30% of browser crashes during the past 24h...

these were the changes going into the first affected nightly build:

• pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fd2778a2ce8b&tochange=c52f9ebca761
• buglist: https://mzl.la/2Xo8nXT

Comment 1

[Marcia Knous [:marcia]]

[Tracking Requested - why for this release]: 69.0b2 has a massive number of crashes - almost 3000. Andreas is on PTO until 7-14 - Can we find someone else to help with this? If Bug 1561899 regressed it can we back it out?

Comment 2

[Marcia Knous [:marcia]]

Nika since you reviewed the bug in Comment 1 can you please take a look? Thanks.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

This blocks 69.0b3 gtb with a crash volume that high. Let's backout bug 1561899 from Beta after today's merges are complete.

Comment 4

[Neha Kochar [:neha]]

Bug 1561899 is a likely cause of the increased crashes so yes, we should back out the patch.

Comment 5

[Neha Kochar [:neha]]

Nika is looking into this during farre's PTO.

Comment 6

[Marcia Knous [:marcia]]

I was able to reproduce the crash on my 10.15 machine by loading https://www.tech-recipes.com/rx/3104/os-x-show-hidden-files-and-folders-in-mac-os-x-finder/. If I wait long enough it seems the browser just crashes on its own - https://crash-stats.mozilla.org/report/index/d8dd2600-ff4e-4a1a-8881-ffb0b0190708.

Comment 7

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

Hey, I think I found the issue, patch incoming which should be easier to land than a back-out (due to other patches potentially building on top of mIsDetached).

In the mIsDetached bug, the code was changed to not set mClosed during Detach, and only set mIsDetached. This looks like a mistake because a bunch of places are only reading mClosed. Specifically when creating a BrowsingContext for an iframe, we check GetClosed() to see whether to skip creating.

This was changed to be no longer correct, as mClosed was not set during Detach anymore.

(In reply to Neha Kochar [:neha] from comment #4)

This crash signature is already being tracked at Bug 1560106. Bug 1561899 is a likely cause of the increased crashes so yes, we should back out the patch.

That signature is tracking Detach, while this is tracking Attach. This crash also occurs outside of fission-enabled.

Comment 8

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

Attachment: Bug 1563542 - Correctly align usage of mIsDiscarded and mClosed for BrowsingContext,

In the bug which introduced mIsDiscarded, the code was changed to not set
mClosed during Detach, and only set mIsDiscarded. This was a mistake because a
bunch of places are only reading mClosed. Specifically when creating a
BrowsingContext for an iframe, we check GetClosed() to see whether to skip
creating it. Not doing this check can lead to assertions like the one in this
bug.

This patch changes the behaviour to continue setting mClosed, and also updates
the relevant GetClosed() checks to correctly check IsDiscarded()

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c5ad42069d0
Correctly align usage of mIsDiscarded and mClosed for BrowsingContext, r=peterv

Comment 10

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

Comment on attachment 9076557 [details]
Bug 1563542 - Correctly align usage of mIsDiscarded and mClosed for BrowsingContext,

Beta/Release Uplift Approval Request

• User impact if declined: Crashes
• 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: Low
• Why is the change risky/not risky? (and alternatives if risky): Small change to restore behaviour.
• String changes made/needed: none

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9076557 [details]
Bug 1563542 - Correctly align usage of mIsDiscarded and mClosed for BrowsingContext,

Hopefully fixes a topcrash blocking 69. Approved for 69.0b3.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9cdcc170dc1a

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6c5ad42069d0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Crash is gone \m/

Comment 16

[Chris Peterson [:cpeterson]]

Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 17

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This bug has been identified as part of a pilot on determining root causes of blocking and dot release drivers.

It needs a root-cause set for it. Please see the list at https://docs.google.com/document/d/1FFEGsmoU8T0N8R9kk-MXWptOPtXXXRRIe4vQo3_HgMw/.

Add the root cause as a whiteboard tag in the form [rca - <cause> ] and remove the rca-needed keyword.

If you have questions, please contact :tmaity.

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Emma Humphries, Bugmaster ☕️🎸🧞‍♀️✨ (she/her) [:emceeaich] (UTC-8) needinfo? me from comment #17)

This bug has been identified as part of a pilot on determining root causes of blocking and dot release drivers.

It needs a root-cause set for it. Please see the list at https://docs.google.com/document/d/1FFEGsmoU8T0N8R9kk-MXWptOPtXXXRRIe4vQo3_HgMw/.

Add the root cause as a whiteboard tag in the form [rca - <cause> ] and remove the rca-needed keyword.

If you have questions, please contact :tmaity.

Hi Nika, could you please help identify the root cause of this bug, following comment 17? Thank you.