1579437 - Crash in [@ mozilla::ipc::IPDLParamTraits<T>::Write]

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P2)

Description

[[:philipp]]

This bug is for crash report bp-cce618a4-2c15-4f51-bd35-d53eb0190719.

Top 10 frames of crashing thread:

0 xul.dll static void mozilla::ipc::IPDLParamTraits<mozilla::dom::BrowsingContext*>::Write docshell/base/BrowsingContext.cpp:970
1 xul.dll mozilla::dom::PContentChild::SendWindowPostMessage ipc/ipdl/PContentChild.cpp:6621
2 xul.dll mozilla::dom::ContentParent::RecvWindowPostMessage dom/ipc/ContentParent.cpp:6018
3 xul.dll mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:10924
4 xul.dll mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2092
5 xul.dll mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:1970
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1225
7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
8 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:88
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308

this crash signature is regressing in volume in the 70.0b cycle - currently it's accounting for 2% of tab crashes.

crashes occur cross-platform and come with MOZ_RELEASE_ASSERT(!aParam->IsDiscarded()) (Cannot send discarded BrowsingContext between processes!) - bug 1566806 doesn't look like it has fully fixed this problem yet.

Comment 1

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

The crash report you have linked on this bug appears to have an old version of our code. Specifically:
Version from linked crash: https://hg.mozilla.org/mozilla-central/annotate/29e9dde37bd231a94959394554154ede52670c65/dom/ipc/ContentParent.cpp#l5995
Current mozilla-beta: https://searchfox.org/mozilla-beta/rev/26cf0e844e011a172635a6a776ccceec28d4f650/dom/ipc/ContentParent.cpp#6031

I think the volume is probably coming from other, similar looking crashes. Looking through some stacks, I can see a few issues:
https://crash-stats.mozilla.org/report/index/1b1583a0-69ff-4505-9a07-700b60190906 <- We're not checking !IsDiscarded() during BrowsingContext::Transaction::Commit()
https://crash-stats.mozilla.org/report/index/ca8a3ec3-b823-4a28-968d-e3efb0190906 <- We're not checking before sending NotifyMediaActiveStateChanged()

There may be other places where we don't check this correctly right now, but my quick scan through mostly saw these two.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a new top crash in beta 70.

Comment 3

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

Attachment: Bug 1579437 - Check for discarded BrowsingContext in more places,

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7333d4f4c7d1
Check for discarded BrowsingContext in more places, r=farre

Comment 5

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7333d4f4c7d1

Comment 7

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

Comment on attachment 9091165 [details]
Bug 1579437 - Check for discarded BrowsingContext in more places,

Beta/Release Uplift Approval Request

• User impact if declined: Potential crashes
• 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:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only adds new, low-risk checks 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

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9091165 [details]
Bug 1579437 - Check for discarded BrowsingContext in more places,

Fix for a top crash in beta, let's uplift for beta 6.

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/34a6de6322f5

Comment 10

[Andrew McCreight [:mccr8]]

People are still running old betas from before this landed, so I'm going to add the crash signature that they now show up as.