Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(310)

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

Created:
3 years, 8 months ago by Romain Pokrzywka
Modified:
3 years, 8 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -5 lines) Patch
M AUTHORS View 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_process_manager.cc View 1 2 1 chunk +10 lines, -5 lines 0 comments Download

Messages

Total messages: 21 (7 generated)
Romain Pokrzywka
Hi, I'd like you to review the following patch for Chromium. Regards, Romain Pokrzywka
3 years, 8 months 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 ...
3 years, 8 months 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 ...
3 years, 8 months 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 ...
3 years, 8 months 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 ...
3 years, 8 months ago (2017-04-13 22:15:18 UTC) #8
michaeln
ok, simple fix for a sticky problem, lgtm
3 years, 8 months 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 ...
3 years, 8 months 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 > ...
3 years, 8 months ago (2017-04-18 00:07:17 UTC) #11
nhiroki
Thank you. LGTM
3 years, 8 months 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
3 years, 8 months 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
3 years, 8 months 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 ...
3 years, 8 months 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 ...
3 years, 8 months ago (2017-04-19 08:18:40 UTC) #20
michaeln
3 years, 8 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?

Powered by Google App Engine
This is Rietveld 408576698