Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2569963002:
MemoryCoordinator checks if ServiceWorker exists on the suspending process (Closed)
|
Created:
4 years ago by shimazu Modified:
4 years ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, serviceworker-reviews, creis+watch_chromium.org, shimazu+serviceworker_chromium.org, nasko+codewatch_chromium.org, jam, kinuko+serviceworker, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+watch, tzik, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMemoryCoordinator checks if ServiceWorker exists on the suspending process
If the suspending process has a worker thread, currently the worker won't
respond with events any more. This patch is to keep the process running which
has workers.
BUG=671084
Committed: https://crrev.com/14169abdc6f1d31468cb1de6212ff256c89b92b2
Cr-Commit-Position: refs/heads/master@{#439399}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added a browser test #Patch Set 3 : Added DCHECK and moved a comment #
Total comments: 2
Patch Set 4 : Hide the tab #
Total comments: 5
Patch Set 5 : Updated several comments #
Total comments: 2
Patch Set 6 : Updated the comment #
Total comments: 8
Patch Set 7 : Added comments and clarified the auto types #
Total comments: 4
Patch Set 8 : Descriptive -> imperative #Messages
Total messages: 45 (22 generated)
The CQ bit was checked by shimazu@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...
shimazu@chromium.org changed reviewers: + bashi@chromium.org
ptal
content/memory lgtm. Thanks for fixing this! BTW, is there a way to test this change (Maybe a browser test)?
horo@chromium.org changed reviewers: + horo@chromium.org
https://codereview.chromium.org/2569963002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2569963002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_impl.cc:1374: return service_worker_ref_count_ + shared_worker_ref_count_; DCHECK_CURRENTLY_ON(BrowserThread::UI);
falken@chromium.org changed reviewers: + falken@chromium.org
drive by https://codereview.chromium.org/2569963002/diff/1/content/public/browser/rend... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2569963002/diff/1/content/public/browser/rend... content/public/browser/render_process_host.h:308: // to the Worker in this renderer process has changed. This comment only refers to Increment/DecrementSharedWorkerRefCount. Please move the comment to above those functions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for your comments. I've added a browser tests and reflected reviews. PTAL again. https://codereview.chromium.org/2569963002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2569963002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_impl.cc:1374: return service_worker_ref_count_ + shared_worker_ref_count_; On 2016/12/13 06:29:33, horo (ooo Dec.14 - Jan.4) wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI); Done. https://codereview.chromium.org/2569963002/diff/1/content/public/browser/rend... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2569963002/diff/1/content/public/browser/rend... content/public/browser/render_process_host.h:308: // to the Worker in this renderer process has changed. On 2016/12/13 06:32:29, falken wrote: > This comment only refers to Increment/DecrementSharedWorkerRefCount. Please move > the comment to above those functions. Done.
still lgtm on my side.
The CQ bit was checked by shimazu@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nhiroki@chromium.org changed reviewers: + nhiroki@chromium.org
https://codereview.chromium.org/2569963002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2569963002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:2988: EXPECT_EQ(1u, rph->GetWorkerRefCount()); Can you check rph->IsProcessBackgrounded() before CanSuspendRenderer()? // A process should be backgrounded to ensure the worker reference count takes effect in CanSuspendRenderer(). EXPECT_TRUE(rph->IsProcessBackgrounded);
The CQ bit was checked by shimazu@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...
shimazu@chromium.org changed reviewers: + avi@chromium.org
Updated the browsertest. ptal again. avi@chromium.org: Could you review changes in (mock_)render_process_host? https://codereview.chromium.org/2569963002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2569963002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:2988: EXPECT_EQ(1u, rph->GetWorkerRefCount()); On 2016/12/13 10:24:29, nhiroki (OOO Dec 14 16) wrote: > Can you check rph->IsProcessBackgrounded() before CanSuspendRenderer()? > > // A process should be backgrounded to ensure the worker reference count takes > effect in CanSuspendRenderer(). > EXPECT_TRUE(rph->IsProcessBackgrounded); Thanks, I forgot to set rph to background. Added.
https://codereview.chromium.org/2569963002/diff/60001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2569963002/diff/60001/content/public/browser/... content/public/browser/render_process_host.h:309: virtual void DecrementServiceWorkerRefCount() = 0; Extra blank line after these lines. Please add a comment for what these functions do. https://codereview.chromium.org/2569963002/diff/60001/content/public/browser/... content/public/browser/render_process_host.h:311: // to the Worker in this renderer process has changed. This sentence is hard to follow. Can you clarify?
https://codereview.chromium.org/2569963002/diff/60001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2569963002/diff/60001/content/public/browser/... content/public/browser/render_process_host.h:311: // to the Worker in this renderer process has changed. On 2016/12/14 06:41:07, Avi wrote: > This sentence is hard to follow. Can you clarify? Yea this is complex but not from this patch. I meant to fix these the last time I understood them. The shared worker ref count in RPH is really more like a boolean flag tracking whether any other renderer process is connected to a shared worker in this renderer process. If there was none, and now there is at least one, Increment is called. If there was at least one, and now there is none Decrement is called. However Increment is *also* called when starting a new shared worker, so the process that houses it does not die in the middle of creation. So I suggest: "The shared worker ref count is nonzero if any other process is connected to a shared worker in this process, or a new shared worker is being created in this process. IncrementSharedWorkerRefCount is called in two cases: - there was no external renderer connected to a shared worker in this process, and now there is at least one - a new worker is being created in this process." DecrementSharedWorkerRefCount is called in two cases: - there was an external renderer connected to a shared worker in this process, and now there is none - a new worker finished being created in this process."
Thanks for your comments! Updated. https://codereview.chromium.org/2569963002/diff/60001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2569963002/diff/60001/content/public/browser/... content/public/browser/render_process_host.h:309: virtual void DecrementServiceWorkerRefCount() = 0; On 2016/12/14 06:41:07, Avi wrote: > Extra blank line after these lines. > > Please add a comment for what these functions do. Done. https://codereview.chromium.org/2569963002/diff/60001/content/public/browser/... content/public/browser/render_process_host.h:311: // to the Worker in this renderer process has changed. On 2016/12/14 06:58:51, falken wrote: > On 2016/12/14 06:41:07, Avi wrote: > > This sentence is hard to follow. Can you clarify? > > Yea this is complex but not from this patch. I meant to fix these the last time > I understood them. The shared worker ref count in RPH is really more like a > boolean flag tracking whether any other renderer process is connected to a > shared worker in this renderer process. If there was none, and now there is at > least one, Increment is called. If there was at least one, and now there is none > Decrement is called. However Increment is *also* called when starting a new > shared worker, so the process that houses it does not die in the middle of > creation. So I suggest: > > "The shared worker ref count is nonzero if any other process is connected to a > shared worker in this process, or a new shared worker is being created in this > process. IncrementSharedWorkerRefCount is called in two cases: > - there was no external renderer connected to a shared worker in this process, > and now there is at least one > - a new worker is being created in this process." > DecrementSharedWorkerRefCount is called in two cases: > - there was an external renderer connected to a shared worker in this process, > and now there is none > - a new worker finished being created in this process." Updated the comment, thanks!
https://codereview.chromium.org/2569963002/diff/80001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2569963002/diff/80001/content/public/browser/... content/public/browser/render_process_host.h:307: // Returns the total number of shared worker and service workers. nit: This isn't really the total number of workers, it's the ref count which in SharedWorker case is quite different (indeed some of us got confused about that). I guess "the sum of the shared worker and service worker ref counts"
https://codereview.chromium.org/2569963002/diff/80001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2569963002/diff/80001/content/public/browser/... content/public/browser/render_process_host.h:307: // Returns the total number of shared worker and service workers. On 2016/12/14 08:21:15, falken wrote: > nit: This isn't really the total number of workers, it's the ref count which in > SharedWorker case is quite different (indeed some of us got confused about > that). I guess "the sum of the shared worker and service worker ref counts" Thanks, updated.
lgtm
falken@, nhiroki@: can I commit this patch as is?
lgtm with a nit https://codereview.chromium.org/2569963002/diff/100001/content/public/browser... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2569963002/diff/100001/content/public/browser... content/public/browser/render_process_host.h:311: // worker ref count is incremented when the worker is trying to look for or "trying to look for" sounds a bit confusing. Simply "when this process is allocated to the worker," would be OK.
https://codereview.chromium.org/2569963002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2569963002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_browsertest.cc:2971: #if !defined(OS_MACOSX) Please add a comment about what this test tests and why OS_MACOSX is excluded. https://codereview.chromium.org/2569963002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_browsertest.cc:2977: auto* memory_coordinator = MemoryCoordinator::GetInstance(); nit: personally I prefer MemoryCoordinator* here, it's not an unwieldy type https://codereview.chromium.org/2569963002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_browsertest.cc:2987: auto* rph = RenderProcessHost::FromID(render_process_id); same here just RenderProcessHost*
The CQ bit was checked by shimazu@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...
https://codereview.chromium.org/2569963002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2569963002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_browsertest.cc:2971: #if !defined(OS_MACOSX) On 2016/12/15 05:16:38, falken wrote: > Please add a comment about what this test tests and why OS_MACOSX is excluded. Done. https://codereview.chromium.org/2569963002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_browsertest.cc:2977: auto* memory_coordinator = MemoryCoordinator::GetInstance(); On 2016/12/15 05:16:38, falken wrote: > nit: personally I prefer MemoryCoordinator* here, it's not an unwieldy type Done. https://codereview.chromium.org/2569963002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_browsertest.cc:2987: auto* rph = RenderProcessHost::FromID(render_process_id); On 2016/12/15 05:16:38, falken wrote: > same here just RenderProcessHost* Done. https://codereview.chromium.org/2569963002/diff/100001/content/public/browser... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2569963002/diff/100001/content/public/browser... content/public/browser/render_process_host.h:311: // worker ref count is incremented when the worker is trying to look for or On 2016/12/15 05:16:04, nhiroki (OOO Dec 16) wrote: > "trying to look for" sounds a bit confusing. Simply "when this process is > allocated to the worker," would be OK. Done.
lgtm https://codereview.chromium.org/2569963002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2569963002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_browsertest.cc:2985: // Ensures only one process host exists. nit: implementation comments are typically imperative not descriptive: "Ensure" not "Ensures", "Check" not "Checks" etc https://codereview.chromium.org/2569963002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_browsertest.cc:2989: // Checks the number of workers. nit: ditto
https://codereview.chromium.org/2569963002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2569963002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_browsertest.cc:2985: // Ensures only one process host exists. On 2016/12/19 01:33:19, falken wrote: > nit: implementation comments are typically imperative not descriptive: "Ensure" > not "Ensures", "Check" not "Checks" etc Thanks, done. https://codereview.chromium.org/2569963002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_browsertest.cc:2989: // Checks the number of workers. On 2016/12/19 01:33:19, falken wrote: > nit: ditto Done.
The CQ bit was checked by shimazu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org, avi@chromium.org, nhiroki@chromium.org, falken@chromium.org Link to the patchset: https://codereview.chromium.org/2569963002/#ps140001 (title: "Descriptive -> imperative")
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": 140001, "attempt_start_ts": 1482113103912560,
"parent_rev": "f7accb389d0e6612eab82bc15b7f6c46c591ac34", "commit_rev":
"e80bc594c05f29f0c7b31f64b502e20b741efd6f"}
Message was sent while issue was closed.
Description was changed from ========== MemoryCoordinator checks if ServiceWorker exists on the suspending process If the suspending process has a worker thread, currently the worker won't respond with events any more. This patch is to keep the process running which has workers. BUG=671084 ========== to ========== MemoryCoordinator checks if ServiceWorker exists on the suspending process If the suspending process has a worker thread, currently the worker won't respond with events any more. This patch is to keep the process running which has workers. BUG=671084 Review-Url: https://codereview.chromium.org/2569963002 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== MemoryCoordinator checks if ServiceWorker exists on the suspending process If the suspending process has a worker thread, currently the worker won't respond with events any more. This patch is to keep the process running which has workers. BUG=671084 Review-Url: https://codereview.chromium.org/2569963002 ========== to ========== MemoryCoordinator checks if ServiceWorker exists on the suspending process If the suspending process has a worker thread, currently the worker won't respond with events any more. This patch is to keep the process running which has workers. BUG=671084 Committed: https://crrev.com/14169abdc6f1d31468cb1de6212ff256c89b92b2 Cr-Commit-Position: refs/heads/master@{#439399} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/14169abdc6f1d31468cb1de6212ff256c89b92b2 Cr-Commit-Position: refs/heads/master@{#439399} |
