|
|
Created:
3 years, 8 months ago by Romain Pokrzywka Modified:
3 years, 8 months ago 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. |
DescriptionServiceWorker: 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)
Description was changed from ========== 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 ========== to ========== 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 ==========
romain.pokrzywka@gmail.com changed reviewers: + dcheng@chromium.org, michaeln@chromium.org, nhiroki@chromium.org
romain.pokrzywka@gmail.com changed required reviewers: + nhiroki@chromium.org
Hi, I'd like you to review the following patch for Chromium. Regards, Romain Pokrzywka
Is it possible to just sequence things in the same order for --single-process? (I'm also unsure of how official --single-process support is these days...)
https://codereview.chromium.org/2781443006/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_process_manager.cc (right): https://codereview.chromium.org/2781443006/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_process_manager.cc:79: I guess its not surprising there are shutdown ordering bugs with QtWebEngine's single process usage. The process refcounting is all managing when to Cleanup() the rph. That is not really applicable to single process mode. If we must change code here to "fix it", i'd vote to do something that's clearly more specific to single process mode (so the contract is clear in multi-process that the RPH outlives SWPM::Shutdown). Maybe only enter the for loop below? if (!RenderProcessHost::run_renderer_in_process()) { for (...) }
Yeah that sounds good, I also like the fact that it's explicitly for single-process mode then. I'll give it a try and update the CL then. Thanks for the feedback!
On 2017/03/29 23:08:46, Romain Pokrzywka wrote: > Yeah that sounds good, I also like the fact that it's explicitly for > single-process mode then. I'll give it a try and update the CL then. Thanks for > the feedback! OK that solution works just fine too, new patch uploaded.
ok, simple fix for a sticky problem, lgtm
Sorry for my delayed response. https://codereview.chromium.org/2781443006/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_process_manager.cc (right): https://codereview.chromium.org/2781443006/diff/20001/content/browser/service... content/browser/service_worker/service_worker_process_manager.cc:80: if (!RenderProcessHost::run_renderer_in_process()) { Could you add a comment about why we need to skip this in the single process mode like the 1st paragraph on the CL description?
On 2017/04/17 23:26:21, nhiroki wrote: > Sorry for my delayed response. > > https://codereview.chromium.org/2781443006/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_process_manager.cc (right): > > https://codereview.chromium.org/2781443006/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_process_manager.cc:80: if > (!RenderProcessHost::run_renderer_in_process()) { > Could you add a comment about why we need to skip this in the single process > mode like the 1st paragraph on the CL description? Sure thing, added a comment in the code too. Please lgtm again if needed. Best, Romain
Thank you. LGTM
The CQ bit was checked by romain.pokrzywka@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/2781443006/#ps40001 (title: "ServiceWorker: Fix crash during shutdown in single-process mode")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492479683133480, "parent_rev": "0d9239bcb33250cd613229d19a90af7cfa28d861", "commit_rev": "ff0fb24270d54a21f036fa77132b11fe03eafd06"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/ff0fb24270d54a21f036fa77132b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/ff0fb24270d54a21f036fa77132b...
Message was sent while issue was closed.
On 2017/04/18 03:11:15, commit-bot: I haz the power wrote: > Committed patchset #3 (id:40001) as > https://chromium.googlesource.com/chromium/src/+/ff0fb24270d54a21f036fa77132b... 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 think it would have been cleaner to test the result of RenderProcessHost::FromID(it->second.process_id) before dereferencing it.
Message was sent while issue was closed.
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.
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? |