1399646 - Fix shutdown issues with StreamFilters in use (and otherwise)

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Kris Maglione [:kmag]]

There are currently some shutdown issues when StreamFilters in a child process are active for requests in the parent.

The main issue is that the combination of the BackgroundPageThumbs service running during shutdown along with unexpected reentrancy in the service worker registrar tends to cause deadlocks at shutdown for short sessions (as triggered by ts_paint tests). That results in the process being killed before it gets passed the profile-before-change shutdown phase, and the background threads therefore shutting down before the actors have had a chance to clean up after themselves.

Aside from that, we don't do a particularly good job of making sure StreamFilter instances are closed as soon as possible after the page they belong to is closed. There are probably more elegant ways we should handle that, but my current solution is to disconnect the actor as soon as we try to dispatch an event to a page that has scriptability blocked.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1399646: Part 1 - Destroy BackgroundPageThumbs instance at shutdown.

During extremely short sessions (such as the ones triggered by many tests),
the code run by BackgroundPageThumbs during shutdown can trigger
hard-to-diagnose issues, among the most serious being a deadlock in the
service worker registrar.

Calling the (currently unused) _destroy() method at the start of shutdown
seems to prevent the majority of these problems.

Review commit: https://reviewboard.mozilla.org/r/179534/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179534/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1399646: Part 2 - Use the async shutdown service for ServiceWorkerRegistrar.

The current shutdown handling code is susceptible to deadlocks, since it spins
the event loop while it holds mMonitor, and other main thread methods which
try to acquire mMonitor can be called from code that runs while the event loop
is spinning.

My initial solution was just to release mMonitor before spinning the event
loop, but at this point I think it makes more sense to switch to the
standardized AsyncShutdown routines, which provide better diagnostics and
allow us to avoid one more nested event loop during shutdown.

Review commit: https://reviewboard.mozilla.org/r/179536/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179536/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1399646: Part 3 - Improve handling of StreamFilters at shutdown.

The main change here is to disconnect stream filters immediately if we try to
send start or data events to a window that's already been destroyed.

It also fixes a race where we end up in the wrong state if a stop event
arrives while the channel is being disconnected.

Review commit: https://reviewboard.mozilla.org/r/179538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179538/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1399646: Part 4 - Increase the extension shutdown blocker timeout.

The current timeout was added to deal with some shutdown deadlocks that were
happining in the wild, but were hard to reproduce locally and therefore
diagnose. It's not clear whether the bulk of those have been fixed, so I'm
reluctant to remove the timeout entirely.

But the current 1s timeout is quite short, and doesn't allow for proper
cleanup in a lot of legitimate cases. The async shutdown service starts to
emit warnings at 10s, so 8s gives us enough time to avoid at least that.

Review commit: https://reviewboard.mozilla.org/r/179540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179540/

Comment 5

[:Gijs (he/him)]

Comment on attachment 8907847 [details]
Bug 1399646: Part 1 - Destroy BackgroundPageThumbs instance at shutdown.

https://reviewboard.mozilla.org/r/179534/#review184720

::: commit-message-43d65:8
(Diff revision 1)
> +Calling the (currently unused) _destroy() method at the start of shutdown
> +seems to prevent the majority of these problems.

Doesn't look unused:

https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#634


Does that mean we should be removing that callsite?

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8907847 [details]
Bug 1399646: Part 1 - Destroy BackgroundPageThumbs instance at shutdown.

https://reviewboard.mozilla.org/r/179534/#review184720

> Doesn't look unused:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#634
> 
> 
> Does that mean we should be removing that callsite?

Hm. I couldn't find any uses of it, but I didn't check tests.

But, no, I don't think we should remove that call site, since it's there to prevent false positive window leak failures for windows created by the background thumbs service. I think the shutdown observer happens too late to handle that.

But the `_destroy()` method can handle being called multiple times, so I don't think having both is a problem.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8907847 [details]
Bug 1399646: Part 1 - Destroy BackgroundPageThumbs instance at shutdown.

https://reviewboard.mozilla.org/r/179534/#review184726

OK, then let's do it.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8907850 [details]
Bug 1399646: Part 4 - Increase the extension shutdown blocker timeout.

https://reviewboard.mozilla.org/r/179540/#review184732

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8907849 [details]
Bug 1399646: Part 3 - Improve handling of StreamFilters at shutdown.

https://reviewboard.mozilla.org/r/179538/#review184734

I'm not familiar with xpc::Scriptability, but what you're doing looks reasonable.

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8220fde3db60b243e9d8b8abd82562bb31509deb
Bug 1399646: Part 1 - Destroy BackgroundPageThumbs instance at shutdown. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/f87e1d6810536ab0dc940f95563869d86aa937d3
Bug 1399646: Part 3 - Improve handling of StreamFilters at shutdown. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7c638b31626ace9d0951e7a69450615673c9656
Bug 1399646: Part 4 - Increase the extension shutdown blocker timeout. r=mixedpuppy

Comment 11

[Kris Maglione [:kmag]]

The three parts I pushed should be enough to fix most of the talos failures, but there will probably be intermittents until the last part lands.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8220fde3db60
https://hg.mozilla.org/mozilla-central/rev/f87e1d681053
https://hg.mozilla.org/mozilla-central/rev/c7c638b31626

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8907848 [details]
Bug 1399646: Part 2 - Use the async shutdown service for ServiceWorkerRegistrar.

https://reviewboard.mozilla.org/r/179536/#review184924

::: dom/workers/ServiceWorkerRegistrar.cpp:1029
(Diff revision 1)
>    }
>  
> +  rv = GetShutdownPhase()->AddBlocker(
> +    this, NS_LITERAL_STRING(__FILE__), __LINE__,
> +    NS_LITERAL_STRING("ServiceWorkerRegistrar"));
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

if (NS_WARN_IF(NS_FAILED(rv))) {
  return;
}

::: dom/workers/ServiceWorkerRegistrar.cpp:1080
(Diff revision 1)
> +}
> +
> +NS_IMETHODIMP
> +ServiceWorkerRegistrar::GetName(nsAString& aName)
> +{
> +  aName = NS_LITERAL_STRING("ServiceWorkerRegistar");

This should be "ServiceWorkerRegistrar: Flushing data" or anything with a meaning.

::: dom/workers/ServiceWorkerRegistrar.cpp:1099
(Diff revision 1)
> +  MOZ_RELEASE_ASSERT(svc);
>  
> -  MOZ_ALWAYS_TRUE(SpinEventLoopUntil([&]() { return completed; }));
> +  nsCOMPtr<nsIAsyncShutdownClient> client;
> +  Unused << svc->GetProfileBeforeChange(getter_AddRefs(client));
> +  MOZ_RELEASE_ASSERT(client);
> +  return Move(client);

Is Move(client) equal to client.forget() ? I guess so.

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4890481365e66c1ef2f7126481908b64002c113f
Bug 1399646: Part 2 - Use the async shutdown service for ServiceWorkerRegistrar. r=baku

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/4890481365e6