Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
765323002:
ServiceWorker: Add support for .skipWaiting and controllerchange event(3/3) (Closed)
|
Created:
6 years ago by xiang Modified:
6 years ago CC:
blink-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, serviceworker-reviews, nhiroki, kinuko+serviceworker, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionServiceWorker: Add support for .skipWaiting and controllerchange event(3/3)
Add layout tests and cleanup FIXME.
(1/3): https://codereview.chromium.org/723923002/
(2/3): https://codereview.chromium.org/717353004/
(3/3): This
TBR=tkent@chromium.org
BUG=370742
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187226
Patch Set 1 #
Total comments: 8
Patch Set 2 : review update #
Total comments: 15
Patch Set 3 : remove chromium test, add no controlled page test #Patch Set 4 : remove no controlled page test #Patch Set 5 : repick the not controlled test #
Total comments: 1
Patch Set 6 : add new test #Messages
Total messages: 30 (10 generated)
xiang.long@intel.com changed reviewers: + falken@chromium.org, jsbell@chromium.org
PTAL, thanks!
This patch doesn't cleanup the FIXME. otherwise lgtm https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/skip-waiting-installed.html (right): https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/skip-waiting-installed.html:13: var promise1 = new Promise(function(resolve) { can you give a more descriptive name like "saw_message" https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/skip-waiting-installed.html:22: var promise2 = new Promise(function(resolve) { "saw_controlledchanged"? https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/skip-waiting-installed.html:62: .then(function() { remove frame https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/skip-waiting.html (right): https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/skip-waiting.html:12: var frame, frame_sw, oncontrollerchanged, worker; |worker| is unused
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
CL updated, I also added one test for document close during controllerchange handler. PTAL, thanks! https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/skip-waiting-installed.html (right): https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/skip-waiting-installed.html:13: var promise1 = new Promise(function(resolve) { On 2014/12/01 07:48:28, falken wrote: > can you give a more descriptive name like "saw_message" Done. https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/skip-waiting-installed.html:22: var promise2 = new Promise(function(resolve) { On 2014/12/01 07:48:28, falken wrote: > "saw_controlledchanged"? Done. https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/skip-waiting-installed.html:62: .then(function() { On 2014/12/01 07:48:28, falken wrote: > remove frame Done. https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... File LayoutTests/http/tests/serviceworker/skip-waiting.html (right): https://codereview.chromium.org/765323002/diff/1/LayoutTests/http/tests/servi... LayoutTests/http/tests/serviceworker/skip-waiting.html:12: var frame, frame_sw, oncontrollerchanged, worker; On 2014/12/01 07:48:28, falken wrote: > |worker| is unused Done.
falken@chromium.org changed reviewers: + tkent@chromium.org
+tkent: in case you'd like to check document-close-during-controllerchange.html https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/chromium/document-close-during-controllerchange.html (right): https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/chromium/document-close-during-controllerchange.html:15: return wait_for_update(t, registration); this can just be: return wait_for_state(t, registration.installing, 'activated'); I know our current tests are not consistent about this, there's a plan to clean them up. https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/chromium/document-close-during-controllerchange.html:24: var frame_sw = frame.contentWindow.navigator.serviceWorker; I'm wondering if this test actually exercises what we want. Keeping a reference to navigator.serviceWorker in JavaScript might protect ServiceWorkerContainer from getting destroyed in C++, so this test wouldn't catch a bug. I think what you actually want is to not have frame_sw, and instead just call frame.remove() inside oncontrollerchange. The test passes if we don't crash. +tkent: Do you agree that would test the risk you pointed out in https://codereview.chromium.org/723923002/ ? https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/skip-waiting-worker.js (right): https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/skip-waiting-worker.js:15: .then(function(results) { nit: you can save indentation by shifting this function into the main promise chain: .then(function() { return Promise.all(promises); }) .then(function(results) { results.forEach(function(r) { ... }); }); https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/skip-waiting-installed.html (right): https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/skip-waiting-installed.html:50: return wait_for_update(t, registration); just wait_for_state(..registration.installing...) https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/skip-waiting.html (right): https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/skip-waiting.html:13: var controllerchanged = new Promise(function(resolve) { nit: pick either controllerchanged or saw_controllerchanged and use consistently over the tests. (i like saw_*, it's used in other tests) https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/skip-waiting.html:27: }); this needs +2 indent
Also this patch doesn't remove the FIXME yet.
https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/chromium/document-close-during-controllerchange.html (right): https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/chromium/document-close-during-controllerchange.html:24: var frame_sw = frame.contentWindow.navigator.serviceWorker; On 2014/12/04 06:15:38, falken wrote: > I'm wondering if this test actually exercises what we want. Keeping a reference > to navigator.serviceWorker in JavaScript might protect ServiceWorkerContainer > from getting destroyed in C++, so this test wouldn't catch a bug. > > I think what you actually want is to not have frame_sw, and instead just call > frame.remove() inside oncontrollerchange. The test passes if we don't crash. > I think use-after-free could happen in both JS & C++ sides. How about change it like below: .then(function() { assert_equals(frame_sw.controller ...); frame_sw = undefined; gc(); }) .then(function() { service_worker_unregister_and_done(t); }) > +tkent: Do you agree that would test the risk you pointed out in > https://codereview.chromium.org/723923002/ > ?
https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/chromium/document-close-during-controllerchange.html (right): https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/chromium/document-close-during-controllerchange.html:24: var frame_sw = frame.contentWindow.navigator.serviceWorker; On 2014/12/04 08:17:26, xiang wrote: > On 2014/12/04 06:15:38, falken wrote: > > I'm wondering if this test actually exercises what we want. Keeping a > reference > > to navigator.serviceWorker in JavaScript might protect ServiceWorkerContainer > > from getting destroyed in C++, so this test wouldn't catch a bug. > > > > I think what you actually want is to not have frame_sw, and instead just call > > frame.remove() inside oncontrollerchange. The test passes if we don't crash. > > > > I think use-after-free could happen in both JS & C++ sides. How about change it > like below: > .then(function() { > assert_equals(frame_sw.controller ...); > frame_sw = undefined; > gc(); > }) > .then(function() { > service_worker_unregister_and_done(t); > }) My doubt with that is it doesn't test what happens if oncontrollerchange dispatch synchronously causes the C++ object to get destroyed, e.g.,: container->dispathchEvent('controllerchange'); container->blahblah(); If gc() is inside the then(), it won't execute before you return from dispatchEvent(), right? How about oncontrollerchange = function() { frame.remove(); assert_equals(frame_sw.controller.state, 'activating'); frame_sw = undefined; gc(); return Promise.resolve(); // or setTimeout(0) }) .then(function() { service_worker_unregister_and_done(t); }) I'm actually doubting the usefulness of this test altogether, it tries to conceive conditions that would cause a hypothetical bug. I'm happy if it's not crashing locally and am OK with omitting this test now...
Just took another quick look. Can you also add a test for a page registering a skipWaiting service worker in a scope that the page itself is inside? So it should verify that after calling register(), the page itself is controlled by the worker. https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/skip-waiting.html (right): https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/skip-waiting.html:32: return wait_for_state(t, sw, 'activated'); Lines 30-32 should be collapsed into simply: return wait_for_state(t, registration.installing, "activated");
On 2014/12/11 05:01:51, falken wrote: > Just took another quick look. > > Can you also add a test for a page registering a skipWaiting service worker in a > scope that the page itself is inside? So it should verify that after calling > register(), the page itself is controlled by the worker. Sorry now I remember that you implemented skipWaiting to only affect pages that already used the registration. And this is also what the current spec says. Let's add a test to confirm the behavior: the page is NOT controlled by the worker.
Patchset #3 (id:80001) has been deleted
Thanks for the review, please take another look. tkent@ would you also review the public/platform change? https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/chromium/document-close-during-controllerchange.html (right): https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/chromium/document-close-during-controllerchange.html:24: var frame_sw = frame.contentWindow.navigator.serviceWorker; On 2014/12/04 08:59:12, falken wrote: > On 2014/12/04 08:17:26, xiang wrote: > > On 2014/12/04 06:15:38, falken wrote: > > > I'm wondering if this test actually exercises what we want. Keeping a > > reference > > > to navigator.serviceWorker in JavaScript might protect > ServiceWorkerContainer > > > from getting destroyed in C++, so this test wouldn't catch a bug. > > > > > > I think what you actually want is to not have frame_sw, and instead just > call > > > frame.remove() inside oncontrollerchange. The test passes if we don't crash. > > > > > > > I think use-after-free could happen in both JS & C++ sides. How about change > it > > like below: > > .then(function() { > > assert_equals(frame_sw.controller ...); > > frame_sw = undefined; > > gc(); > > }) > > .then(function() { > > service_worker_unregister_and_done(t); > > }) > > My doubt with that is it doesn't test what happens if oncontrollerchange > dispatch synchronously causes the C++ object to get destroyed, e.g.,: > container->dispathchEvent('controllerchange'); > container->blahblah(); > > If gc() is inside the then(), it won't execute before you return from > dispatchEvent(), right? > > How about > > oncontrollerchange = function() { > frame.remove(); > assert_equals(frame_sw.controller.state, 'activating'); > frame_sw = undefined; > gc(); > return Promise.resolve(); // or setTimeout(0) > }) > .then(function() { > service_worker_unregister_and_done(t); > }) > > I'm actually doubting the usefulness of this test altogether, it tries to > conceive conditions that would cause a hypothetical bug. I'm happy if it's not > crashing locally and am OK with omitting this test now... You're right, the gc() won't real happen before return from dispatchEvent(). I have test it locally, it doesn't crash. So I will not include this case in this CL, is this OK? Another issue(not related to controllerchange event) I met is: after frame.remove(), if call frame_sw.register(...) it will return undefined, other than a rejected promise. It seems right from the code, but is it the intended behavior? The spec doesn't define it. https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/resources/skip-waiting-worker.js (right): https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/resources/skip-waiting-worker.js:15: .then(function(results) { On 2014/12/04 06:15:38, falken wrote: > nit: you can save indentation by shifting this function into the main promise > chain: > > .then(function() { > return Promise.all(promises); > }) > .then(function(results) { > results.forEach(function(r) { ... }); > }); Done. https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/skip-waiting-installed.html (right): https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/skip-waiting-installed.html:50: return wait_for_update(t, registration); On 2014/12/04 06:15:38, falken wrote: > just wait_for_state(..registration.installing...) Done. https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... File LayoutTests/http/tests/serviceworker/skip-waiting.html (right): https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/skip-waiting.html:13: var controllerchanged = new Promise(function(resolve) { On 2014/12/04 06:15:38, falken wrote: > nit: pick either controllerchanged or saw_controllerchanged and use consistently > over the tests. (i like saw_*, it's used in other tests) Done. https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/skip-waiting.html:27: }); On 2014/12/04 06:15:38, falken wrote: > this needs +2 indent Done. https://codereview.chromium.org/765323002/diff/60001/LayoutTests/http/tests/s... LayoutTests/http/tests/serviceworker/skip-waiting.html:32: return wait_for_state(t, sw, 'activated'); On 2014/12/11 05:01:51, falken wrote: > Lines 30-32 should be collapsed into simply: return wait_for_state(t, > registration.installing, "activated"); Done.
On 2014/12/11 05:35:19, falken wrote: > On 2014/12/11 05:01:51, falken wrote: > > Just took another quick look. > > > > Can you also add a test for a page registering a skipWaiting service worker in > a > > scope that the page itself is inside? So it should verify that after calling > > register(), the page itself is controlled by the worker. > > Sorry now I remember that you implemented skipWaiting to only affect pages that > already used the registration. And this is also what the current spec says. > Let's add a test to confirm the behavior: the page is NOT controlled by the > worker. In current implementation, the page will be controlled by the worker. The new registration will associate the page as it doesn't have other associated registration. I'm not sure whether it is OK in the spec or not(at least the algorithm part is aligned). However I think it's a reasonable behavior. The new test is added to skip-waiting.html, PTAL, thanks.
haven't reviewed yet but want to send this comment first: i just confirmed with the spec folks that it should not be controlled. but we can land first and fix in follow-up patches. On 2014年12月11日(木) 20:32 null <xiang.long@intel.com> wrote: > On 2014/12/11 05:35:19, falken wrote: > > On 2014/12/11 05:01:51, falken wrote: > > > Just took another quick look. > > > > > > Can you also add a test for a page registering a skipWaiting service > > worker > in > > a > > > scope that the page itself is inside? So it should verify that after > > calling > > > register(), the page itself is controlled by the worker. > > > Sorry now I remember that you implemented skipWaiting to only affect > pages > that > > already used the registration. And this is also what the current spec > > says. > > Let's add a test to confirm the behavior: the page is NOT controlled by > > the > > worker. > > In current implementation, the page will be controlled by the worker. The > new > registration will associate the page as it doesn't have other associated > registration. > > I'm not sure whether it is OK in the spec or not(at least the algorithm > part is > aligned). However I think it's a reasonable behavior. > The new test is added to skip-waiting.html, PTAL, thanks. > > > https://codereview.chromium.org/765323002/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/12/11 11:35:33, falken wrote: > haven't reviewed yet but want to send this comment first: i just confirmed > with the spec folks that it should not be controlled. but we can land first > and fix in follow-up patches. > OK, I have removed the test, and uploaded https://codereview.chromium.org/798833002 to fix it, please also take a look of that, thanks.
On 2014/12/12 04:30:18, xiang wrote: > On 2014/12/11 11:35:33, falken wrote: > > haven't reviewed yet but want to send this comment first: i just confirmed > > with the spec folks that it should not be controlled. but we can land first > > and fix in follow-up patches. > > > OK, I have removed the test, and uploaded > https://codereview.chromium.org/798833002 to fix it, please also take a look of > that, thanks. Thanks. Please keep the test but reverse its expectation.
On 2014/12/15 02:31:17, falken wrote: > On 2014/12/12 04:30:18, xiang wrote: > > On 2014/12/11 11:35:33, falken wrote: > > > haven't reviewed yet but want to send this comment first: i just confirmed > > > with the spec folks that it should not be controlled. but we can land first > > > and fix in follow-up patches. > > > > > OK, I have removed the test, and uploaded > > https://codereview.chromium.org/798833002 to fix it, please also take a look > of > > that, thanks. > > Thanks. Please keep the test but reverse its expectation. Done. PTAL, thanks!
lgtm, thanks Another good test is when you're in a state with both an active and waiting version, register a SW that calls skipWaiting(). That new SW should reach active and the other two should hit redundant. But that can be a follow-up patch. https://codereview.chromium.org/765323002/diff/140001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/skip-waiting-installed.html (right): https://codereview.chromium.org/765323002/diff/140001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/skip-waiting-installed.html:52: return wait_for_state(t, sw, 'installed'); you can collapse lines 48-52 into: service_worker = registration.installing; return wait_for_state(t, service_worker, 'installed');
On 2014/12/15 06:54:04, falken wrote: > lgtm, thanks > > Another good test is when you're in a state with both an active and waiting > version, register a SW that calls skipWaiting(). That new SW should reach active > and the other two should hit redundant. But that can be a follow-up patch. > Thanks for the suggestion, I have added that test in skip-waiting.html, you may want to take another look. > https://codereview.chromium.org/765323002/diff/140001/LayoutTests/http/tests/... > File LayoutTests/http/tests/serviceworker/skip-waiting-installed.html (right): > > https://codereview.chromium.org/765323002/diff/140001/LayoutTests/http/tests/... > LayoutTests/http/tests/serviceworker/skip-waiting-installed.html:52: return > wait_for_state(t, sw, 'installed'); > you can collapse lines 48-52 into: > > service_worker = registration.installing; > return wait_for_state(t, service_worker, 'installed'); Done.
On 2014/12/15 08:20:38, xiang wrote: > On 2014/12/15 06:54:04, falken wrote: > > lgtm, thanks > > > > Another good test is when you're in a state with both an active and waiting > > version, register a SW that calls skipWaiting(). That new SW should reach > active > > and the other two should hit redundant. But that can be a follow-up patch. > > > > Thanks for the suggestion, I have added that test in skip-waiting.html, you may > want to take another look. Awesome, still lgtm.
The CQ bit was checked by xiang.long@intel.com
The CQ bit was unchecked by xiang.long@intel.com
The CQ bit was checked by xiang.long@intel.com
The CQ bit was unchecked by xiang.long@intel.com
The CQ bit was checked by xiang.long@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/765323002/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187226 |
