Closed Bug 1246066 Opened 4 years ago Closed 4 years ago

Ensure the Push service finishes cleaning up before shutdown

Categories

(Core :: DOM: Push Notifications, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: Lina, Assigned: wchen)

Details

(Whiteboard: dom-triaged)

Attachments

(1 file)

Bug 1243856 makes it a lot more obvious when we're spinning the event loop on shutdown. It's noticeable in xpcshell on OS X; probably less so in real-world usage. I've seen three cases:

* `SubscriptionListener` will keep retrying failed requests (https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/push/PushServiceHttp2.jsm#264-269). We can probably keep these in a map and remove them on complete. On shutdown, we reject pending requests and clear the retry timers. OTOH, the only issue seems to be spurious error logging, so maybe it's not worth the trouble.

* `PushSubscriptionListener` tries to reconnect on shutdown (https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/push/PushServiceHttp2.jsm#755-757). `_shutdownConnections` removes the listener (https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/push/PushServiceHttp2.jsm#620-622), but this is only done during `xpcom-shutdown`. I wonder if we need to listen for an earlier observer topic, like `profile-change-net-teardown`, and clean up the connections then.

* Speaking of `xpcom-shutdown`, we're queueing a promise in `PushService.uninit`. This causes some interesting side effects. Sometimes, it prints a warning to the console (https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/promise/Promise.cpp#2115-2119); a few times, it crashes with an assertion in IDB (https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/quota/ActorsParent.cpp#5145). Adding a shutdown blocker (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/asyncshutdown/AsyncShutdown.jsm) might help here.

It would also be nice to remove this hack: https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/push/test/xpcshell/head.js#28-46
Whiteboard: dom-triaged
Bug 1243856 caused shutdown crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=21197523&repo=mozilla-inbound. Might be related to the second case.
Blocks: 1243856
Priority: -- → P1
Assignee: nobody → wchen
The crash in comment 1 is caused by opening a channel during shutdown [1]. The nsProtocolProxyService does shutdown cleanup during "xpcom-shutdown" [2]. In the crash case I see a task from the timer firing after the protocol proxy service shutdown which does some setup on a dead object [3] and leads to a crash on an assertion [4].

[1] https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/dom/push/PushServiceHttp2.jsm#538
[2] https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/netwerk/base/nsProtocolProxyService.cpp#528
[3] https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/netwerk/base/nsProtocolProxyService.cpp#1021
[4] https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/netwerk/base/nsProtocolProxyService.cpp#436

We probably need to find a different time for shutdown tasks that require the network. Also, if the timer causes tasks to run during "xpcom-shutdown" time, we need to make sure they don't call into network objects.
Would "quit-application" be early enough?
No longer blocks: 1243856
This patch clear the tasks in the timer because they call into resources that are not safe to use during xpcom-shutdown.

(In reply to Kit Cambridge [:kitcambridge] from comment #0)
> * `SubscriptionListener` will keep retrying failed requests
> (https://dxr.mozilla.org/mozilla-central/rev/
> 584870f1cbc5d060a57e147ce249f736956e2b62/dom/push/PushServiceHttp2.jsm#264-
> 269). We can probably keep these in a map and remove them on complete. On
> shutdown, we reject pending requests and clear the retry timers. OTOH, the
> only issue seems to be spurious error logging, so maybe it's not worth the
> trouble.
The retry will call into network code so if those tasks run during shutdown we can easily crash. I've added code to clear the timers.
> * `PushSubscriptionListener` tries to reconnect on shutdown
> (https://dxr.mozilla.org/mozilla-central/rev/
> 584870f1cbc5d060a57e147ce249f736956e2b62/dom/push/PushServiceHttp2.jsm#755-
> 757). `_shutdownConnections` removes the listener
> (https://dxr.mozilla.org/mozilla-central/rev/
> 584870f1cbc5d060a57e147ce249f736956e2b62/dom/push/PushServiceHttp2.jsm#620-
> 622), but this is only done during `xpcom-shutdown`. I wonder if we need to
> listen for an earlier observer topic, like `profile-change-net-teardown`,
> and clean up the connections then.
We shouldn't be doing anything interesting during xpcom-shutdown but it looks like we cancel HTTP channels there. From a quick inspection of that code, there is a code path that calls into the HTTP connection manager which is shutdown on profile-change-net-teardown (and xpcom-shutdown). I'm guessing there are other code paths that make it unsafe during xpcom-shutdown. As you suggested, I've changed it so that we cleanup during quit-application.
> * Speaking of `xpcom-shutdown`, we're queueing a promise in
> `PushService.uninit`. This causes some interesting side effects. Sometimes,
> it prints a warning to the console
> (https://dxr.mozilla.org/mozilla-central/rev/
> 584870f1cbc5d060a57e147ce249f736956e2b62/dom/promise/Promise.cpp#2115-2119);
> a few times, it crashes with an assertion in IDB
> (https://dxr.mozilla.org/mozilla-central/rev/
> 584870f1cbc5d060a57e147ce249f736956e2b62/dom/quota/ActorsParent.cpp#5145).
> Adding a shutdown blocker
> (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/
> asyncshutdown/AsyncShutdown.jsm) might help here.
It looks like the promise assertion comes from [1] which is unrelated to the push code. I have not seen an IDB crash after fixing Bug 1243856, so it may have been a task that wasn't cleaned up and trying to call into IDB during xpcom-shutdown.

[1] https://dxr.mozilla.org/mozilla-central/rev/be593a64d7c6a826260514fe758ef32a6ee580f7/testing/xpcshell/head.js#622
Attachment #8727691 - Flags: review?(kcambridge)
Comment on attachment 8727691 [details] [diff] [review]
Clear PushService timeout tasks on uninitialization.

Review of attachment 8727691 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks for doing this!

::: dom/push/PushService.jsm
@@ +106,5 @@
>    // reduce the quota for a record. Used for testing purposes.
>    _updateQuotaTestCallback: null,
>  
> +  // List of timeout ID of tasks to reduce quota.
> +  _updateQuotaTimeouts: [],

Could these all be sets? Might make the removals a bit cleaner, unless you'd like to keep the logging if the timer ID doesn't exist.
Attachment #8727691 - Flags: review?(kcambridge) → review+
https://hg.mozilla.org/mozilla-central/rev/605b761af96f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.