LayoutTests/http/tests/serviceworker/service-worker-gc.html - Issue 330173003: Make ServiceWorker an ActiveDOMObject

Side by Side Diff: LayoutTests/http/tests/serviceworker/service-worker-gc.html

Issue 330173003: Make ServiceWorker an ActiveDOMObject (Closed) Base URL: svn://svn.chromium.org/blink/trunk

Patch Set: review comments Created 6 years, 6 months 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 | Annotate | Revision Log

« no previous file with comments | « LayoutTests/http/tests/serviceworker/fetch-event.html ('k') | LayoutTests/http/tests/serviceworker/service-worker-gc-expected.txt » ('j') | Source/modules/serviceworkers/ServiceWorker.cpp » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

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

	3 <body>

	4 <script>

	5 (function() {

	6 window.jsTestIsAsync = true;

	7 description('Test that a registered Service Worker with an event handler is not garbage collected');
	dominicc (has gone to gerrit) 2014/06/13 19:55:13 Maybe add '...prematurely'? Maybe add '...prematurely'? falken 2014/06/13 22:34:18 Done. Show quoted text On 2014/06/13 19:55:13, dominicc wrote: > Maybe add '...prematurely'? Done.
	8 var worker = 'resources/empty-worker.js';

	9 var scope = 'gc';

	10 swObservation = null;

	11

	12 unregister_and_register(worker, scope).then(onRegister);

	13

	14 function unregister_and_register(url, scope) {

	15 return navigator.serviceWorker.unregister(scope).then(function() {

	16 return navigator.serviceWorker.register(url, { scope: scope });

	17 }).catch(function(error) {

	18 debug('Could not register worker: ' + error);

	19 finishJSTest();

	20 });

	21 }

	22

	23 function collectGarbage(shouldBeCollected, description) {
	dominicc (has gone to gerrit) 2014/06/13 19:55:13 I'm not sure this function is pulling its weight. I'm not sure this function is pulling its weight. A specific assertion can be done in three lines: observation = internals.observeGC(x); ... gc(); test observation.wasCollected That is only two more lines than calling collectGarbage; it might be more readable that way. WDYT? falken 2014/06/13 22:34:18 I tried but there's a couple problems with that: 1 Show quoted text On 2014/06/13 19:55:13, dominicc wrote: > I'm not sure this function is pulling its weight. A specific assertion can be > done in three lines: > > observation = internals.observeGC(x); > ... > gc(); > test observation.wasCollected > > That is only two more lines than calling collectGarbage; it might be more > readable that way. WDYT? I tried but there's a couple problems with that: 1) onUnregister doesn't have sw, so it can't call internals.observeGC(sw) 2) If we replace the collectGarbage in onRegister with a function() { ... }, I think sw would not be garbage collected inside that function, since it has access to sw (even if it doesn't actually use it), right?
	24 gc();

	25 if (swObservation) {
	dominicc (has gone to gerrit) 2014/06/13 19:55:13 Would it be better to fail if this is not set up? Would it be better to fail if this is not set up? If this was accidentally missing, the test would silently pass by not testing any assertions. falken 2014/06/13 22:34:18 I agree this is making the test more complex and c Show quoted text On 2014/06/13 19:55:13, dominicc wrote: > Would it be better to fail if this is not set up? If this was accidentally > missing, the test would silently pass by not testing any assertions. I agree this is making the test more complex and changed it to fail early if there's no observeGC. My original intention was that the test could still run in environments without internals.observeGC. It would timeout if the sw got gc'd too early (and gc() attempts to trigger gc if internals is not available).
	26 debug(description);

	27 if (shouldBeCollected)

	28 shouldBeTrue('swObservation.wasCollected');

	29 else

	30 shouldBeFalse('swObservation.wasCollected');

	31 }

	32 }

	33

	34 function onUnregister() {

	35 // FIXME: The expectation should be reversed, but our implementation cur rently fails it.

	36 // When properly implemented, the SW not leak here. It may be better to move this test into a separate, targeted file.

	37 collectGarbage(false, 'Service Worker should not leak after unregistrati on');

	38 finishJSTest();

	39 }

	40

	41 function onRegister(sw) {

	42 sw.addEventListener('statechange', onStateChange);

	43 if (window.internals)

	44 swObservation = internals.observeGC(sw);
	dominicc (has gone to gerrit) 2014/06/13 19:55:13 Is this a global just so shouldBe works? Is this a global just so shouldBe works? falken 2014/06/13 22:34:18 That, and onRegister needs it. Show quoted text On 2014/06/13 19:55:13, dominicc wrote: > Is this a global just so shouldBe works? That, and onRegister needs it.
	45 setTimeout(collectGarbage.bind(undefined, false, 'Service Worker should not be gc\'d after registration'), 0);

	46 }

	47

	48 function onStateChange(event) {

	49 collectGarbage(false, 'Service Worker should not be gc\'d in ' + event.t arget.state + ' state');
	dominicc (has gone to gerrit) 2014/06/13 19:55:13 If it is garbage collected, won't event.target be If it is garbage collected, won't event.target be null? Or do we create a new one? falken 2014/06/13 22:34:18 From what I've seen on the timing out test, if we Show quoted text On 2014/06/13 19:55:13, dominicc wrote: > If it is garbage collected, won't event.target be null? Or do we create a new > one? From what I've seen on the timing out test, if we got to this line, it meant the sw was not gc'd. If sw is gc'd, then the event handler is never entered. This line tries to trigger gc to test that the sw stays alive.
	50 if (event.target.state != 'active')

	51 return;

	52 navigator.serviceWorker.unregister(scope).then(onUnregister);

	53 }

	54 }());

	55 </script>

	56 </body>

OLD	NEW