Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925623004:
Only use SiteInstance process reuse for ServiceWorker with no failures (Closed)
|
Created:
3 years, 6 months ago by clamy Modified:
3 years, 6 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, 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. |
DescriptionOnly use SiteInstance process reuse for ServiceWorker with no failures
This CL ensures we will only attempt to use the SiteInstance process
reuse mechanism when creating a SiteInstance for a ServiceWorker when
the ServiceWorker has not failed to start. Otherwise, we will try to
start it in a brand new process.
BUG=728030
Review-Url: https://codereview.chromium.org/2925623004
Cr-Commit-Position: refs/heads/master@{#477493}
Committed: https://chromium.googlesource.com/chromium/src/+/47dc328bb08ac9c66848eeaf68fc8e96a39975cb
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comments #
Total comments: 4
Messages
Total messages: 24 (14 generated)
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Only use SiteInstance process reuse for ServiceWorker with no failures Thsi CL ensures we will only attempt to use the SiteInstance process reuse mechanism when creating a SiteInstance for a ServiceWorker when the ServiceWorker has not failed to start. Otherwise, we will try to start it in a brand new process. BUG=728030 ========== to ========== Only use SiteInstance process reuse for ServiceWorker with no failures This CL ensures we will only attempt to use the SiteInstance process reuse mechanism when creating a SiteInstance for a ServiceWorker when the ServiceWorker has not failed to start. Otherwise, we will try to start it in a brand new process. BUG=728030 ==========
Description was changed from ========== Only use SiteInstance process reuse for ServiceWorker with no failures This CL ensures we will only attempt to use the SiteInstance process reuse mechanism when creating a SiteInstance for a ServiceWorker when the ServiceWorker has not failed to start. Otherwise, we will try to start it in a brand new process. BUG=728030 ========== to ========== Only use SiteInstance process reuse for ServiceWorker with no failures This CL ensures we will only attempt to use the SiteInstance process reuse mechanism when creating a SiteInstance for a ServiceWorker when the ServiceWorker has not failed to start. Otherwise, we will try to start it in a brand new process. BUG=728030 ==========
clamy@chromium.org changed reviewers: + falken@chromium.org, horo@chromium.org
@horo, falken: PTAL. This follows horo's suggestion in the bug.
Seems like a good idea. Could we add a unit test in service_worker_process_manager_unittest.cc for this? https://codereview.chromium.org/2925623004/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_process_manager.cc (right): https://codereview.chromium.org/2925623004/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_process_manager.cc:220: // existing process if possible. Now these sentences read like a contradiction :) How about something like: // ServiceWorkerProcessManager does not know of any renderer processes that are available for |pattern|. Create a SiteInstance and ask for a renderer process. Attempt to reuse an existing process if possible.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Thanks! I have added some unit tests in the latest patch set. https://codereview.chromium.org/2925623004/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_process_manager.cc (right): https://codereview.chromium.org/2925623004/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_process_manager.cc:220: // existing process if possible. On 2017/06/06 13:32:43, falken wrote: > Now these sentences read like a contradiction :) How about something like: > > // ServiceWorkerProcessManager does not know of any renderer processes that are > available for |pattern|. Create a SiteInstance and ask for a renderer process. > Attempt to reuse an existing process if possible. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks for the test, lgtm https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_process_manager.cc (right): https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_process_manager.cc:226: SiteInstanceImpl::CreateForURL(browser_context_, script_url); does it make sense to DCHECK_NE(REUSE_PENDING_OR_COMMITTED_SITE)? Or can we DCHECK something more specific? I'm just wondering if we're sure that CreateForURL will create a new SiteInstanceImpl that definitely doesn't have the REUSE flag set (i.e., it's not going to return an existing SiteInstanceImpl). Reading the code comments and impl, that indeed seems true, so we could DCHECK this assumption? https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_process_manager_unittest.cc (right): https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_process_manager_unittest.cc:248: // Create a process that is hosting a frame with URL |patter_|. |pattern_| https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_process_manager_unittest.cc:294: // Create a process that is hosting a frame with URL |patter_|. ditto https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_process_manager_unittest.cc:315: // An new process should be allocated to the worker. nit: A new
lgtm
The CQ bit was checked by jam@chromium.org
On 2017/06/07 00:12:22, falken wrote: > thanks for the test, lgtm > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_process_manager.cc (right): > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_process_manager.cc:226: > SiteInstanceImpl::CreateForURL(browser_context_, script_url); > does it make sense to DCHECK_NE(REUSE_PENDING_OR_COMMITTED_SITE)? Or can we > DCHECK something more specific? > > I'm just wondering if we're sure that CreateForURL will create a new > SiteInstanceImpl that definitely doesn't have the REUSE flag set (i.e., it's not > going to return an existing SiteInstanceImpl). Reading the code comments and > impl, that indeed seems true, so we could DCHECK this assumption? > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_process_manager_unittest.cc > (right): > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_process_manager_unittest.cc:248: > // Create a process that is hosting a frame with URL |patter_|. > |pattern_| > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_process_manager_unittest.cc:294: > // Create a process that is hosting a frame with URL |patter_|. > ditto > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_process_manager_unittest.cc:315: > // An new process should be allocated to the worker. > nit: A new
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/07 00:12:22, falken wrote: > thanks for the test, lgtm > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_process_manager.cc (right): > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_process_manager.cc:226: > SiteInstanceImpl::CreateForURL(browser_context_, script_url); > does it make sense to DCHECK_NE(REUSE_PENDING_OR_COMMITTED_SITE)? Or can we > DCHECK something more specific? > > I'm just wondering if we're sure that CreateForURL will create a new > SiteInstanceImpl that definitely doesn't have the REUSE flag set (i.e., it's not > going to return an existing SiteInstanceImpl). Reading the code comments and > impl, that indeed seems true, so we could DCHECK this assumption? > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > File content/browser/service_worker/service_worker_process_manager_unittest.cc > (right): > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_process_manager_unittest.cc:248: > // Create a process that is hosting a frame with URL |patter_|. > |pattern_| > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_process_manager_unittest.cc:294: > // Create a process that is hosting a frame with URL |patter_|. > ditto > > https://codereview.chromium.org/2925623004/diff/20001/content/browser/service... > content/browser/service_worker/service_worker_process_manager_unittest.cc:315: > // An new process should be allocated to the worker. > nit: A new Since these are nits, I've hit CQ button so that this can make it in today's canary to see if it fixes the problem. The nits can be done in a followup, thanks.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496796503688600,
"parent_rev": "abde9ad7a7b06f7b50518fb8ea1caee9baaf2608", "commit_rev":
"47dc328bb08ac9c66848eeaf68fc8e96a39975cb"}
Message was sent while issue was closed.
Description was changed from ========== Only use SiteInstance process reuse for ServiceWorker with no failures This CL ensures we will only attempt to use the SiteInstance process reuse mechanism when creating a SiteInstance for a ServiceWorker when the ServiceWorker has not failed to start. Otherwise, we will try to start it in a brand new process. BUG=728030 ========== to ========== Only use SiteInstance process reuse for ServiceWorker with no failures This CL ensures we will only attempt to use the SiteInstance process reuse mechanism when creating a SiteInstance for a ServiceWorker when the ServiceWorker has not failed to start. Otherwise, we will try to start it in a brand new process. BUG=728030 Review-Url: https://codereview.chromium.org/2925623004 Cr-Commit-Position: refs/heads/master@{#477493} Committed: https://chromium.googlesource.com/chromium/src/+/47dc328bb08ac9c66848eeaf68fc... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/47dc328bb08ac9c66848eeaf68fc...
Message was sent while issue was closed.
On 2017/06/07 00:49:21, jam wrote: > Since these are nits, I've hit CQ button so that this can make it in today's > canary to see if it fixes the problem. The nits can be done in a followup, > thanks. Sure, created https://chromium-review.googlesource.com/c/526893/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
