Revamp EmbeddedWorkerInstance::Start |
||
Issue descriptionThis is basically the same as issue 855952 but not about UMA. I found removing some of the IPCs is a bit tricky because of how EWI::Start works. Today's EmbeddedWorkerInstance::Start(callback) API is tricky. The caller has to use a combination of the callback and the OnStarted/OnStopped observer functions. The callback gets called on "almost done" success before OnStarted gets called, so it's not the reliable signal of success. Currently we ignore the callback on success. But! It gets called with failure even when startup didn't fail, and it sometimes doesn't get called even when startup fails. Here are some bizarre sequences: 1. An uncaught runtime error occurs on initial script evaluation. 2. The callback is called with ErrorScriptEvaluation. 3. OnStarted() is called. 1. Startup fails before script evaluation starts. 2. The callback is not called. 3. OnStopped() is called. The ideal API is EmbeddedWorkerInstance::Start(callback) where callback gets called every time. However it's a bit hard to get there immediately since we rely on OnStarted() and OnStopped() IPCs from the renderer. The impl also deviates from the spec. We currently don't dispatch events if a runtime error occurred while starting the worker, even though the worker thread remains running. The spec only cares about runtime errors for install/update. My rough plan: 1. Eliminate the EWI::Start() callback. It's a weird pseudo-step before OnStarted/OnStopped. We should still be able to preserve current behavior by adding a startup status to OnStarted() which indicates runtime error. 2. Change the Mojo method for Start to Start(callback) instead of relying on OnStarted/OnStopped. 3. Re-add the callback for EWI::Start(), backed by the Mojo method.
,
Jul 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6d71c629186c482fd08594c5ccb09f99ea9b8708 commit 6d71c629186c482fd08594c5ccb09f99ea9b8708 Author: Matt Falkenhagen <falken@chromium.org> Date: Fri Jul 06 09:48:43 2018 service worker: Change EmbeddedWorkerInstance::Start callback semantics. Run the callback when the Start IPC is sent, or if failure occurs before that. The previous behavior was confusing because it was sent on start "almost" finished, or when the start IPC failed. This CL limits the scope to just the start IPC. This will enable future patches to cleanup the OnScriptEvaluated IPC, namely by folding it into OnStarted IPC. This CL relaxes some EXPECTs in embedded_worker_test_helper.cc because some tests send the Start IPC and then immediately call Stop() on the worker. This means on the renderer-side, when the IPC is received the worker is not in STARTING status. No behavior change expected. Bug: 859912 Change-Id: Iea329d7bf20f14625949d2caf9ec92ad87b0862f Reviewed-on: https://chromium-review.googlesource.com/1126804 Commit-Queue: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Cr-Commit-Position: refs/heads/master@{#572928} [modify] https://crrev.com/6d71c629186c482fd08594c5ccb09f99ea9b8708/content/browser/service_worker/embedded_worker_instance.cc [modify] https://crrev.com/6d71c629186c482fd08594c5ccb09f99ea9b8708/content/browser/service_worker/embedded_worker_instance.h [modify] https://crrev.com/6d71c629186c482fd08594c5ccb09f99ea9b8708/content/browser/service_worker/embedded_worker_instance_unittest.cc [modify] https://crrev.com/6d71c629186c482fd08594c5ccb09f99ea9b8708/content/browser/service_worker/embedded_worker_test_helper.cc [modify] https://crrev.com/6d71c629186c482fd08594c5ccb09f99ea9b8708/content/browser/service_worker/service_worker_version.cc [modify] https://crrev.com/6d71c629186c482fd08594c5ccb09f99ea9b8708/content/browser/service_worker/service_worker_version.h
,
Jul 11
I ended up making the Start() callback mean "IPC was sent" per Kinuko's suggestion on the code review. |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Jul 4