1633469 - Crash in [@ mozilla::dom::`anonymous namespace'::GetSubscriptionCallback::OnPushSubscription]

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect, P3)

Description

[Gabriele Svelto [:gsvelto]]

This bug is for crash report bp-2649db29-1fcb-4ed5-b436-3040f0200427.

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::`anonymous namespace'::GetSubscriptionCallback::OnPushSubscription dom/push/PushManager.cpp:177
1 xul.dll mozilla::dom::`anonymous namespace'::GetSubscriptionRunnable::Run dom/push/PushManager.cpp
2 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1200
3 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:481
4 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87
5 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308
6 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:290
7 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:137
8 xul.dll nsAppShell::Run widget/windows/nsAppShell.cpp:406
9 xul.dll XRE_RunAppShell toolkit/xre/nsEmbedFunctions.cpp:909

Thist started with buildid 20200409095500, looks like a regression from bug 1620152.

Comment 1

[Simon Giesecke [:sg] [he/him]]

This is probably due to the dispatch happening when the worker is already shutting down.

Not sure if https://searchfox.org/mozilla-central/rev/55a4faa52f72918efa51150d127aebdc057dc6cf/dom/push/PushManager.cpp#177 should rather simply be changed to

if (!r->Dispatch()) {
  return NS_ERROR_UNEXPECTED;
}

This seems to be called from https://searchfox.org/mozilla-central/rev/55a4faa52f72918efa51150d127aebdc057dc6cf/dom/push/PushManager.cpp#184, which is already an error handler that ignores the result.

Comment 2

[Jens Stutte [:jstutte]]

This might be related to bug 1435343: The Dispatch() function returns false and GetSubscriptionResultRunnable derives from WorkerRunnable, which implements a suspicious PreDispatch().

Edit: Suspicious probably only in the shutdown case, where we might see a race between the main thread and the worker thread? I remember to have seen some assertions that indicate something like this on a different bug, need to check back.

Comment 3

[Jens Stutte [:jstutte]]

Found, and maybe related: bug 1611094 ?

Comment 4

[Chris Peterson [:cpeterson]]

firefox77 = affected
firefox76 (beta) = fix-optional because it's not crashing (because 76 doesn't have the diagnostic assertion), but it might be affected the underlying worker shutdown problem.

Comment 5

[Jens Stutte [:jstutte]]

Maybe another case of event loop spinning during shutdown?

Comment 8

[Alex Chronopoulos [:achronop]]

Thank you! I am adding the signature to make the bug appear in the crash triage list.

Comment 9

[u608768]

This appears to have spiked over the past few weeks. Jens, can you take a look?

Comment 10

[Jens Stutte [:jstutte]]

Looking at the one and only use of OnPushSubscription, it seems that possible error returns are expected at least in theory. I think it is worth a try to follow Simon's suggestion from comment 1. Simon, can you give this a spin?

Comment 11

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1633469 - Propagate failure of Dispatch in GetSubscriptionCallback::OnPushSubscription. r=#dom-workers-and-storage

Comment 12

[Simon Giesecke [:sg] [he/him]]

I provided a patch along the lines of comment 1.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f857195f8c1
Propagate failure of Dispatch in GetSubscriptionCallback::OnPushSubscription. r=jstutte

Comment 14

[Simon Giesecke [:sg] [he/him]]

While the patch removes the crash here, this will just transform into another error situation. I noticed that quite some of the URLs (where they have been reported) are from youtube.com here.

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4f857195f8c1