New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 697307 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

PlzNavigate: external/wpt/service-workers/service-worker/navigate-window.https.html failure

Project Member Reported by ananta@chromium.org, Mar 1 2017

Issue description

This test fails because the order of ServiceWorkerProviderHosts is different
for PlzNavigate as hosts are precreated. The layout test expects urls to
show up in a specific order.

It seems a better option would be to rewrite the layout test to match the urls
and their frame types by doing an exhaustive search in the expected list.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/54e4fbdde6ae7aa7844a0d11489ba76c9f859afb

commit 54e4fbdde6ae7aa7844a0d11489ba76c9f859afb
Author: ananta <ananta@chromium.org>
Date: Fri Mar 03 00:52:56 2017

PlzNavigate: Rewrite the url and frame type validation part of the  external/wpt/service-workers/service-worker/navigate-window.https.html layout test.

This test expects the ServiceWorkerProviderHosts to be in a specific order which breaks with PlzNavigate due to precreated hosts. It seemed to be a better
to rewrite the validation code to do an exhaustive match of the URLs and the frame types in the result list.

BUG= 697307 

Review-Url: https://codereview.chromium.org/2726793002
Cr-Commit-Position: refs/heads/master@{#454455}

[modify] https://crrev.com/54e4fbdde6ae7aa7844a0d11489ba76c9f859afb/third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
[modify] https://crrev.com/54e4fbdde6ae7aa7844a0d11489ba76c9f859afb/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigate-window.https-expected.txt
[modify] https://crrev.com/54e4fbdde6ae7aa7844a0d11489ba76c9f859afb/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/navigate-window.https.html

Comment 2 by mek@chromium.org, Mar 3 2017

Note that the spec actually does require the clients to show up in a specific order. There are some open spec issues (https://github.com/w3c/ServiceWorker/issues/1078, https://github.com/w3c/ServiceWorker/issues/1080) to clarify what exactly that order should be, but the required order is supposed to be fully defined by the spec.

Comment 3 by mek@chromium.org, Mar 3 2017

Components: Blink>ServiceWorker
mek. Thanks for the tip. It does not seem like PlzNavigate is in anyway violating the spec. It appears to be a purely timing related issue as to which clients show up.

Can you clarify why you don't think the spec is being violated? The test was asserting an ordering of clients, and as mek said the spec does appear to guarantee an ordering.

I think we should revert the change unless we're sure it's faithful to the spec, as the WPT tests are shared by other browsers.
From the spec the else part states this for window clients.

For each service worker client client in targetClients, in the most recently focused order for window clients.

In PlzNavigate's case window.open is sent to the browser where it creates a service worker client (Precreates it to ensure it works correctly when the renderer) needs it. This is not a violation of focus order. In the non PlzNavigate case the window.open occurs a touch later in blink and hence the ordering is different.


Comment 7 by bke...@mozilla.com, Mar 3 2017

As the author of this test I can tell you I was not intending to verify the spec ordering of clients.matchAll().  In fact, at the time I wrote this test gecko did not implement clients.matchAll() ordering guarantees.

We could sort the clients in the service worker script as is done in clients-matchall-worker.js:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resources/clients-matchall-worker.js#23

The real motivation behind this test was to verify that clients.matchAll() does not expose Client objects for windows that might be in the bfcache.  See:

https://bugzilla.mozilla.org/show_bug.cgi?id=1265795

Similarly, I would expect any pre-created windows that aren't actually loaded or activated to *not* be shown in clients.matchAll() unless you use includeReserved:true.  Its not clear if this is an issue here, though.

As a side note, I have just recently created a new WPT test that validates clients.matchAll() ordering behavior:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/clients-matchall-order.https.html

It has not been upstreamed to the wpt github repo yet, though.  I can ping jgraham to do that if it would be helpful here.

Anyway, sorry for cutting in here.  I just thought the context might be helpful.
bkelly, Thanks for adding your thoughts. 

To clarify, PlzNavigate is not changing the expectation that precreated windows should not show up the list. So I think we are good here.

Thanks for the info bkelly. Sorry ananta for being too quick to request reverting.
Status: Fixed (was: Assigned)

Sign in to add a comment