Site/process tracking should work per-StoragePartition instead of per-BrowserContext |
|||
Issue descriptionCurrently, there is one SiteProcessCountTracker per BrowserContext, which allows process reuse across StoragePartitions. This has caused renderer kills due to isolated origins incorrectly reusing a process from a <webview> in the sign-in isolation trial (see issues 751916 and 751920), but it could also affect ServiceWorkers with PlzNavigate, for which the affected process reuse policy was originally implemented. Though fwiw, nasko@ mentioned that he isn't seeing any of the same crashes in the PlzNavigate trial. I've got a short-term fix in progress to restrict this tracking to the default StoragePartition (https://chromium-review.googlesource.com/c/602707/), which will prevent cross-StoragePartition reuse. But the right fix is probably to move this tracking to be done per-StoragePartition, so that we can properly deal with ServiceWorkers in <webviews>. Camille, would you be able to take a look at this? falken@: do you have any thoughts about using ServiceWorkers in non-default StoragePartitions (e.g., in <webviews> - is this even possible?), and whether the ServiceWorker/PlzNavigate performance issue 705318 might apply to that?
,
Aug 7 2017
Comment 1: We're talking about the Chrome Apps <webview> tag, rather than Android WebView.
,
Aug 7 2017
clamy@ points out we'll need to update UnmatchedServiceWorkerProcessTracker as well, since that might use an existing ServiceWorker process for a navigation in a different StoragePartition today. If so, that would probably also lead to renderer kills.
,
Aug 8 2017
@alexmos: I don't have time to look at it right now, may be in a few weeks. Right now it's only affecting <webview>, which is a small enough portion of navigations. However, if we move towards having more navigation use non-default StoragePartition we should fix this quickly.
,
Dec 13 2017
Note that ServiceWorkers created from <webview> tags are indeed possible, and falken@ is fixing them to stay in the <webview>'s StoragePartition in https://chromium-review.googlesource.com/c/chromium/src/+/807973. When that lands, the ServiceWorker will end up using a new process, rather than reusing an existing process for the <webview>, because of this issue. Fixing SiteProcessCountTracker to work across StoragePartitions should allow the ServiceWorker to share the <webview>'s process, which I think is more desirable. Also +avi@ just for awareness, since he was looking at some of this process reuse logic recently.
,
Dec 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9 commit 24abf78274e01a17c4f0a4c8cc18e55720f3b3b9 Author: Matt Falkenhagen <falken@chromium.org> Date: Fri Dec 15 23:55:11 2017 service worker: Teach StoragePartition the Site URL to use for service workers. The "service worker context" is attached to a StoragePartition. When this context tries to start a service worker, it asks SiteInstance for a process to start the worker in. This process must be in the same StoragePartition as the context. In some cases like the <webview> tag, a non-default StoragePartition is created for guests, but the service worker was trying to start up in a process for the default StoragePartition since it created a SiteInstance using the script URL of the worker (e.g., https://www.example.com) and got a process from that. The solution is to stash the site URL on the StoragePartition when it's for guests (e.g., chrome-guest://someapp/somepath), and have service worker use that site URL when creating a SiteInstance. Bug: 781313 , 752667 Change-Id: I8bf7ee8fd78a42e6c4e23684160c18da4e5189e5 Reviewed-on: https://chromium-review.googlesource.com/807973 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Tsuyoshi Horo <horo@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#524528} [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/chrome/browser/chromeos/power/renderer_freezer_unittest.cc [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/chrome/browser/media_galleries/media_file_system_registry_unittest.cc [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/chrome/browser/metrics/chrome_stability_metrics_provider_unittest.cc [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/components/visitedlink/test/visitedlink_unittest.cc [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/browser/renderer_host/render_process_host_unittest.cc [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/browser/renderer_host/text_input_client_mac_unittest.mm [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/browser/service_worker/service_worker_context_wrapper.cc [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/browser/service_worker/service_worker_context_wrapper.h [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/browser/service_worker/service_worker_process_manager.cc [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/browser/service_worker/service_worker_process_manager.h [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/browser/service_worker/service_worker_process_manager_unittest.cc [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/browser/storage_partition_impl.h [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/public/browser/render_process_host_factory.h [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/public/test/mock_render_process_host.cc [modify] https://crrev.com/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9/content/public/test/mock_render_process_host.h |
|||
►
Sign in to add a comment |
|||
Comment 1 by falken@chromium.org
, Aug 4 2017