Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should the order of registrations returned by getRegistrations() be sorted by scope? #1465

Closed
makotoshimazu opened this issue Aug 30, 2019 · 13 comments

Comments

@makotoshimazu
Copy link

This question is raised when I tried to fix a flaky test in Chromium.

Currently we use an ordered map for scope to registration map but don't use the sort operation before iterating over the map (step 3.2 in the spec of getRegistrations()). This means that the order of the scope to registration map is FIFO.
Should we change it to sort the registration map before iteration, or is FIFO better in this case?
Do you have any thoughts?

@mfalken
Copy link
Member

mfalken commented Sep 2, 2019

FWIW I think we could make Chromium match the current spec by sorting by registration id.

Do other browsers preserve the FIFO order? If so, maybe we should just change Chromium to do so and call it a day. @youennf @asutherland

@asutherland
Copy link

asutherland commented Sep 2, 2019

Per https://searchfox.org/mozilla-central/rev/9415ecf29ba1acbef9381335e0ecde5916ca4073/dom/serviceworkers/ServiceWorkerManager.cpp#1678 we (Firefox/Gecko) apparently order such that:

    // Sort by length, with longest match first.
    // /foo/bar should be before /foo/
    // Similarly /foo/b is between the two.

This is an outgrowth of #287 deciding on longest prefix match now codified in https://w3c.github.io/ServiceWorker/#scope-match-algorithm. We store things as an ordered list, and we order the list so that if we traverse the list in order, we will naturally find the longest prefix first.

This seems better than FIFO order to me and most consistent with the rest of the spec implementation.

@mfalken
Copy link
Member

mfalken commented Sep 2, 2019

Thanks @asutherland. What order does getRegistrations() return when the scopes are "foo" and "bar"?

@youennf
Copy link

youennf commented Sep 2, 2019

WebKit implementation is sorting by insertion order.

@asutherland
Copy link

I think it's insertion-order dependent in that case, which makes it undesirable after all.

For sanity of tests, I think the current spec of ordering based on the serialized scope is perhaps best. Actual insertion order depends on invocation of Set Registration invoked from inside Register which happens from inside the per-scope job queue which is triggered in parallel from Run Job queuing a task on an unnamed task source.

@makotoshimazu
Copy link
Author

So if we do that, will the order be a kind of lexicographical order but longer scopes will be earlier?

For example:

bar/abc
bar/abd
bar/ac
ba/
b
foo/

Now I'm feeling that simple lexicographical order might be simpler. What do you think?

@wangw-1991
Copy link

In chromium, the registrations returned by getRegistrations() are composed of two parts (the corresponding code): stored registrations and unstored registrations. These two parts are inserted into the same vector and then return the vector.
I think before return the vector, we can sort it by registration.scope using simple lexicographical order. Is it OK?

@wangw-1991
Copy link

What's your opinion? @mattto

@mfalken
Copy link
Member

mfalken commented Jan 14, 2020

To summarize the current situation:

  1. Spec: insertion order
  2. Firefox: Sort by length first, with insertion-order to break ties
  3. WebKit: insertion order
  4. Chrome: no intended order

I'm leaning toward insertion order as the one with least changes. For the tests that actually test the order, we'd have to make sure to set up the test correctly, as @asutherland mentions there are parallel tasks on unspecified task sources.

But I don't feel strongly. cc @jakearchibald @jungkees

@wanderview
Copy link
Member

A minor note: If my scopes pattern proposal moves forward then insertion order might work a little bit better. Of course, we can always define some sort order for the patterns and list-of-patterns but that may not be intuitive for developers.

@jungkees
Copy link
Collaborator

Insertion order sounds good to me if @asutherland is fine with that. I don't think we had any other intention except letting it be an ordered map for the level of compat we get from it. @mattto's comment on the test would be important, if we go with this decision.

@asutherland
Copy link

Insertion order works for me.

@mfalken
Copy link
Member

mfalken commented Jan 16, 2020

Thanks everyone. It seems we have consensus to keep the current spec which requires insertion order.

@mfalken mfalken closed this as completed Jan 16, 2020
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 16, 2020
… ID.

This patch makes the order of the registrations returned by
getRegistrations() be sorted by registration ID, which fixes the flaky
test of getregistrations.https.html. Related Github issue is in
w3c/ServiceWorker#1465

Bug:925740
Change-Id: Iaa5716eec886232df61bd23487fbae5c63413e9c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 17, 2020
… ID.

This patch makes the order of the registrations returned by
getRegistrations() be sorted by registration ID, which fixes the flaky
test of getregistrations.https.html. Related Github issue is in
w3c/ServiceWorker#1465

