1718333 - Crash in [@ mozilla::ipc::NodeController::DeserializeEventMessage]

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Andrew McCreight [:mccr8]]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/d3a84151-8a42-40d4-8036-d884e0210625

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(mWorkerThread && !mWorkerThread->IsOnCurrentThread()) (on worker thread but should not be!)

Top 10 frames of crashing thread:

0 XUL mozilla::ipc::NodeController::DeserializeEventMessage ipc/glue/NodeController.cpp:217
1 XUL mozilla::ipc::ExportSharedJSInit ipc/glue/ProcessUtils_common.cpp:263
2 XUL mojo::core::ports::Node::RemoveFromPeerPortMap ipc/chromium/src/mojo/core/ports/node.cc:1665
3 XUL mozilla::ipc::ProcessChild::ProcessChild ipc/glue/ProcessChild.cpp:28
4 XUL mojo::core::ports::Node::AcceptEvent ipc/chromium/src/mojo/core/ports/node.cc:413
5 XUL mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2012
6 XUL mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2031
7 XUL mojo::core::ports::Node::InitializePort ipc/chromium/src/mojo/core/ports/node.cc:153
8 XUL mozilla::dom::PContentParent::SendUpdateMediaCodecsSupported ipc/ipdl/PContentParent.cpp:5138
9 XUL {virtual override thunk} 

I hit this crash in my own session. I was loading some XenForo forum page, but visiting the page again did not cause the crash again.

Seems like it could be related to Nika's Mojo IPC work.

Comment 1

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

Looking at other crashes with the same MOZ_RELEASE_ASSERT signature I found https://crash-stats.mozilla.org/report/index/48b15128-cf46-43d9-a75c-8d7fb0210625. This might have the same general cause.

Based on that crash, this looks like it might be caused trying to send an Endpoint over IPC to a process which is gone, which leads to the endpoint being closed immediately, and potentially an IPC channel error occuring on the worker thread. We might need to either make MessageChannel handle getting notifications from the worker thread reliably, or dispatch notifications to the IO thread if they occur on the worker thread. I think it should be possible to make MessageChannel handle notifications on the worker thread, so that's probably my preferred approach, though it's probably more work than the IO thread dispatch.

Leaving a ni? for me to get back to this.

Comment 2

[Chris Peterson [:cpeterson]]

This doesn't look like a Fission crash even though the crash report in comment 0 has DOMFissionEnabled=1. Adding [not-a-fission-bug] whiteboard tag to hide this bug from Fission bug triage queries.

Comment 5

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

Attachment: Bug 1718333 - Remove unnecessary AssertLinkThread assertions, r=handyman

After looking through the methods which have this assertion, I couldn't
find any examples of places where not having a specific "link thread"
sequence would cause any issues. I think these assertions can and should
be removed.

The main change required by this was to remove the !NS_IsMainThread()
assertion from the SchedulerGroup listener. Due to how callbacks work,
it would be possible for a vsync message to be detected by a
MessageChannel from the main thread if it was sent before the channel
was bound. I don't believe that this change should cause any issues.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b1c17e12271
Remove unnecessary AssertLinkThread assertions, r=handyman

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/8b1c17e12271

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:nika, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 11

[Pascal Chevrel:pascalc (PTO until April 26)]

With 40 crashes per day on beta, I think we should evaluate if the fix we have in nightly would be safe to uplift.

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Nika, is that a patch we should uplift?

Comment 13

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

Comment on attachment 9230190 [details]
Bug 1718333 - Remove unnecessary AssertLinkThread assertions, r=handyman

Beta/Release Uplift Approval Request

• User impact if declined: Unnecessary crashes due to overzealous assertions
• 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): Patch removes release-mode assertions which were determined to be unnecessary due to code analysis.
• String changes made/needed: None

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9230190 [details]
Bug 1718333 - Remove unnecessary AssertLinkThread assertions, r=handyman

Approved for 91 beta 8, thanks.

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5c703e3753b4