Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(27)

Issue 2781443006: ServiceWorker: Fix crash during shutdown in single-process mode (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 4 weeks ago by Romain Pokrzywka
Modified:
6 months ago
Reviewers:
michaeln, *nhiroki, dcheng
CC:
chromium-reviews, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, kinuko+serviceworker, horo+watch_chromium.org, darin-cc_chromium.org, kinuko+watch, tzik, blink-worker-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

ServiceWorker: Fix crash during shutdown in single-process mode In single-process mode, ServiceWorkerProcessManager::Shutdown() may end up being called after the RenderProcessHost was deleted, as part of deleting the default browser context. (which itself has to be destroyed after the RenderProcessHost since it's accessed during destruction) RenderProcessHost::FromID() returns a nullptr in this case, which causes a segfault. The crash is 100% reproducible after visiting youtube.com, which seems to register a service worker. This is happening at least with QtWebEngine in single-process mode, I haven't tested with Chromium's own shell or other implementations with single-process support. It might be specific to QtWebEngine's default browser context management too, in which case any tips on how to fix the lifecycle issue on the QtWebEngine side is appreciated. For reference, here's a link to the QtWebEngine change causing the default browser context to be deleted after the RenderProcessHostImpl is deleted: https://codereview.qt-project.org/#/c/120985/4 Review-Url: https://codereview.chromium.org/2781443006 Cr-Commit-Position: refs/heads/master@{#465123} Committed: https://chromium.googlesource.com/chromium/src/+/ff0fb24270d54a21f036fa77132b11fe03eafd06

Patch Set 1 #

Total comments: 1

Patch Set 2 : ServiceWorker: Fix crash during shutdown in single-process mode #

Total comments: 1

Patch Set 3 : ServiceWorker: Fix crash during shutdown in single-process mode #

Messages

Total messages: 21 (7 generated)
Romain Pokrzywka
Hi, I'd like you to review the following patch for Chromium. Regards, Romain Pokrzywka
6 months, 4 weeks ago (2017-03-28 19:32:55 UTC) #4
dcheng
Is it possible to just sequence things in the same order for --single-process? (I'm also ...
6 months, 4 weeks ago (2017-03-28 21:15:17 UTC) #5
michaeln
https://codereview.chromium.org/2781443006/diff/1/content/browser/service_worker/service_worker_process_manager.cc File content/browser/service_worker/service_worker_process_manager.cc (right): https://codereview.chromium.org/2781443006/diff/1/content/browser/service_worker/service_worker_process_manager.cc#newcode79 content/browser/service_worker/service_worker_process_manager.cc:79: I guess its not surprising there are shutdown ordering ...
6 months, 3 weeks ago (2017-03-29 23:02:08 UTC) #6
Romain Pokrzywka
Yeah that sounds good, I also like the fact that it's explicitly for single-process mode ...
6 months, 3 weeks ago (2017-03-29 23:08:46 UTC) #7
Romain Pokrzywka
On 2017/03/29 23:08:46, Romain Pokrzywka wrote: > Yeah that sounds good, I also like the ...
6 months, 1 week ago (2017-04-13 22:15:18 UTC) #8
michaeln
ok, simple fix for a sticky problem, lgtm
6 months, 1 week ago (2017-04-17 21:09:22 UTC) #9
nhiroki
Sorry for my delayed response. https://codereview.chromium.org/2781443006/diff/20001/content/browser/service_worker/service_worker_process_manager.cc File content/browser/service_worker/service_worker_process_manager.cc (right): https://codereview.chromium.org/2781443006/diff/20001/content/browser/service_worker/service_worker_process_manager.cc#newcode80 content/browser/service_worker/service_worker_process_manager.cc:80: if (!RenderProcessHost::run_renderer_in_process()) { Could ...
6 months, 1 week ago (2017-04-17 23:26:21 UTC) #10
Romain Pokrzywka
On 2017/04/17 23:26:21, nhiroki wrote: > Sorry for my delayed response. > > https://codereview.chromium.org/2781443006/diff/20001/content/browser/service_worker/service_worker_process_manager.cc > ...
6 months, 1 week ago (2017-04-18 00:07:17 UTC) #11
nhiroki
Thank you. LGTM
6 months, 1 week ago (2017-04-18 01:33:42 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2781443006/40001
6 months, 1 week ago (2017-04-18 01:42:00 UTC) #15
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/ff0fb24270d54a21f036fa77132b11fe03eafd06
6 months, 1 week ago (2017-04-18 03:11:15 UTC) #18
AlgorithMan
On 2017/04/18 03:11:15, commit-bot: I haz the power wrote: > Committed patchset #3 (id:40001) as ...
6 months, 1 week ago (2017-04-19 08:11:44 UTC) #19
AlgorithMan
On 2017/04/19 08:11:44, Viktor.Engelmann wrote: > I don't think it's a good idea to disable ...
6 months, 1 week ago (2017-04-19 08:18:40 UTC) #20
michaeln
6 months ago (2017-04-19 23:01:35 UTC) #21
Message was sent while issue was closed.
On 2017/04/19 08:18:40, AlgorithMan wrote:
> On 2017/04/19 08:11:44, Viktor.Engelmann wrote:
> > I don't think it's a good idea to disable the cleaning up completely in a
> > specific mode, even if that mode is unsupported.
> 
> I mean this could introduce a memory leak and since WebEngine users often run
> memory validation and often test in --single-process mode (although it's not
> officially supported) I can already smell the bug reports we will get because
of
> this fix.

Mostly I'm concerned with not having the null test in multi-process mode. In
multi-process mode, those objects should be there. A comment making that claim
is not enough.

I don't believe there's any risk of a memory leak here?
DecrementServiceWorkerRefCount does not free any memory in --single-process
mode, it decrements an int value and returns. Do you see a way for something to
leak?

It might make sense to have the IncrementServiceWorkerRefCount and
DecrementServiceWorkerRefCount  methods simply return in single-process mode to
make it clear that there's no value in calling the methods or harm in not
calling them. Or maybe just have GetWorkerRefcount
DCHECK(!run_renderer_in_process()) to make that point with zero production code
increase?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa