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

Issue 701233 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 678905



Sign in to add a comment

external/wpt/service-workers/service-worker/clients-matchall-order.https.html is flaky

Project Member Reported by qyears...@chromium.org, Mar 14 2017

Issue description

See flakiness dashboard:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=webkit_tests&tests=external/wpt/service-workers/service-worker/clients-matchall-order.https.html

The test sometimes times out, sometimes passes, and sometimes fails.

Specifically, looking at particular results, the method that appears to sometimes pass and sometimes fail is "Clients.matchAll() returns non-focused controlled windows in creation order".

Test code search link: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-matchall.https.html
 
Components: Blink>ServiceWorker
Clarification: this is a new wpt test that hasn't been imported yet; when it was imported in 6782567de4ad3aadd41740db62fbd4123b7378b6 it was run for a bit but it was flaky.
Cc: -shimazu@chromium.org
Labels: -Pri-2 Pri-1
Owner: shimazu@chromium.org
Status: Available (was: Unconfirmed)
I believe the test is correct, and that Chromium may not be honoring the
`matchAll` algorithm step that dictates the expected ordering of the returned
"clients" array (currently specified as 2.5) [1]. This seems likely given that
the sorting behavior was specified relatively recently (just under 5 months
ago, on 2016-11-07) [2].

A novice's interpretation of the source tends to confirm this. I'd expect the
logic to appear in
`content/browser/service_worker/service_worker_client_utils.cc`, but of the two
changes made in the last six months, neither seems related to this behavior.
There is some sorting logic present, but it only concerns the "last focused
time," (see [3] and [4]). I believe this would produce unstable results in
cases where frames had never received focus (as in the offending test).

[1] https://w3c.github.io/ServiceWorker/#clients-getall

> 2. Run these substeps in parallel:
>
> [...]
>
>    5. Sort matchedClients such that:
>       - WindowClient objects are always placed before Client objects whose
>         associated service worker clients are worker clients.
>       - WindowClient objects that have been focused are placed first sorted
>         in the most recently focused order, and WindowClient objects that
>         have never been focused are placed next sorted in their service
>         worker clients' creation order.
>       - Client objects whose associated service worker clients are worker
>         clients are placed next sorted in their service worker clients'
>         creation order.

[2] https://github.com/w3c/ServiceWorker/commit/2e67d60ca8605e2a614f696efbdb2687dc845b80

[3] https://chromium.googlesource.com/chromium/src/+/45fe549/content/browser/service_worker/service_worker_client_utils.cc#357

    // Sort clients so that the most recently active tab is in the front.
    std::sort(clients->begin(), clients->end(), ServiceWorkerClientInfoSortMRU());

[4] https://chromium.googlesource.com/chromium/src/+/45fe549/content/browser/service_worker/service_worker_client_utils.cc#346

    struct ServiceWorkerClientInfoSortMRU {
      bool operator()(const ServiceWorkerClientInfo& a,
                      const ServiceWorkerClientInfo& b) const {
        return a.last_focus_time > b.last_focus_time;
      }
    };

Comment 4 by falken@chromium.org, Apr 18 2017

Blocking: 678905
Labels: -Pri-1 Pri-2
Status: Assigned (was: Available)
I don't think this is Pri=1 since the test always failed on Chrome.
@shimazu
I have created a patch https://codereview.chromium.org/2905593002#, and with this patch, one failed case can pass now.
But I have some questions while debugging the root cause of the other failed test cases in ./third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-matchall-order.https.html:

(1) Test cases 4(no-focus-uncontrolled-windows),5(focus-uncontrolled-windows-1),6(focus-uncontrolled-windows-2). All of them expected a URL(TOP_HARNESS_URL) to be loaded, but I didn't find where it will be loaded. And the failure indeed is "can not find this expected URL". So if the three cases is correct? I found that if comments TOP_HARNESS_URL in 3rd test case, it will pass, but not in 4th and 5th test case, and then there is a new question: if the iframe was focused, 
are the focus times for both parent/child frames updated or just the iframe? That's why both of them still fail.

(2) The 7th test case. I find a discussion about it and above question, https://github.com/w3c/ServiceWorker/issues/1080.
I am not sure what's the conclusion of Chromium, but anyway the root cause of the test fail is the Chromium implementation instead of the matchAll algorithm? If so, we can then fix it of cause.

Comment 6 by bke...@mozilla.com, May 24 2017

> All of them expected a URL(TOP_HARNESS_URL) to be loaded, but I didn't find where it will be loaded.

These are the WPT test harness windows themselves.  They are created before the test case starts.
@bkelly, thanks a lot for your explanation.
As only a ServiceWorkerProviderHost that was already added, the test can then expected this window client, so the WPT test harness windows is also associated with a service worker? And do you know the test result on FireFox? 
Cc: shimazu@chromium.org
Owner: xiaofeng...@intel.com
Status: Started (was: Assigned)
re #5:
(1): I guess your question is "why are TEST_HARNESS_URL and TOP_HARNESS_URL in the list of expectation even though they are out of the scope?". 
I also now revisited the spec and found that matchAll API returns not only clients within the scope but also all clients which have the same origin if options.includeUncontrolled is set to true. I didn't know the reason why the spec is like this, but TOP_HARNESS_URL and TEST_HARNESS_URL should be included even if it's out of the scope set at registration. 

(2): Yes, I think the cause of failure is coming from our implementation. 

Comment 9 by bke...@mozilla.com, May 25 2017

Right, its because `includeUncontrolled:true` should return all same-origin clients.
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 5 2017

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

commit 5c3497c94f604dee3cfe64ca84a7828a7b9bec04
Author: xiaofeng.zhang <xiaofeng.zhang@intel.com>
Date: Mon Jun 05 03:29:50 2017

Clients.matchAll() should return ordered clients by type/focus time/creation time

According to current service worker specification, sort matchedClients such that:
1) WindowClient objects are always placed before worker clients.
2) WindowClient objects that have been focused are placed first sorted in the
   most recently focused order, and WindowClient objects that have never been
   focused are placed in their creation order.
3) WorkerClients are placed in their creation order.

Current implementation only considers the focus time.

BUG= 701233 

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

[modify] https://crrev.com/5c3497c94f604dee3cfe64ca84a7828a7b9bec04/content/browser/service_worker/service_worker_client_utils.cc
[modify] https://crrev.com/5c3497c94f604dee3cfe64ca84a7828a7b9bec04/content/browser/service_worker/service_worker_client_utils.h
[modify] https://crrev.com/5c3497c94f604dee3cfe64ca84a7828a7b9bec04/content/browser/service_worker/service_worker_provider_host.cc
[modify] https://crrev.com/5c3497c94f604dee3cfe64ca84a7828a7b9bec04/content/browser/service_worker/service_worker_provider_host.h
[modify] https://crrev.com/5c3497c94f604dee3cfe64ca84a7828a7b9bec04/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/5c3497c94f604dee3cfe64ca84a7828a7b9bec04/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/5c3497c94f604dee3cfe64ca84a7828a7b9bec04/content/common/service_worker/service_worker_client_info.cc
[modify] https://crrev.com/5c3497c94f604dee3cfe64ca84a7828a7b9bec04/content/common/service_worker/service_worker_client_info.h
[modify] https://crrev.com/5c3497c94f604dee3cfe64ca84a7828a7b9bec04/third_party/WebKit/LayoutTests/TestExpectations
[rename] https://crrev.com/5c3497c94f604dee3cfe64ca84a7828a7b9bec04/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-matchall-order.https-expected.txt
[delete] https://crrev.com/8f43c3fa3816ec6b70168653eafd9b924412b426/third_party/WebKit/LayoutTests/platform/mac-retina/external/wpt/service-workers/service-worker/clients-matchall-order.https-expected.txt
[delete] https://crrev.com/8f43c3fa3816ec6b70168653eafd9b924412b426/third_party/WebKit/LayoutTests/platform/mac/external/wpt/service-workers/service-worker/clients-matchall-order.https-expected.txt
[delete] https://crrev.com/8f43c3fa3816ec6b70168653eafd9b924412b426/third_party/WebKit/LayoutTests/platform/win/external/wpt/service-workers/service-worker/clients-matchall-order.https-expected.txt
[delete] https://crrev.com/8f43c3fa3816ec6b70168653eafd9b924412b426/third_party/WebKit/LayoutTests/platform/win7/external/wpt/service-workers/service-worker/clients-matchall-order.https-expected.txt

Status: Fixed (was: Started)

Sign in to add a comment