Bug: 925740
Change-Id: Iaa5716eec886232df61bd23487fbae5c63413e9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1984900
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Wei4 Wang <wei4.wang@intel.com>
Cr-Commit-Position: refs/heads/master@{#732749}
pull bot pushed a commit to FreddyZeng/chromium that referenced this issue Jan 17, 2020
… ID.

This patch makes the order of the registrations returned by
getRegistrations() be sorted by registration ID, which fixes the flaky
test of getregistrations.https.html. Related Github issue is in
w3c/ServiceWorker#1465

Bug: 925740
Change-Id: Iaa5716eec886232df61bd23487fbae5c63413e9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1984900
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Wei4 Wang <wei4.wang@intel.com>
Cr-Commit-Position: refs/heads/master@{#732749}
stephenmcgruer pushed a commit to web-platform-tests/wpt that referenced this issue Jan 20, 2020
… ID. (#21046)

This patch makes the order of the registrations returned by
getRegistrations() be sorted by registration ID, which fixes the flaky
test of getregistrations.https.html. Related Github issue is in
w3c/ServiceWorker#1465

Bug: 925740
Change-Id: Iaa5716eec886232df61bd23487fbae5c63413e9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1984900
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Wei4 Wang <wei4.wang@intel.com>
Cr-Commit-Position: refs/heads/master@{#732749}

Co-authored-by: WangW_intel <54053982+wangw-1991@users.noreply.github.com>
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jan 23, 2020
…gistrations() by registration ID., a=testonly

Automatic update from web-platform-tests
Sort the registrations returned by getRegistrations() by registration ID. (#21046)

This patch makes the order of the registrations returned by
getRegistrations() be sorted by registration ID, which fixes the flaky
test of getregistrations.https.html. Related Github issue is in
w3c/ServiceWorker#1465

Bug: 925740
Change-Id: Iaa5716eec886232df61bd23487fbae5c63413e9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1984900
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Wei4 Wang <wei4.wang@intel.com>
Cr-Commit-Position: refs/heads/master@{#732749}

Co-authored-by: WangW_intel <54053982+wangw-1991@users.noreply.github.com>

--

wpt-commits: 3b8990ef1b99135ad02f13b3389049921054c88f
wpt-pr: 21046
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 23, 2020
…gistrations() by registration ID., a=testonly

Automatic update from web-platform-tests
Sort the registrations returned by getRegistrations() by registration ID. (#21046)

This patch makes the order of the registrations returned by
getRegistrations() be sorted by registration ID, which fixes the flaky
test of getregistrations.https.html. Related Github issue is in
w3c/ServiceWorker#1465

Bug: 925740
Change-Id: Iaa5716eec886232df61bd23487fbae5c63413e9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1984900
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Wei4 Wang <wei4.wang@intel.com>
Cr-Commit-Position: refs/heads/master@{#732749}

Co-authored-by: WangW_intel <54053982+wangw-1991@users.noreply.github.com>

--

wpt-commits: 3b8990ef1b99135ad02f13b3389049921054c88f
wpt-pr: 21046
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Jan 28, 2020
…gistrations() by registration ID., a=testonly

Automatic update from web-platform-tests
Sort the registrations returned by getRegistrations() by registration ID. (#21046)

This patch makes the order of the registrations returned by
getRegistrations() be sorted by registration ID, which fixes the flaky
test of getregistrations.https.html. Related Github issue is in
w3c/ServiceWorker#1465

Bug: 925740
Change-Id: Iaa5716eec886232df61bd23487fbae5c63413e9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1984900
Reviewed-by: Makoto Shimazu <shimazuchromium.org>
Commit-Queue: Wei4 Wang <wei4.wangintel.com>
Cr-Commit-Position: refs/heads/master{#732749}

Co-authored-by: WangW_intel <54053982+wangw-1991users.noreply.github.com>

--

wpt-commits: 3b8990ef1b99135ad02f13b3389049921054c88f
wpt-pr: 21046

UltraBlame original commit: 3cdd21c5e484de85c344fe475dbeadb697bf886c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jan 28, 2020
…gistrations() by registration ID., a=testonly

Automatic update from web-platform-tests
Sort the registrations returned by getRegistrations() by registration ID. (#21046)

This patch makes the order of the registrations returned by
getRegistrations() be sorted by registration ID, which fixes the flaky
test of getregistrations.https.html. Related Github issue is in
w3c/ServiceWorker#1465

Bug: 925740
Change-Id: Iaa5716eec886232df61bd23487fbae5c63413e9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1984900
Reviewed-by: Makoto Shimazu <shimazuchromium.org>
Commit-Queue: Wei4 Wang <wei4.wangintel.com>
Cr-Commit-Position: refs/heads/master{#732749}

Co-authored-by: WangW_intel <54053982+wangw-1991users.noreply.github.com>

--

wpt-commits: 3b8990ef1b99135ad02f13b3389049921054c88f
wpt-pr: 21046

UltraBlame original commit: 3cdd21c5e484de85c344fe475dbeadb697bf886c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants