Shutdown Service Workers before the profile is destroyed

Shutdown Service Workers before the profile is destroyed

This fixes a crash when closing an incognito browser window that used
Service Workers.

Service Workers keep render process hosts alive. Service Workers are
shutdown during the destruction of a profile (in ~StoragePartitionImpl).
If the shutdown causes render process hosts to die, ExtensionService
(and possibly other clients) receive a
NOTIFICATION_RENDERER_PROCESS_TERMINATED and try to access the
half-destroyed profile, causing a crash.

We don't see this in a non-incognito window since the main profile is
not destroyed until after browser shutdown, and we had special code to
shutdown Service Workers there.

This patch changes the special Service Worker shutdown code to happen
before a profile is destroyed.

BUG=419290
Committed: https://crrev.com/41f417516ab894faa1ebfe33cd6197cb072f1bf7
Cr-Commit-Position: refs/heads/master@{#301800}

[falken, 2014-10-21 09:36:34]

falken@chromium.org changed reviewers:
+ dominicc@chromium.org, horo@chromium.org, michaeln@chromium.org

[falken, 2014-10-21 09:36:36]

This is a bit WIP still, see comments inline.

I have a fix in ExtensionService that seems to work, just replaced the
  profile.IsSameProfile(host_profile.GetOriginalProfile())
with
  profile.IsSameProfile(host_profile)

But it seems the RPHs outliving the Profile is violating some assumptions in the
codebase.

https://codereview.chromium.org/653633004/diff/1/chrome/browser/chrome_servic...
File chrome/browser/chrome_service_worker_browsertest.cc (right):

https://codereview.chromium.org/653633004/diff/1/chrome/browser/chrome_servic...
chrome/browser/chrome_service_worker_browsertest.cc:109: // Test passes if we
don't crash.
On ToT this test just prints "Test failed" with no indication why. I'm assuming
it's because the test crashed.

https://codereview.chromium.org/653633004/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_destroyer.cc (right):

https://codereview.chromium.org/653633004/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_destroyer.cc:45: profile,
base::Bind(ShutdownServiceWorkerContext));
I'm not sure all codepaths that destroy a Profile go through here. Ideally we
listen to PROFILE_DESTROYED_NOTIFICATION but that is only observable in chrome,
not in content. And we don't have Service Worker classes in chrome.

https://codereview.chromium.org/653633004/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_destroyer.cc:60: // RenderProcessHosts in order
to erase private data quickly, and
This comment is a bit sketchy, it seems to contradict what we see with
ExtensionService.

https://codereview.chromium.org/653633004/diff/1/content/public/browser/servi...
File content/public/browser/service_worker_context.h (left):

https://codereview.chromium.org/653633004/diff/1/content/public/browser/servi...
content/public/browser/service_worker_context.h:16: // the IO thread.
Shutdown must be called on UI thread.

[michaeln, 2014-10-21 23:24:14]

fyi: I think normal partitions are never deleted till the end due to the
problems in the shutdown codepaths.

https://codereview.chromium.org/653633004/diff/1/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (left):

https://codereview.chromium.org/653633004/diff/1/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:353:
partition->GetServiceWorkerContext()->Terminate();
I was wondering where the Terminate() method came into play.

https://codereview.chromium.org/653633004/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_destroyer.cc (right):

https://codereview.chromium.org/653633004/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_destroyer.cc:45: profile,
base::Bind(ShutdownServiceWorkerContext));
Profile::MaybeSendDestroyedNotification() could call a method on its subclass to
propagate this event. If this is only use of SWC->Terminate(), we could probably
remove it from the public api and traverse storage partitions in the content
lib.

base::NotifyImminentDestruction() ?

[michaeln, 2014-10-21 23:43:03]

> I have a fix in ExtensionService that seems to work, just replaced the
>   profile.IsSameProfile(host_profile.GetOriginalProfile())
> with
>   profile.IsSameProfile(host_profile)

Doesn't that change the semantics of the test such that it returns early instead
of even trying to do its book keeping?

> But it seems the RPHs outliving the Profile is violating some assumptions in
the
> codebase.

Yup, despite the comment you pointed at earlier.

> On ToT this test just prints "Test failed" with no indication why. I'm
assuming
> it's because the test crashed.

To clarify, with the CL patched in, this test passes. Is that right?

[falken, 2014-10-22 01:27:12]

Thanks for the comments:

>> I have a fix in ExtensionService that seems to work, just replaced the
>>   profile.IsSameProfile(host_profile.GetOriginalProfile())
>> with
>>   profile.IsSameProfile(host_profile)

> Doesn't that change the semantics of the test such that it returns early
instead
> of even trying to do its book keeping?

It's equivalent because IsSampleProfile() effectively does GetOriginalProfile()
on its argument itself without actually using a virtual method.

>> On ToT this test just prints "Test failed" with no indication why. I'm
assuming
>> it's because the test crashed.

> To clarify, with the CL patched in, this test passes. Is that right?

Yep it passes in Debug and Release with the CL. Without the CL, it hits a RPH
leak assert in Debug and just fails with no other output in Release.

https://codereview.chromium.org/653633004/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_destroyer.cc (right):

https://codereview.chromium.org/653633004/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_destroyer.cc:45: profile,
base::Bind(ShutdownServiceWorkerContext));
On 2014/10/21 23:24:13, michaeln wrote:
> Profile::MaybeSendDestroyedNotification() could call a method on its subclass
to
> propagate this event. If this is only use of SWC->Terminate(), we could
probably
> remove it from the public api and traverse storage partitions in the content
> lib.

That sounds promising and more bulletproof than hooking in ProfileDestroyer.
Something like NotifyWillBeDestroyed() that BrowserContext overrides?

Alternatively, is it better to make a chrome class that listens to the
NOTIFICATION_PROFILE_DESTROYED and destroys SWC? It feels like the virtual
method is just making for SW a special way to listen for the notification.

> base::NotifyImminentDestruction() ?

I'm not sure what you mean by this one.

[falken, 2014-10-22 05:18:13]

Patchset #2 (id:20001) has been deleted

[falken, 2014-10-22 05:23:22]

falken@chromium.org changed reviewers:
+ jam@chromium.org

[falken, 2014-10-22 05:23:23]

I changed the patch to hook into MaybeSendDestroyedNotification as michaeln
suggested.

jam: please review as OWNER
others: FYI for ServiceWorker

[michaeln, 2014-10-22 22:52:46]

jam should take a look at the public api changes

https://codereview.chromium.org/653633004/diff/60001/content/public/browser/b...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/653633004/diff/60001/content/public/browser/b...
content/public/browser/browser_context.h:169: void NotifyWillBeDestroyed();
I think this is nicer than what is currently being done. In keeping with the
other concrete methods in this class, probably should make it a static method
that takes a BrowserContext* as input.

https://codereview.chromium.org/653633004/diff/60001/content/public/browser/s...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/653633004/diff/60001/content/public/browser/s...
content/public/browser/service_worker_context.h:58: virtual void Terminate() =
0;
I think this method is no longer needed.

[falken, 2014-10-23 02:06:38]

Thanks, PTAL.

https://codereview.chromium.org/653633004/diff/60001/content/public/browser/b...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/653633004/diff/60001/content/public/browser/b...
content/public/browser/browser_context.h:169: void NotifyWillBeDestroyed();
On 2014/10/22 22:52:46, michaeln wrote:
> I think this is nicer than what is currently being done. In keeping with the
> other concrete methods in this class, probably should make it a static method
> that takes a BrowserContext* as input.
> 

Done.

https://codereview.chromium.org/653633004/diff/60001/content/public/browser/s...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/653633004/diff/60001/content/public/browser/s...
content/public/browser/service_worker_context.h:58: virtual void Terminate() =
0;
On 2014/10/22 22:52:46, michaeln wrote:
> I think this method is no longer needed.

Done.

[jam, 2014-10-23 22:45:22]

https://codereview.chromium.org/653633004/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/653633004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:256: NotifyWillBeDestroyed(this);
we shouldn't have content depend on every embedder knowing to do this or else
something in content (SW) breaks. can you find another way of initiating this
from within content?

[michaeln, 2014-10-24 00:45:26]

i think the proposed change is an improvement over the existing code, so from
that point of view lgtm

> chrome/browser/profiles/profile.cc:256: NotifyWillBeDestroyed(this);
> we shouldn't have content depend on every embedder knowing to do this or else
> something in content (SW) breaks.

agreed

> can you find another way of initiating this
> from within content?

do you have any suggestions?

[falken, 2014-10-24 00:53:34]

https://codereview.chromium.org/653633004/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/653633004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile.cc:256: NotifyWillBeDestroyed(this);
On 2014/10/23 22:45:22, jam wrote:
> we shouldn't have content depend on every embedder knowing to do this or else
> something in content (SW) breaks. can you find another way of initiating this
> from within content?

It's actually chrome stuff that's breaking because it expects the Profiles to
outlive RPHs (this particular crash is in ExtensionService). The current code
already depends on chrome shutting down Service Workers in BrowserProcessImpl to
prevent DCHECKs in chrome. This patch moves that special shutdown code to a
place that works for incognito profiles too.

It seems SW needs some signal from the embedder that it's time to shutdown.
Another option may be to change chrome's assumptions about RPH lifetimes, but
I'm not sure how involved that is.

[falken, 2014-10-28 13:14:41]

jam: ping

This seems like an improvement of existing code, although it still relies on a
signal from chrome. I can't see a way to initiate the shutdown at the right time
from content.

[jam, 2014-10-29 05:12:16]

lgtm for now to fix this problem.

i don't have time to look into this deeply to suggest an alternative atm

On 2014/10/24 00:53:34, falken wrote:
>
https://codereview.chromium.org/653633004/diff/100001/chrome/browser/profiles...
> File chrome/browser/profiles/profile.cc (right):
> 
>
https://codereview.chromium.org/653633004/diff/100001/chrome/browser/profiles...
> chrome/browser/profiles/profile.cc:256: NotifyWillBeDestroyed(this);
> On 2014/10/23 22:45:22, jam wrote:
> > we shouldn't have content depend on every embedder knowing to do this or
else
> > something in content (SW) breaks. can you find another way of initiating
this
> > from within content?
> 
> It's actually chrome stuff that's breaking because it expects the Profiles to
> outlive RPHs

this assumption is central in content btw

> (this particular crash is in ExtensionService). The current code
> already depends on chrome shutting down Service Workers in BrowserProcessImpl
to
> prevent DCHECKs in chrome. This patch moves that special shutdown code to a
> place that works for incognito profiles too.
> 
> It seems SW needs some signal from the embedder that it's time to shutdown.
> Another option may be to change chrome's assumptions about RPH lifetimes, but
> I'm not sure how involved that is.

[falken, 2014-10-29 05:30:31]

Thanks for the review.

On 2014/10/29 05:12:16, jam wrote:
> > It's actually chrome stuff that's breaking because it expects the Profiles
to
> > outlive RPHs
> 
> this assumption is central in content btw

OK, noted.

[falken, 2014-10-29 05:30:45]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-10-29 05:31:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/653633004/120001

[commit-bot: I haz the power, 2014-10-29 07:03:49]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2014-10-29 07:04:28]

Patchset 6 (id:??) landed as
https://crrev.com/41f417516ab894faa1ebfe33cd6197cb072f1bf7
Cr-Commit-Position: refs/heads/master@{#301800}