Add tests for service worker inheritance #4610

jungkees · 2017-01-24T08:30:57Z

Loading an iframe without involving a navigate fetch inherits its
parent's active service worker, if it has one. By this rule:

Initial about:blank Document on iframe inherits its parent's SW
iframe srcdoc Document inherits its parent's SW

However, the navigate fetch through http fetch still wins. So, if a
client obtains a matched SW during the navigation, the matched SW
replaces the inherited SW.

An exception:

Navigating an iframe to a non-initial about:blank Document inherits
the parent's SW

Related work: whatwg/html#2080.

wpt-pr-bot · 2017-01-24T08:30:58Z

Notifying @ehsan and @mkruisselbrink. (Learn how reviewing works.)

jungkees · 2017-01-24T09:08:07Z

The spec work for this is in progress here: whatwg/html#2080. It'd be appreciated if anyone could review it.

/cc @ehsan, @mkruisselbrink, @jakearchibald, @annevk

mkruisselbrink · 2017-01-25T00:43:25Z

service-workers/service-worker/iframe-controller.https.html

+          return service_worker_unregister(t, scope1);
+        })
+      .then(function() {
+          service_worker_unregister_and_done(t, scope2);


you don't need the and_done() variant in a promise_test. When the overall promise resolves the test will be marked as done anyway.

Removed it.

mkruisselbrink · 2017-01-25T00:43:54Z

service-workers/service-worker/iframe-controller.https.html

+      .then(function() {
+          service_worker_unregister_and_done(t, scope2);
+        })
+      .catch(unreached_rejection(t));


You also don't need this .catch(unreached_rejection(t)) in a promise_test. If the promise passed to promise_test rejects the test will be considered a failure.

Removed it.

mkruisselbrink · 2017-01-25T00:47:03Z

service-workers/service-worker/iframe-controller.https.html

+                        'about:blank Document should inherit parent\'s SW.');
+        })
+      .then(function() {
+          return with_srcdoc_iframe('<p>Some HTML</p>');


It would be nice if the various tests (about:blank, srcdoc, etc) would be actually separate promise_tests, rather than just assertions all in the same test. That way the test results would much clearer show which features work/don't work, rather than only reporting the first failing result. Not sure how tricky that is going to be with the various service worker registrations, but it shouldn't be too bad I think.

One way might be to do something like (but then with proper syntax etc):

let registered_workers = service_worker_unregister_and_register(...).then(...).then(...); promise_test(t => { registered_workers.then(() => with_iframe('')) .then(f => { assert(...) }); }, 'about:blank document'); promise_test(t => {....}, 'srcdoc iframe'); // etc

This is a good point indeed. I addressed them as suggested basically but put the registration and activation chain in the first promise_test as it needs |t|.

mkruisselbrink · 2017-01-25T00:48:04Z

service-workers/service-worker/iframe-controller.https.html

+    var script1 = script + '?for_scope1';
+    var script2 = script + '?for_scope2';
+    var registration1, registration2;
+    var parent_controller, child_controller;


Why did you declare child_controller at this scope? I think just declaring it in all the closures where you actually use it would be cleaner (I don't think you ever use it across closure boundaries?)

Addressed by declaring them in their closures.

mkruisselbrink · 2017-01-25T00:48:50Z

service-workers/service-worker/iframe-controller.https.html

+    var scope2 = 'resources/blank.html';
+    var script1 = script + '?for_scope1';
+    var script2 = script + '?for_scope2';
+    var registration1, registration2;


What are these variables used for? You assign to them, but I don't see them being read anywhere?

Right. They're unused. Deleted.

mkruisselbrink · 2017-01-25T00:50:46Z

service-workers/service-worker/iframe-controller.https.html

+                        'matched SW should set the controller to null.');
+        })
+      .then(function() {
+          return service_worker_unregister(t, scope1);


Rather than calling service_worker_unregister at the end of your test here, it is probably cleaner to call add_completion_callback(() => {r.unregister();}); as soon as you have registered a service worker, to make sure they get cleaned up even if the test fails.

Right. Addressed them for registration.unregister() and frame.remove().

mkruisselbrink · 2017-01-25T00:58:35Z

I think the travis check failure is because an unrelated test (that just includes the test-helpers.sub.js file you modified) has non-unique test names (see #4404)

jungkees · 2017-01-25T06:37:46Z

@mkruisselbrink, thanks for reviewing! I uploaded a patch addressing those comments.

jungkees · 2017-01-25T06:41:48Z

I think the travis check failure is because an unrelated test (that just includes the test-helpers.sub.js file you modified) has non-unique test names

What would be the expected resolution for this for now? It seems

ERROR:check_stability:| /service-workers/service-worker/fetch-event.https.html

has non-unique test names at least.

mkruisselbrink · 2017-01-25T19:59:15Z

What would be the expected resolution for this for now? It seems

ERROR:check_stability:| /service-workers/service-worker/fetch-event.https.html

has non-unique test names at least.

That should be fixed now #4626 landed (hopefully that was the only one). Not sure how to trigger checks again on this CL though...

mkruisselbrink · 2017-01-25T20:02:56Z

service-workers/service-worker/iframe-controller.https.html

+        // This client's active service worker is set to scope1's registration's
+        // active worker by clients.claim() during the activation.
+        parent_controller = navigator.serviceWorker.controller;
+        return with_iframe('')


Maybe adding an "options" parameter to with_iframe with an auto_remove option (like in the chromium version of with_iframe) would make sense? Having a boolean that defaults to true like in the chromium version seems wrong, but having an option to pass to with_iframe to get auto remove behavior does seem reasonable.

Yes, that makes sense. I think there are quite a few tests already that manually remove the iframes. So I made it opt-in to auto remove the iframe by passing options.auto_remove set to true.

mkruisselbrink · 2017-01-25T20:04:53Z

service-workers/service-worker/resources/test-helpers.sub.js

@@ -58,6 +58,25 @@ function with_iframe(url) {
    });
 }

+function with_sandboxed_iframe(url, sandbox) {


If you add an "options" dictionary to with_iframe to support auto_remove, you could also add a sandbox option to that same dictionary rather than having a separate function for this.

Yes, I added sandbox property and srcdoc property to the options as suggested. Looks cleaner.

jungkees · 2017-01-31T09:52:44Z

Does anyone have any idea what causes this travis-ci error?: https://travis-ci.org/w3c/web-platform-tests/jobs/195441119.

/cc @Ms2ger @jgraham @zcorpan @annevk

annevk · 2017-01-31T10:07:24Z

It seems the test takes too long to run from "The job exceeded the maximum time limit for jobs, and has been terminated."

Ms2ger · 2017-01-31T11:27:09Z

WebDriverException: Message: no such session, it says... Not sure what's up with that.

ghost · 2017-02-14T07:39:51Z

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision f403d50
Using browser at version 59.0.3063.4 dev

ghost · 2017-02-14T07:39:52Z

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision f403d50
Using browser at version BuildID 20170406100208; SourceStamp 3c68d659c2b715f811708f043a1e7169d77be2ba

jungkees · 2017-02-14T08:00:42Z

The test added in this PR seems okay now. Need to look at /service-workers/service-worker/request-end-to-end.https.html that's flaky on Chrome.

ghost · 2017-03-10T04:09:19Z

These tests are now available on w3c-test.org

jungkees · 2017-03-10T04:11:06Z

Try the Travis CI again after checking /service-workers/service-worker/request-end-to-end.https.html is constantly working as expected (failure) in Chrome: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=webkit_tests%20(with%20patch)&tests=external%2Fwpt%2Fservice-workers%2Fservice-worker%2Frequest-end-to-end.https.html

jungkees · 2017-03-10T05:44:25Z

Recently added /service-workers/service-worker/clients-matchall-order.https.html is failing in Chrome. When the test is rolled to Chromium, we'll need to update the TestExpectations to proceed with this PR.

ghost · 2017-03-23T07:54:36Z

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision 578f5ea
Git command exited with status 128
fatal: Invalid revision range cf62b85..

ghost · 2017-03-23T07:54:37Z

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision 578f5ea
Git command exited with status 128
fatal: Invalid revision range cf62b85..

Loading an iframe without involving a navigate fetch inherits its parent's active service worker, if it has one. By this rule: - Initial about:blank Document on iframe inherits its parent's SW - iframe srcdoc Document inherits its parent's SW However, the navigate fetch through http fetch still wins. So, if a client obtains a matched SW during the navigation, the matched SW replaces the inherited SW. An exception: - Navigating an iframe to a non-initial about:blank Document inherits the parent's SW Related work: whatwg/html#2080.

Add options param to with_iframe to set the iframe's sandbox attribute and srcdoc attribute as well as add a callback to remove the iframe when the test is done.

foolip · 2018-10-01T11:56:58Z

@jungkees, can you rebase this to resolve the conflicts and retrigger Travis? Whatever the trouble with Travis was over a year ago, it's probably gone now.

mfalken · 2018-10-23T09:31:03Z

Some of this may have changed in the intervening time see eg whatwg/html#3725 and existing tests mentioned at whatwg/html#3725 (comment)

wpt-pr-bot added service-workers wg-webplatform labels Jan 24, 2017

jungkees mentioned this pull request Jan 24, 2017

Specify active service worker inheritance whatwg/html#2080

Closed

mkruisselbrink reviewed Jan 25, 2017

View reviewed changes

jungkees added 7 commits March 24, 2017 10:58

Address @mkruisselbrink's comments: separate out tests, etc.

ac88d41

Add options to with_iframe

273ab57

Add options param to with_iframe to set the iframe's sandbox attribute and srcdoc attribute as well as add a callback to remove the iframe when the test is done.

Try the Travis CI again; added guards

0d7869f

Try the Travis CI again

9c0bbda

Try the Travis CI again

88869c8

Try the Travis CI again

c7d9ddf

Try the Travis CI again

24f6162

jungkees force-pushed the sw-inheritance-fix branch from e57580e to 24f6162 Compare March 24, 2017 02:07

jungkees mentioned this pull request Mar 24, 2017

clarify creation URL when initial about:blank is replaced whatwg/html#2456

Open

jungkees added 2 commits March 28, 2017 11:23

Try the Travis CI again

9415101

Try the Travis CI

d7fe390

jungkees mentioned this pull request Jul 4, 2017

Specify active service worker inheritance whatwg/html#2809

Open

wpt-pr-bot added the wg-sw label Oct 1, 2018

wpt-pr-bot requested review from asutherland, mfalken and wanderview October 1, 2018 11:57

Add tests for service worker inheritance #4610

Are you sure you want to change the base?

Add tests for service worker inheritance #4610

Conversation

jungkees commented Jan 24, 2017 • edited by wpt-pr-bot Loading

wpt-pr-bot commented Jan 24, 2017

jungkees commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkruisselbrink commented Jan 25, 2017

jungkees commented Jan 25, 2017

jungkees commented Jan 25, 2017

mkruisselbrink commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees commented Jan 31, 2017

annevk commented Jan 31, 2017

Ms2ger commented Jan 31, 2017

ghost commented Feb 14, 2017 • edited by ghost Loading

Chrome (unstable channel)

ghost commented Feb 14, 2017 • edited by ghost Loading

Firefox (nightly channel)

jungkees commented Feb 14, 2017

ghost commented Mar 10, 2017 • edited by ghost Loading

jungkees commented Mar 10, 2017

jungkees commented Mar 10, 2017

ghost commented Mar 23, 2017

Firefox (nightly channel)

ghost commented Mar 23, 2017

Chrome (unstable channel)

foolip commented Oct 1, 2018

mfalken commented Oct 23, 2018

jungkees commented Jan 24, 2017 •

edited by wpt-pr-bot

Loading

ghost commented Feb 14, 2017 •

edited by ghost

Loading

ghost commented Feb 14, 2017 •

edited by ghost

Loading

ghost commented Mar 10, 2017 •

edited by ghost

Loading