Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2578023002:
ServiceWorker: Stop don't send a message before connection established (Closed)
|
Created:
4 years ago by shimazu Modified:
3 years, 11 months ago CC:
chromium-reviews, michaeln, mlamouri+watch-content_chromium.org, tzik, jsbell+serviceworker_chromium.org, serviceworker-reviews, jam, kinuko+serviceworker, horo+watch_chromium.org, darin-cc_chromium.org, kinuko+watch, shimazu+serviceworker_chromium.org, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Stop don't send a message before connection established
Currently a callback passed on the second time is discarded when StopWorker
message is sent twice. This patch fixes it by not calling EWInstance::Stop twice
and not sending the StopWorker message before StartWorker message.
BUG=674443
TEST= ./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Master 'http/tests/serviceworker/chromium/resolve-after-window-close.html' --no-retry-failures --order=random --iterations=100 --exit-after-n-crashes-or-timeouts=1
Review-Url: https://codereview.chromium.org/2578023002
Cr-Commit-Position: refs/heads/master@{#441878}
Committed: https://chromium.googlesource.com/chromium/src/+/840051ec723d7dcacc54b81b8c77957c8ce8dad3
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed more #Patch Set 3 : Added DCHECKs #
Total comments: 16
Patch Set 4 : Addressed comments from nhiroki@ #
Total comments: 6
Patch Set 5 : Addressed comments from nhiroki@ #
Total comments: 8
Patch Set 6 : Don't call Stop twice and simplify the condition #Patch Set 7 : Fixed wrong DCHECK #
Total comments: 10
Patch Set 8 : comment from falken@ #
Total comments: 6
Patch Set 9 : Updated comments #
Total comments: 4
Patch Set 10 : Rebase #Patch Set 11 : Reset starting_phase when releasing the process #
Total comments: 4
Messages
Total messages: 67 (40 generated)
shimazu@chromium.org changed reviewers: + nhiroki@chromium.org
ptal
https://codereview.chromium.org/2578023002/diff/1/content/renderer/service_wo... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (left): https://codereview.chromium.org/2578023002/diff/1/content/renderer/service_wo... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:64: DCHECK(embedded_worker_id_); Instead of removing this, can you move this after line 67 in order to confirm "embedded_worker_id_.value()" is valid for RecordStopWorkerTimer? https://codereview.chromium.org/2578023002/diff/1/content/renderer/service_wo... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2578023002/diff/1/content/renderer/service_wo... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:46: "EmbeddedWorkerInstanceClientImpl::StartWorker"); DCHECK(!embedded_worker_id_);
Description was changed from ========== ServiceWorker: Removed an extra DCHECK StopWorker had a DCHECK to check if it's not called before StartWorker though it is possbile. This patch removes the DCHECK. BUG=N/A ========== to ========== ServiceWorker: Stop don't send a message before connection established Currently a callback passed on the second time is discarded when StopWorker is sent twice. Also this patch fixes the behavior when the StopWorker finishes on the renderer process and the callback is called while AllocateWorkerProcess invoked by StartWorker before StopWorker is still running on the UI thread. BUG=674443 TEST= ./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Master 'http/tests/serviceworker/chromium/resolve-after-window-close.html' --no-retry-failures --order=random --iterations=100 --exit-after-n-crashes-or-timeouts=1 ==========
Description was changed from ========== ServiceWorker: Stop don't send a message before connection established Currently a callback passed on the second time is discarded when StopWorker is sent twice. Also this patch fixes the behavior when the StopWorker finishes on the renderer process and the callback is called while AllocateWorkerProcess invoked by StartWorker before StopWorker is still running on the UI thread. BUG=674443 TEST= ./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Master 'http/tests/serviceworker/chromium/resolve-after-window-close.html' --no-retry-failures --order=random --iterations=100 --exit-after-n-crashes-or-timeouts=1 ========== to ========== ServiceWorker: Stop don't send a message before connection established Currently a callback passed on the second time is discarded when StopWorker is sent twice. This patch fixes to call all received callbacks when StopWorker finishes. Also this patch fixes the behavior when the StopWorker finishes on the renderer process and the callback is called while AllocateWorkerProcess invoked by StartWorker before StopWorker is still running on the UI thread. BUG=674443 TEST= ./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Master 'http/tests/serviceworker/chromium/resolve-after-window-close.html' --no-retry-failures --order=random --iterations=100 --exit-after-n-crashes-or-timeouts=1 ==========
Thanks for your comments! I've found other corner cases, so added more fixes. PTAL again! https://codereview.chromium.org/2578023002/diff/1/content/renderer/service_wo... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (left): https://codereview.chromium.org/2578023002/diff/1/content/renderer/service_wo... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:64: DCHECK(embedded_worker_id_); On 2016/12/15 05:43:26, nhiroki (OOO Dec 16) wrote: > Instead of removing this, can you move this after line 67 in order to confirm > "embedded_worker_id_.value()" is valid for RecordStopWorkerTimer? Done. https://codereview.chromium.org/2578023002/diff/1/content/renderer/service_wo... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2578023002/diff/1/content/renderer/service_wo... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:46: "EmbeddedWorkerInstanceClientImpl::StartWorker"); On 2016/12/15 05:43:26, nhiroki (OOO Dec 16) wrote: > DCHECK(!embedded_worker_id_); Done.
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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:508: SERVICE_WORKER_OK, Does StopWorker() always succeed? If so, do we still need to record the result? https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:513: case NOT_STARTING: Is this case really possible? https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:516: return SERVICE_WORKER_ERROR_PROCESS_NOT_FOUND; Why do we handle this case as an error? I wonder if this can easily happen when a web developer consecutively clicks a start/stop button on the devtools. https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:519: return SERVICE_WORKER_ERROR_IPC_FAILED; ditto. https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:520: default: I'd prefer to avoid 'default' because this could silently break when we add a new phase during starting a worker. https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:33: for (auto& callback : stop_callbacks_) Generally, it would be preferable to swap a callback list with an empty list before invoking each callback like [1] because one of the callbacks could mutate the callback list during the iteration (I think this case is safe though) [1] https://chromium.googlesource.com/chromium/src/+/master/content/browser/servi... https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:34: callback.Run(); Do we need to call EmbeddedWorkerRegistry::OnWorkerStopped() multiple times on the same worker?
nhiroki@chromium.org changed reviewers: + falken@chromium.org
+falken@ because I'll be ooo tomorrow...
I'd like to take a closer look after nhiroki's comments and the trybot failures are addressed.
Description was changed from ========== ServiceWorker: Stop don't send a message before connection established Currently a callback passed on the second time is discarded when StopWorker is sent twice. This patch fixes to call all received callbacks when StopWorker finishes. Also this patch fixes the behavior when the StopWorker finishes on the renderer process and the callback is called while AllocateWorkerProcess invoked by StartWorker before StopWorker is still running on the UI thread. BUG=674443 TEST= ./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Master 'http/tests/serviceworker/chromium/resolve-after-window-close.html' --no-retry-failures --order=random --iterations=100 --exit-after-n-crashes-or-timeouts=1 ========== to ========== ServiceWorker: Stop don't send a message before connection established Currently a callback passed on the second time is discarded when StopWorker is sent twice. This patch fixes to call all received callbacks when StopWorker finishes. Also this patch fixes the behavior when the StopWorker finishes on the renderer process and the callback is called while AllocateWorkerProcess invoked by StartWorker before StopWorker is still running on the UI thread. BUG=674443 TEST= ./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Master 'http/tests/serviceworker/chromium/resolve-after-window-close.html' --no-retry-failures --order=random --iterations=100 --exit-after-n-crashes-or-timeouts=1 ==========
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...
Thanks for your comments. I've forgotten to fix the test for the buildbot failures. PTAL. https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:508: SERVICE_WORKER_OK, On 2016/12/15 09:53:15, nhiroki wrote: > Does StopWorker() always succeed? If so, do we still need to record the result? Oops, I forgot to change that. Fixed it. https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:513: case NOT_STARTING: On 2016/12/15 09:53:15, nhiroki wrote: > Is this case really possible? No actually. Stop should be called when status_ == STARTING || RUNNING, and that means Stop isn't called before Start. Added NOTREACHED(). https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:516: return SERVICE_WORKER_ERROR_PROCESS_NOT_FOUND; On 2016/12/15 09:53:15, nhiroki wrote: > Why do we handle this case as an error? I wonder if this can easily happen when > a web developer consecutively clicks a start/stop button on the devtools. This is because it's the same with legacy IPC. https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:519: return SERVICE_WORKER_ERROR_IPC_FAILED; On 2016/12/15 09:53:15, nhiroki wrote: > ditto. Acknowledged. https://codereview.chromium.org/2578023002/diff/40001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:520: default: On 2016/12/15 09:53:15, nhiroki wrote: > I'd prefer to avoid 'default' because this could silently break when we add a > new phase during starting a worker. Done. https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:33: for (auto& callback : stop_callbacks_) On 2016/12/15 09:53:15, nhiroki wrote: > Generally, it would be preferable to swap a callback list with an empty list > before invoking each callback like [1] because one of the callbacks could mutate > the callback list during the iteration (I think this case is safe though) > > [1] > https://chromium.googlesource.com/chromium/src/+/master/content/browser/servi... Done. https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:34: callback.Run(); On 2016/12/15 09:53:15, nhiroki wrote: > Do we need to call EmbeddedWorkerRegistry::OnWorkerStopped() multiple times on > the same worker? No we don't in terms of the chromium IPC, but mojo IPC requires all callbacks should be called.
https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:34: callback.Run(); On 2016/12/19 08:25:12, shimazu wrote: > On 2016/12/15 09:53:15, nhiroki wrote: > > Do we need to call EmbeddedWorkerRegistry::OnWorkerStopped() multiple times on > > the same worker? > > No we don't in terms of the chromium IPC, but mojo IPC requires all callbacks > should be called. What happens when EmbeddedWorkerInstance::Start() is called on the browser process during several mojo-callbacks are invoked here? I'm concerned that 2nd or subsequent mojo-callbacks could wrongly cancel the start worker attempt. https://codereview.chromium.org/2578023002/diff/60001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/60001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:527: default: Can you use "STARTING_PHASE_MAX_VALUE:" instead of "default"? https://codereview.chromium.org/2578023002/diff/60001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:556: return status; nit: We could merge line 553-556 into 538-541 and then move them out of if(IsMojoForServiceWorkerEnabled()). https://codereview.chromium.org/2578023002/diff/60001/content/renderer/servic... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2578023002/diff/60001/content/renderer/servic... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:73: DCHECK(embedded_worker_id_); nit: For readability, it'd be better to place this DCHECK at the beginning of this function or immediately before it's used.
Patchset #5 (id:80001) has been deleted
Thanks, updated. https://codereview.chromium.org/2578023002/diff/60001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/60001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:527: default: On 2016/12/19 09:21:55, nhiroki wrote: > Can you use "STARTING_PHASE_MAX_VALUE:" instead of "default"? Done. https://codereview.chromium.org/2578023002/diff/60001/content/browser/service... content/browser/service_worker/embedded_worker_instance.cc:556: return status; On 2016/12/19 09:21:55, nhiroki wrote: > nit: We could merge line 553-556 into 538-541 and then move them out of > if(IsMojoForServiceWorkerEnabled()). Done. https://codereview.chromium.org/2578023002/diff/60001/content/renderer/servic... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2578023002/diff/60001/content/renderer/servic... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:73: DCHECK(embedded_worker_id_); On 2016/12/19 09:21:55, nhiroki wrote: > nit: For readability, it'd be better to place this DCHECK at the beginning of > this function or immediately before it's used. Done.
I'll be ooo until next week, so let me leave this to falken... On 2016/12/19 09:21:55, nhiroki wrote: > https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... > File content/renderer/service_worker/embedded_worker_instance_client_impl.cc > (right): > > https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... > content/renderer/service_worker/embedded_worker_instance_client_impl.cc:34: > callback.Run(); > On 2016/12/19 08:25:12, shimazu wrote: > > On 2016/12/15 09:53:15, nhiroki wrote: > > > Do we need to call EmbeddedWorkerRegistry::OnWorkerStopped() multiple times > on > > > the same worker? > > > > No we don't in terms of the chromium IPC, but mojo IPC requires all callbacks > > should be called. > > What happens when EmbeddedWorkerInstance::Start() is called on the browser > process during several mojo-callbacks are invoked here? I'm concerned that 2nd > or subsequent mojo-callbacks could wrongly cancel the start worker attempt. ^^^Just in case you missed this
Incrementally this change seems OK but I'm getting wary of the complexity added lately to handle starting/stopping interleaving. Can we simplify this somehow? Here are some recent changes: [1] Try to handle when stop is sent twice (https://chromiumcodereview.appspot.com/2430403005) This happens when browser-side doesn’t notice that Stop was already sent, since it checks |ServiceWorkerVersion::stop_callbacks_| rather than |EmbeddedWorkerInstance::status|. This drops the callback. [2] Try to handle when stop is sent after we already stopped (https://codereview.chromium.org/2498573002) This happens in the same as above, but the Stop raches us after we already Stopped. This also drops the callback. [3] Try to handle when stop is sent before start (https://codereview.chromium.org/2496073002/) This happens when Stop is sent after StartWorker() is called but between ProcessAllocated() and SendStartWorker(). Currently we have code that tries to avoid sending Stop when Stop is already sent (ServiceWorkerVersion::StopWorker()) yet also code that tries to handle multiple Stops (EmbeddedWorkerClientImpl::StopWorker()) and now this code that will try to avoid sending Stop at particular times in the startup sequence. It feels like somehow we should centralize this code browser-side so we only send Stop after a Start Worker is sent and don’t need complex handling of edge cases renderer-side? https://codereview.chromium.org/2578023002/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:515: status = SERVICE_WORKER_ERROR_IPC_FAILED; Hm I don't think we really need to log these as errors. The point of SendStopWorker status was to track when sending an IPC fails. But this Mojo-based code is really just avoiding sending a Stop before a Start was sent. IPC_FAILED in particular feels misleading. Basically I think SendStopWorker shouldn't be logged in the mojo case, WDYT? https://codereview.chromium.org/2578023002/diff/100001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2578023002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:68: // StopWorker is possible to be called before StartWorker(). Is this still possible? https://codereview.chromium.org/2578023002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:73: // StopWorker is possible to be called twice. Can the comment expand on how?
https://codereview.chromium.org/2578023002/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:498: << static_cast<int>(status_); Also wouldn't this DCHECK fail if Stop() is called twice?
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...
Patchset #7 (id:140001) has been deleted
Patchset #6 (id:120001) has been deleted
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #6 (id:160001) has been deleted
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...
Description was changed from ========== ServiceWorker: Stop don't send a message before connection established Currently a callback passed on the second time is discarded when StopWorker is sent twice. This patch fixes to call all received callbacks when StopWorker finishes. Also this patch fixes the behavior when the StopWorker finishes on the renderer process and the callback is called while AllocateWorkerProcess invoked by StartWorker before StopWorker is still running on the UI thread. BUG=674443 TEST= ./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Master 'http/tests/serviceworker/chromium/resolve-after-window-close.html' --no-retry-failures --order=random --iterations=100 --exit-after-n-crashes-or-timeouts=1 ========== to ========== ServiceWorker: Stop don't send a message before connection established Currently a callback passed on the second time is discarded when StopWorker message is sent twice. This patch fixes it by not calling EWInstance::Stop twice and not sending the StopWorker message before StartWorker message. BUG=674443 TEST= ./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Master 'http/tests/serviceworker/chromium/resolve-after-window-close.html' --no-retry-failures --order=random --iterations=100 --exit-after-n-crashes-or-timeouts=1 ==========
The CQ bit was unchecked by shimazu@chromium.org
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...
Thanks for your comments. Actually I was a bit confused by the state management... I've updated the patch not to call EWInstance::Stop twice and not to send the IPC before sending StartWorker message on the browser process. I hope this simplifies the situation. PTAL again. https://codereview.chromium.org/2578023002/diff/100001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:498: << static_cast<int>(status_); On 2016/12/20 04:47:12, falken wrote: > Also wouldn't this DCHECK fail if Stop() is called twice? Yes, I think so. https://codereview.chromium.org/2578023002/diff/100001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:515: status = SERVICE_WORKER_ERROR_IPC_FAILED; On 2016/12/20 04:41:31, falken wrote: > Hm I don't think we really need to log these as errors. The point of > SendStopWorker status was to track when sending an IPC fails. But this > Mojo-based code is really just avoiding sending a Stop before a Start was sent. > IPC_FAILED in particular feels misleading. > > Basically I think SendStopWorker shouldn't be logged in the mojo case, WDYT? Removing the uma seems good. Removed. https://codereview.chromium.org/2578023002/diff/100001/content/renderer/servi... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2578023002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:68: // StopWorker is possible to be called before StartWorker(). On 2016/12/20 04:41:31, falken wrote: > Is this still possible? No. This patch actually fixes this case. Updated. https://codereview.chromium.org/2578023002/diff/100001/content/renderer/servi... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:73: // StopWorker is possible to be called twice. On 2016/12/20 04:41:31, falken wrote: > Can the comment expand on how? This also shouldn't happen. Fixed on the browser side.
This is looking good, thanks for taking up the suggestion! Also I'm thinking about tests. Could it be worth it to add a test like EmbeddedWorkerInstance::Stop is called then ServiceWorkerVersion::StopWorker is called, and ensure that stop isn't sent twice (it'd DCHECK in that case)? https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:507: // been established yet. More than "connection established", I think this is ensuring we don't send Stop if Start hasn't been sent yet. I wonder if this would be more readable as a helper function. if (status_ == EmbeddedWorkerStatus::STARTING && !HasSentStartWorker(starting_phase())) { OnDetached(); return ERROR; } status = SERVICE_WORKER_OK; client_->StopWorker(); https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:514: return SERVICE_WORKER_ERROR_FAILED; It doesn't seem right to return error here and have the callback to ServiceWorkerVersion::StopWorker be invoked with an error. Nothing bad happened: we released the process and the worker is stopped (it never started). However I see if we return OK here then StopWorker won't invoke its callback. Really in the Mojo-world it seems Stop() isn't returning a success/failure status but a boolean about whether the Stop will happen asynchronously or not. Actually I think the code would be more stable if this path still runs through the motions of observer.OnStopping() followed by observer.OnStopped() just like the normal case. It should be abstracted away from the caller whether the Stop really was sent or not. WDYT? Should it be a TODO for a later patch? I think the code would look something like: status_ = STOPPING; for (auto& observer : listener_list_) observer.OnStopping(); if (!was_stop_sent) RunSoon(&OnStopped); // OnStopped will call ReleaseProcess() return OK; "Detach" is kind of a messy hack BTW. It was originally added to try to workaround a bug where workers got stuck in STOPPING status. It turned out the bug was a deadlocked renderer <https://bugs.chromium.org/p/chromium/issues/detail?id=509904>. https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:525: OnDetached(); nit: remove OnDetached(). we shouldn't handle the impossible condition per style guide https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:535: UMA_HISTOGRAM_ENUMERATION("ServiceWorker.SendStopWorker.Status", status, We should update histograms.xml that it's only used in the disable-mojo case. https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:500: running_status() == EmbeddedWorkerStatus::RUNNING) { How about turning lines 494 and 499-500 into a switch statement?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Updated the patch. PTAL again:) https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... File content/renderer/service_worker/embedded_worker_instance_client_impl.cc (right): https://codereview.chromium.org/2578023002/diff/40001/content/renderer/servic... content/renderer/service_worker/embedded_worker_instance_client_impl.cc:34: callback.Run(); On 2016/12/19 09:21:55, nhiroki wrote: > On 2016/12/19 08:25:12, shimazu wrote: > > On 2016/12/15 09:53:15, nhiroki wrote: > > > Do we need to call EmbeddedWorkerRegistry::OnWorkerStopped() multiple times > on > > > the same worker? > > > > No we don't in terms of the chromium IPC, but mojo IPC requires all callbacks > > should be called. > > What happens when EmbeddedWorkerInstance::Start() is called on the browser > process during several mojo-callbacks are invoked here? I'm concerned that 2nd > or subsequent mojo-callbacks could wrongly cancel the start worker attempt. Sorry for missing the comment. It's an unwelcome situation. I've updated this patch not to invoke Stop twice. https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:507: // been established yet. On 2016/12/20 09:12:07, falken (ooo happy new year) wrote: > More than "connection established", I think this is ensuring we don't send Stop > if Start hasn't been sent yet. > > I wonder if this would be more readable as a helper function. > > if (status_ == EmbeddedWorkerStatus::STARTING && > !HasSentStartWorker(starting_phase())) { > OnDetached(); > return ERROR; > } > status = SERVICE_WORKER_OK; > client_->StopWorker(); Done. https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:514: return SERVICE_WORKER_ERROR_FAILED; On 2016/12/20 09:12:06, falken (ooo happy new year) wrote: > It doesn't seem right to return error here and have the callback to > ServiceWorkerVersion::StopWorker be invoked with an error. Nothing bad happened: > we released the process and the worker is stopped (it never started). However I > see if we return OK here then StopWorker won't invoke its callback. Really in > the Mojo-world it seems Stop() isn't returning a success/failure status but a > boolean about whether the Stop will happen asynchronously or not. > > Actually I think the code would be more stable if this path still runs through > the motions of observer.OnStopping() followed by observer.OnStopped() just like > the normal case. It should be abstracted away from the caller whether the Stop > really was sent or not. > > WDYT? Should it be a TODO for a later patch? > > I think the code would look something like: > > status_ = STOPPING; > for (auto& observer : listener_list_) > observer.OnStopping(); > if (!was_stop_sent) > RunSoon(&OnStopped); // OnStopped will call ReleaseProcess() > return OK; > > "Detach" is kind of a messy hack BTW. It was originally added to try to > workaround a bug where workers got stuck in STOPPING status. It turned out the > bug was a deadlocked renderer > <https://bugs.chromium.org/p/chromium/issues/detail?id=509904>. > > That sounds good, but I'd like to create another patch for that after removing the legacy IPC path. https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:525: OnDetached(); On 2016/12/20 09:12:06, falken (ooo happy new year) wrote: > nit: remove OnDetached(). we shouldn't handle the impossible condition per style > guide Done. https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:535: UMA_HISTOGRAM_ENUMERATION("ServiceWorker.SendStopWorker.Status", status, On 2016/12/20 09:12:06, falken (ooo happy new year) wrote: > We should update histograms.xml that it's only used in the disable-mojo case. Done. https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/2578023002/diff/200001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:500: running_status() == EmbeddedWorkerStatus::RUNNING) { On 2016/12/20 09:12:07, falken (ooo happy new year) wrote: > How about turning lines 494 and 499-500 into a switch statement? Done.
Thanks, this is nice. lgtm. https://codereview.chromium.org/2578023002/diff/220001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:529: // removed. for clarity, "legacy IPC path" https://codereview.chromium.org/2578023002/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/2578023002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:497: if (!embedded_worker_->Stop()) { can you add a TODO here about how the return value from Stop doesn't necessarily mean failure and point to the comments in EmbeddedWorkerInstance::Stop. https://codereview.chromium.org/2578023002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2578023002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58948: + This is only recorded for the legacy IPC. Maybe for more clarity "This was previously recorded for both the IPC and Mojo service worker implementations, but as of Jan 2017, this is only recorded for the IPC implementation. This histogram will be removed after the IPC implementation is removed."
shimazu@chromium.org changed reviewers: + mpearson@chromium.org
mpearson@: could you check histogram.xml? https://codereview.chromium.org/2578023002/diff/220001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/220001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:529: // removed. On 2017/01/05 06:40:17, falken wrote: > for clarity, "legacy IPC path" Done. https://codereview.chromium.org/2578023002/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/2578023002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:497: if (!embedded_worker_->Stop()) { On 2017/01/05 06:40:17, falken wrote: > can you add a TODO here about how the return value from Stop doesn't necessarily > mean failure and point to the comments in EmbeddedWorkerInstance::Stop. Done. https://codereview.chromium.org/2578023002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2578023002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58948: + This is only recorded for the legacy IPC. On 2017/01/05 06:40:17, falken wrote: > Maybe for more clarity "This was previously recorded for both the IPC and Mojo > service worker implementations, but as of Jan 2017, this is only recorded for > the IPC implementation. This histogram will be removed after the IPC > implementation is removed." Done.
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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm https://codereview.chromium.org/2578023002/diff/240001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/240001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:527: // been sent. Should we also avoid sending the StopWorker message over the legacy IPC? https://codereview.chromium.org/2578023002/diff/240001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:898: thread_id_ = kInvalidEmbeddedWorkerThreadId; nit: We might want to reset |starting_phase_| here.
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/2578023002/diff/240001/content/browser/servic... File content/browser/service_worker/embedded_worker_instance.cc (right): https://codereview.chromium.org/2578023002/diff/240001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:527: // been sent. On 2017/01/05 08:32:57, nhiroki wrote: > Should we also avoid sending the StopWorker message over the legacy IPC? The legacy IPC fails and returns a status code when StartWorker message hasn't been sent. Checking the return code would be the same with HasSentStartWorker. https://codereview.chromium.org/2578023002/diff/240001/content/browser/servic... content/browser/service_worker/embedded_worker_instance.cc:898: thread_id_ = kInvalidEmbeddedWorkerThreadId; On 2017/01/05 08:32:57, nhiroki wrote: > nit: We might want to reset |starting_phase_| here. Sure. Updated. Actually it shouldn't cause a problem because starting_phase() can be read only during STARTING state and starting_phase will be set at the beginning of STARTING state, but I also think it's better to reset starting_phase here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2578023002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2578023002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:59367: + This was previously recorded for both the IPC and Mojo service worker A better practice would be to rename the current ServiceWorker.SendStopWorker.Status histogram to something like ServiceWorker.SendStopWorker.IPC.Status, marked the current histogram as obsolete in histograms.xml, and add the new histogram to the file. Is there a particular reason you want to maintain the same name?
https://codereview.chromium.org/2578023002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2578023002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:59367: + This was previously recorded for both the IPC and Mojo service worker On 2017/01/05 20:11:08, Mark P wrote: > A better practice would be to rename the current > ServiceWorker.SendStopWorker.Status histogram to something like > ServiceWorker.SendStopWorker.IPC.Status, marked the current histogram as > obsolete in histograms.xml, and add the new histogram to the file. > > Is there a particular reason you want to maintain the same name? The IPC implementation is already off by default (as of M56 in Beta) and will be removed soon. We've only been maintaining it in case some big disaster is found with the Mojo implementation that requires us to reflip the flag. Adding a new histogram IPC.Status would be wasteful since essentially there are zero users and we'll just remove it soon. I think it was a mistake for Mojo to use this histogram in the first place since there is no notion of "send an IPC message" but it's too late to fix that now (currently it's recording OK everytime). So I suggested just adding this comment to make clear what happened. If this comment still doesn't sound good, I think we can just go ahead and obsolete this histogram now and remove the logging from the code. If Mojo gets reverted we can add a new IPC status histogram.
I think Mark may be gone for the day. In the interest of moving this patch forward, I think we can keep the "always log OK" behavior for the Mojo case with a TODO that we'll remove the histogram when IPC is removed. We don't really need to touch histograms with this patch, since it'll just be removed soon.
histograms.xml lgtm --mark https://codereview.chromium.org/2578023002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2578023002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:59367: + This was previously recorded for both the IPC and Mojo service worker On 2017/01/06 02:14:54, falken wrote: > On 2017/01/05 20:11:08, Mark P wrote: > > A better practice would be to rename the current > > ServiceWorker.SendStopWorker.Status histogram to something like > > ServiceWorker.SendStopWorker.IPC.Status, marked the current histogram as > > obsolete in histograms.xml, and add the new histogram to the file. > > > > Is there a particular reason you want to maintain the same name? > > The IPC implementation is already off by default (as of M56 in Beta) and will be > removed soon. We've only been maintaining it in case some big disaster is found > with the Mojo implementation that requires us to reflip the flag. Adding a new > histogram IPC.Status would be wasteful since essentially there are zero users > and we'll just remove it soon. > > I think it was a mistake for Mojo to use this histogram in the first place since > there is no notion of "send an IPC message" but it's too late to fix that now > (currently it's recording OK everytime). So I suggested just adding this comment > to make clear what happened. Thanks for the explanation. In this case--it's effectively obsolete already--I think the comment suffices. > If this comment still doesn't sound good, I think we can just go ahead and > obsolete this histogram now and remove the logging from the code. If Mojo gets > reverted we can add a new IPC status histogram.
On 2017/01/06 04:52:43, Mark P wrote: > histograms.xml lgtm (This comment applies to patchset 11. I didn't look at the new version for patchset 12.) --mark > > --mark > > https://codereview.chromium.org/2578023002/diff/280001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2578023002/diff/280001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:59367: + This was previously recorded > for both the IPC and Mojo service worker > On 2017/01/06 02:14:54, falken wrote: > > On 2017/01/05 20:11:08, Mark P wrote: > > > A better practice would be to rename the current > > > ServiceWorker.SendStopWorker.Status histogram to something like > > > ServiceWorker.SendStopWorker.IPC.Status, marked the current histogram as > > > obsolete in histograms.xml, and add the new histogram to the file. > > > > > > Is there a particular reason you want to maintain the same name? > > > > The IPC implementation is already off by default (as of M56 in Beta) and will > be > > removed soon. We've only been maintaining it in case some big disaster is > found > > with the Mojo implementation that requires us to reflip the flag. Adding a new > > histogram IPC.Status would be wasteful since essentially there are zero users > > and we'll just remove it soon. > > > > I think it was a mistake for Mojo to use this histogram in the first place > since > > there is no notion of "send an IPC message" but it's too late to fix that now > > (currently it's recording OK everytime). So I suggested just adding this > comment > > to make clear what happened. > > Thanks for the explanation. In this case--it's effectively obsolete already--I > think the comment suffices. > > > If this comment still doesn't sound good, I think we can just go ahead and > > obsolete this histogram now and remove the logging from the code. If Mojo gets > > reverted we can add a new IPC status histogram.
Patchset #12 (id:300001) has been deleted
https://codereview.chromium.org/2578023002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2578023002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:59367: + This was previously recorded for both the IPC and Mojo service worker On 2017/01/06 04:52:43, Mark P wrote: > On 2017/01/06 02:14:54, falken wrote: > > On 2017/01/05 20:11:08, Mark P wrote: > > > A better practice would be to rename the current > > > ServiceWorker.SendStopWorker.Status histogram to something like > > > ServiceWorker.SendStopWorker.IPC.Status, marked the current histogram as > > > obsolete in histograms.xml, and add the new histogram to the file. > > > > > > Is there a particular reason you want to maintain the same name? > > > > The IPC implementation is already off by default (as of M56 in Beta) and will > be > > removed soon. We've only been maintaining it in case some big disaster is > found > > with the Mojo implementation that requires us to reflip the flag. Adding a new > > histogram IPC.Status would be wasteful since essentially there are zero users > > and we'll just remove it soon. > > > > I think it was a mistake for Mojo to use this histogram in the first place > since > > there is no notion of "send an IPC message" but it's too late to fix that now > > (currently it's recording OK everytime). So I suggested just adding this > comment > > to make clear what happened. > > Thanks for the explanation. In this case--it's effectively obsolete already--I > think the comment suffices. > > > If this comment still doesn't sound good, I think we can just go ahead and > > obsolete this histogram now and remove the logging from the code. If Mojo gets > > reverted we can add a new IPC status histogram. > Thanks for your comments. I'll commit as is (ps11) for now and mark it as obsolete when the legacy IPC path is removed.
The CQ bit was checked by shimazu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/2578023002/#ps280001 (title: "Reset starting_phase when releasing the process")
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": 280001, "attempt_start_ts": 1483678708516420,
"parent_rev": "5e285562f783c806c764b11fcc48d638843c0f64", "commit_rev":
"840051ec723d7dcacc54b81b8c77957c8ce8dad3"}
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Stop don't send a message before connection established Currently a callback passed on the second time is discarded when StopWorker message is sent twice. This patch fixes it by not calling EWInstance::Stop twice and not sending the StopWorker message before StartWorker message. BUG=674443 TEST= ./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Master 'http/tests/serviceworker/chromium/resolve-after-window-close.html' --no-retry-failures --order=random --iterations=100 --exit-after-n-crashes-or-timeouts=1 ========== to ========== ServiceWorker: Stop don't send a message before connection established Currently a callback passed on the second time is discarded when StopWorker message is sent twice. This patch fixes it by not calling EWInstance::Stop twice and not sending the StopWorker message before StartWorker message. BUG=674443 TEST= ./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Master 'http/tests/serviceworker/chromium/resolve-after-window-close.html' --no-retry-failures --order=random --iterations=100 --exit-after-n-crashes-or-timeouts=1 Review-Url: https://codereview.chromium.org/2578023002 Cr-Commit-Position: refs/heads/master@{#441878} Committed: https://chromium.googlesource.com/chromium/src/+/840051ec723d7dcacc54b81b8c77... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:280001) as https://chromium.googlesource.com/chromium/src/+/840051ec723d7dcacc54b81b8c77... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
