1935475 - Crash in [@ std::_Func_class<T>::operator() | mozilla::dom::notification::NotificationParent::RecvShow]

Status: RESOLVED FIXED

(Core :: DOM: Notifications, defect)

Description

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

20-30 crashes from 6-8 installs of Firefox beta builds for v134.

Crash report: https://crash-stats.mozilla.org/report/index/4c6764b4-d168-4d1d-9987-076900241205

MOZ_CRASH Reason:

MOZ_RELEASE_ASSERT(isSome())

Top 10 frames:

0  xul.dll  std::_Func_class<void, const mozilla::CopyableErrorResult&>::operator()(mozil...  mfbt/Maybe.h:929
0  xul.dll  mozilla::dom::notification::NotificationParent::RecvShow(std::function<void (...  dom/notification/NotificationParent.cpp:0
1  xul.dll  mozilla::dom::notification::PNotificationParent::OnMessageReceived(IPC::Messa...  ipc/ipdl/PNotificationParent.cpp:152
2  xul.dll  mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecyc...  ipc/glue/MessageChannel.cpp:1727
2  xul.dll  mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecyclePro...  ipc/glue/MessageChannel.cpp:1654
2  xul.dll  mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::ActorLifecycleProxy*, ...  ipc/glue/MessageChannel.cpp:1445
2  xul.dll  mozilla::ipc::MessageChannel::MessageTask::Run()  ipc/glue/MessageChannel.cpp:1545
3  xul.dll  mozilla::RunnableTask::Run()  xpcom/threads/TaskController.cpp:620
3  xul.dll  mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::det...  xpcom/threads/TaskController.cpp:947
4  xul.dll  mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detai...  xpcom/threads/TaskController.cpp:770

Comment 1

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

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 20 desktop browser crashes on beta

For more information, please visit BugBot documentation.

Comment 2

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

The bug is marked as tracked for firefox134 (beta) and tracked for firefox135 (nightly). However, the bug still isn't assigned.

:jstutte, could you please find an assignee for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 3

[Jens Stutte [:jstutte]]

I tried to break up a minidump, but also there I get no better line number for NotificationParent::RecvShow. Anyhow, it seems like MOZ_RELEASE_ASSERT(isSome()) must come from value which would mean that mResolver.emplace(std::move(aResolver)); left mResolver as Nothing ? I cannot really see how that would be possible coming from here ?

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

I think we must be seeing:

• The call to show...
• Results in a synchronous observer notification which takes the value...
• And then in the event Show returns an error we blindly take the value without checking if it's still there.

As this comment points out, the XUL alerts service will absolutely synchronously generate an observer notification in DND mode, but it returns NS_OK so it should not be the source of the problem. But for the windows toast handler we absolutely add a bunch of callbacks which as the diagram shows all like to call SendFinished which will result in the value being taken above, and there are a lot of potential error returns after we add the callbacks, although I'm not immediately sure what semantics they operate under. (Would they normally asynchronously be run during the normal windows message loop?)

So I think the straightforward thing to do here is to make sure that we check to make sure the resolver is still around in the error branch before trying to take it. I'll provide a patch for that now.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1935475 - Defend against sync callbacks in Show call.

The crash in this bug seems to indicate that we are receiving a sync
observer notification callback during our call to Show() and this is
resulting in Show() returning an error code. Because the callbacks
will already take mResolver, mResolver will be Nothing() on return.
As analyzed in the bug the windows toast notifier seems capable of
generating this series of events, but this is somewhat speculative
and the patch here is defensive in nature.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/941ad27caa8e
Defend against sync callbacks in Show call. r=edenchuang

Comment 7

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/941ad27caa8e

Comment 8

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

The patch landed in nightly and beta is affected.
:asuth, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox134 to wontfix.

For more information, please visit BugBot documentation.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1935475 - Defend against sync callbacks in Show call.

The crash in this bug seems to indicate that we are receiving a sync
observer notification callback during our call to Show() and this is
resulting in Show() returning an error code. Because the callbacks
will already take mResolver, mResolver will be Nothing() on return.
As analyzed in the bug the windows toast notifier seems capable of
generating this series of events, but this is somewhat speculative
and the patch here is defensive in nature.

Original Revision: https://phabricator.services.mozilla.com/D231434

Comment 10

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: (Safe assertion) crashes
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a - don't know how to reproduce
• Risk associated with taking this patch: extremely low
• Explanation of risk level: Adds a check/guard that explicitly guards against the assertion-based crash, otherwise does not change control flow. The crash is an indication that the promise was already resolved so this should not change control-flow.
• String changes made/needed: none
• Is Android affected?: no

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/27bbd2bd3439