Closed Bug 1718333 Opened 4 months ago Closed 3 months ago

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

Categories

(Core :: IPC, defect)

defect

Tracking

()

RESOLVED FIXED
92 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox89 --- unaffected
firefox90 --- unaffected
firefox91 --- fixed
firefox92 --- fixed

People

(Reporter: mccr8, Assigned: nika)

References

Details

(Keywords: crash, regression, Whiteboard: [not-a-fission-bug])

Crash Data

Attachments

(1 file)

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.

Severity: -- → S2

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.

Flags: needinfo?(nika)

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.

Duplicate of this bug: 1718783
Duplicate of this bug: 1719496
Crash Signature: [@ mozilla::ipc::NodeController::DeserializeEventMessage] → [@ mozilla::ipc::NodeController::DeserializeEventMessage] [@ mozilla::ipc::MessageChannel::OnChannelErrorFromLink]
Crash Signature: [@ mozilla::ipc::NodeController::DeserializeEventMessage] [@ mozilla::ipc::MessageChannel::OnChannelErrorFromLink] → [@ mozilla::ipc::NodeController::DeserializeEventMessage] [@ mozilla::ipc::MessageChannel::OnChannelErrorFromLink]

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.

Assignee: nobody → nika
Status: NEW → ASSIGNED
Crash Signature: [@ mozilla::ipc::NodeController::DeserializeEventMessage] [@ mozilla::ipc::MessageChannel::OnChannelErrorFromLink] → [@ mozilla::ipc::NodeController::DeserializeEventMessage] [@ mozilla::ipc::MessageChannel::OnChannelErrorFromLink] [@ mozilla::ipc::MessageChannel::DispatchMessage ] [@ mozilla::ipc::MessageChannel::MaybeHandleError ]
Flags: needinfo?(nika)
Duplicate of this bug: 1720096
Crash Signature: [@ mozilla::ipc::NodeController::DeserializeEventMessage] [@ mozilla::ipc::MessageChannel::OnChannelErrorFromLink] [@ mozilla::ipc::MessageChannel::DispatchMessage ] [@ mozilla::ipc::MessageChannel::MaybeHandleError ] → [@ mozilla::ipc::NodeController::DeserializeEventMessage] [@ mozilla::ipc::MessageChannel::OnChannelErrorFromLink] [@ mozilla::ipc::MessageChannel::DispatchMessage ] [@ mozilla::ipc::MessageChannel::MaybeHandleError ] [@ XUL@0x91d753 | mozilla::ipc::Node…
Duplicate of this bug: 1720714
Crash Signature: mozilla::ipc::NodeController::OnEventMessage] [@ mozilla::ipc::MessagePump::MessagePump] → mozilla::ipc::NodeController::OnEventMessage] [@ mozilla::ipc::MessagePump::MessagePump] [@ mozilla::ipc::PortLink::SendMessage]
Attachment #9230190 - Attachment description: Bug 1718333 - Remove unnecessary AssertLinkThread assertions, → Bug 1718333 - Remove unnecessary AssertLinkThread assertions, r=handyman
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b1c17e12271
Remove unnecessary AssertLinkThread assertions, r=handyman
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch
Crash Signature: [@ mozilla::ipc::NodeController::DeserializeEventMessage] [@ mozilla::ipc::MessageChannel::OnChannelErrorFromLink] [@ mozilla::ipc::MessageChannel::DispatchMessage ] [@ mozilla::ipc::MessageChannel::MaybeHandleError ] [@ XUL@0x91d753 | mozilla::ipc::Node… → [@ mozilla::ipc::NodeController::DeserializeEventMessage] [@ mozilla::ipc::MessageChannel::OnChannelErrorFromLink] [@ mozilla::ipc::MessageChannel::DispatchMessage ] [@ mozilla::ipc::MessageChannel::MaybeHandleError ] [@ XUL@0x91d753 | mozilla::ipc::Nod…

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.

Flags: needinfo?(nika)

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

Nika, is that a patch we should uplift?

Crash Signature: [@ mozilla::ipc::NodeController::DeserializeEventMessage] [@ mozilla::ipc::MessageChannel::OnChannelErrorFromLink] [@ mozilla::ipc::MessageChannel::DispatchMessage ] [@ mozilla::ipc::MessageChannel::MaybeHandleError ] [@ XUL@0x91d753 | mozilla::ipc::Nod… → [@ mozilla::ipc::NodeChannel::OnMessageReceived] [@ mozilla::ipc::NodeController::DeserializeEventMessage] [@ mozilla::ipc::MessageChannel::OnChannelErrorFromLink] [@ mozilla::ipc::MessageChannel::DispatchMessage ] [@ mozilla::ipc::MessageChannel::Maybe…

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
Flags: needinfo?(nika)
Attachment #9230190 - Flags: approval-mozilla-beta?

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

Approved for 91 beta 8, thanks.

Attachment #9230190 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.