LayoutTests/http/tests/serviceworker/chromium/document-close-during-controllerchange.html - Issue 765323002: ServiceWorker: Add support for .skipWaiting and controllerchange event(3/3)

Side by Side Diff: LayoutTests/http/tests/serviceworker/chromium/document-close-during-controllerchange.html

Issue 765323002: ServiceWorker: Add support for .skipWaiting and controllerchange event(3/3) (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: review update Created 6 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <title>Service Worker: document close during controllerchange</title>

	3 <script src="../../resources/testharness.js"></script>

	4 <script src="../../resources/testharnessreport.js"></script>

	5 <script src="../resources/test-helpers.js"></script>

	6 <script>

	7

	8 async_test(function(t) {

	9 var scope = '../resources/blank.html';

	10 var url1 = '../resources/empty.js';

	11 var url2 = '../resources/skip-waiting-worker.js';

	12

	13 service_worker_unregister_and_register(t, url1, scope)

	14 .then(function(registration) {

	15 return wait_for_update(t, registration);
	falken 2014/12/04 06:15:38 this can just be: return wait_for_state(t, registr 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.
	16 })

	17 .then(function(sw) {

	18 return wait_for_state(t, sw, 'activated');

	19 })

	20 .then(function() {

	21 return with_iframe(scope);

	22 })

	23 .then(function(frame) {

	24 var frame_sw = frame.contentWindow.navigator.serviceWorker;
	falken 2014/12/04 06:15:38 I'm wondering if this test actually exercises what 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/ ? xiang 2014/12/04 08:17:26 I think use-after-free could happen in both JS & C Show quoted text 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); }) Show quoted text > +tkent: Do you agree that would test the risk you pointed out in > https://codereview.chromium.org/723923002/ > ? falken 2014/12/04 08:59:12 My doubt with that is it doesn't test what happens Show quoted text 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... xiang 2014/12/11 11:24:32 You're right, the gc() won't real happen before re Show quoted text 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.
	25 frame_sw.oncontrollerchange = t.step_func(function() {

	26 unload_iframe(frame)

	27 .then(function() {

	28 assert_equals(frame_sw.controller.state, 'activating',

	29 'Controller should still be accessible');

	30 service_worker_unregister_and_done(t);

	31 });

	32 });

	33 navigator.serviceWorker.register(url2, {scope: scope});

	34 })

	35 .catch(unreached_rejection(t));

	36 }, 'Access ServiceWorkerContainer after its document removed.');

	37

	38 </script>

OLD	NEW