Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
294593002:
ServiceWorker: Support longest-prefix-match for registration scope (Closed)
|
Created:
6 years, 7 months ago by nhiroki Modified:
6 years, 6 months ago CC:
chromium-reviews, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+watch, alecflett+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionServiceWorker: Support longest-prefix-match for registration scope
Current implementation matches a registration in the first-match way, but the
spec[1] requires to match one in the longest-prefix-match way.
This CL introduces LongestScopeMatcher which compares candidate scopes in the
longest-prefix-match way.
[1] http://www.w3.org/TR/2014/WD-service-workers-20140508/
BUG=370773
TEST=content_unittests --gtest_filter=ServiceWorkerStorageTest.*
TEST=content_unittests --gtest_filter=ServiceWorkerUtilsTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275153
Patch Set 1 : #
Total comments: 17
Patch Set 2 : remake (drop the exact-match) #
Total comments: 8
Patch Set 3 : rebase #Patch Set 4 : add LongestScopeMatcher #Patch Set 5 : cleanup #
Total comments: 4
Patch Set 6 : rebase #Patch Set 7 : address comments #Patch Set 8 : fix #
Messages
Total messages: 22 (0 generated)
Hi, can you review this? This change obviously conflicts with your changes[1,2]. I'll rebase this after those changes are committed. [1] https://codereview.chromium.org/293483002/ [2] https://codereview.chromium.org/288693002/
https://codereview.chromium.org/294593002/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:411: if (result == ServiceWorkerUtils::SCOPE_EXACT_MATCH || The spec doesn't call out an exact match as a special case. This is reflected in the unit tests where "http://www.example.com/scope/foobar" is considered a better match than "http://www.example.com/scope/foobar*" This should either match the spec or we should file a spec bug and reference it from a comment here. https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:419: pattern_, host->pending_version()->scope()) >= 0) { If result here == 0 that would mean the scopes were the same - is there anything we can assert in that case? https://codereview.chromium.org/294593002/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_storage.cc:569: if (result == ServiceWorkerUtils::SCOPE_EXACT_MATCH) { Same as previous, exact vs. longest-wildcard https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_storage.cc:577: it->scope, match.scope) >= 0) { Same: if comparison == 0, the scopes must be the same; can we assert anything? https://codereview.chromium.org/294593002/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_storage_unittest.cc:451: // Registration for "/scope/fooba*". There's a lot of repeated code here - can this be refactored at all? https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_storage_unittest.cc:505: EXPECT_EQ(live_registration2, found_registration); See note about exact match vs. longest match. Per spec, I'd expect live_registration3 to win. https://codereview.chromium.org/294593002/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_utils.cc (right): https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_utils.cc:57: // Prioritize a scope which doesn't contain a wildcard (i.e. exact match). Again, not clear that prioritizing exact match over wildcard match is desired behavior per spec. https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_utils.cc:67: return scope1.spec().size() - scope2.spec().size(); unsigned - unsigned = unsigned; this only works because the (typically large) unsigned result is being cast to an int and truncates to negative - ISTR this is implementation dependent. Maybe cast before subtracting?
Thanks for your comments! https://codereview.chromium.org/294593002/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:411: if (result == ServiceWorkerUtils::SCOPE_EXACT_MATCH || On 2014/05/21 19:20:03, jsbell wrote: > The spec doesn't call out an exact match as a special case. > > This is reflected in the unit tests where "http://www.example.com/scope/foobar" > is considered a better match than "http://www.example.com/scope/foobar*" > > This should either match the spec or we should file a spec bug and reference it > from a comment here. Filed a spec bug: https://github.com/slightlyoff/ServiceWorker/issues/287 I'm thinking about dropping the exact-match case for now.
https://codereview.chromium.org/294593002/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:401: ServiceWorkerVersion* version) { when the input is NULL, this method should only affect those host that are current associated with this registration. as coded it can alter hosts that have an association to a different registration https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:413: host->SetPendingVersion(version); What if active_version is from an entirely different registration? I think we can't mix and match version from different registrations on the same host. If there is some assocation to a different registration on the document (pending or active)... i think this method needs to leave it alone. https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:419: pattern_, host->pending_version()->scope()) >= 0) { per above, once there is some association on place, i think we leave it alone for the life of that doc also, SetPendingVersion results in ipc traffic and events and such. We should pick one, then call that method once for a given host. As implemented, this loop can spam that method. https://codereview.chromium.org/294593002/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_storage.cc:569: if (result == ServiceWorkerUtils::SCOPE_EXACT_MATCH) { On 2014/05/21 19:20:03, jsbell wrote: > Same as previous, exact vs. longest-wildcard agree with jsbell length of scope trumps exact vs not
I have some questions inline. I'm not super familiar with this code so some of these might be obvious... https://codereview.chromium.org/294593002/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:413: host->SetPendingVersion(version); On 2014/05/22 00:56:29, michaeln wrote: > What if active_version is from an entirely different registration? I think we > can't mix and match version from different registrations on the same host. If > there is some assocation to a different registration on the document (pending or > active)... i think this method needs to leave it alone. FWIW there are some related comments on the spec bug about .active, .waiting properties <https://github.com/slightlyoff/ServiceWorker/issues/232> Maybe we as implementers should decide what makes sense here, make a specific proposal, and just optimistically implement that. I know we don't mix versions from different registrations, but why? What breaks if that happens? I am wondering if we're missing an object and that is what makes this hard to model. https://codereview.chromium.org/294593002/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_storage.cc:565: ServiceWorkerUtils::MatchResult result = From an API design point of view would it make sense for the caller to provide an iterator of URLs and have the method return the best match? Or have an object that you can feed URLs and ask it the best match? It looks to me like many callers will use this pattern and it is a lot of lines of code to produce it here. I'm not sure the caller needs to know about CompareScopePriorities, for example, that should be an implementation detail of the method/class/whatever. https://codereview.chromium.org/294593002/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_utils.h (right): https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_utils.h:38: // Returns SCOPE_EXACT_MATCH if |scope| exactly matches |url|. Returns I expect sometimes people call this with a URL and sometimes with a scope. Is it dangerous to represent these two things with different meanings with the same type? (I know the existing code did this, and cleaning it up would be a separate patch, just want to know what you think.)
https://codereview.chromium.org/294593002/diff/120001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/294593002/diff/120001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:413: host->SetPendingVersion(version); > FWIW there are some related comments on the spec bug about .active, .waiting > properties <https://github.com/slightlyoff/ServiceWorker/issues/232> > > Maybe we as implementers should decide what makes sense here, make a specific > proposal, and just optimistically implement that. > > I know we don't mix versions from different registrations, but why? What breaks > if that happens? I am wondering if we're missing an object and that is what > makes this hard to model. I think its the intent of the spec to not mix-n-match. Is there a compelling reason to mix registrations like that? Seems like it would add complexity for webdevs and brwsrdevs alike, and for something that isn't really wanted or needed. > what breaks If a page is associated with an intalled/active sw, that registration may itself be updating and something shows up in the 'downloading' or 'installing' state. But then if a new registration with a longer scope comes along, that too may result in a new version appearing in the 'downloading' or 'installing' state.
On 2014/05/23 01:23:27, michaeln wrote: > > I know we don't mix versions from different registrations, but why? What > breaks > > if that happens? I am wondering if we're missing an object and that is what > > makes this hard to model. > > I think its the intent of the spec to not mix-n-match. This is true. > Is there a compelling > reason to mix registrations like that? Seems like it would add complexity for > webdevs and brwsrdevs alike, and for something that isn't really wanted or > needed. I don't get this part, it seems vague. > > what breaks > > If a page is associated with an intalled/active sw, that registration may itself > be updating and something shows up in the 'downloading' or 'installing' state. > But then if a new registration with a longer scope comes along, that too may > result in a new version appearing in the 'downloading' or 'installing' state. This can happen anyway if there's an update pending and then there's yet another update. So the bit about having different contexts seems orthogonal. My gut at this point is that we should try to understand use cases better until we can crisply explain what the right thing to do here is.
Hmm... this CL seems to try to clear some issues: - Support the longest-prefix-match - Support (or not support) the mixed-registration-match - Clear a pending version associated with documents on activating To make things simpler, let me separate them into several CLs and focus on the longest-prefix-match support in this CL. I'll send review requests for them later.
On 2014/05/27 16:00:00, nhiroki wrote: > - Clear a pending version associated with documents on activating - Clear a pending version from associated documents on activating
> > I think its the intent of the spec to not mix-n-match. > > This is true. > > > Is there a compelling > > reason to mix registrations like that? Seems like it would add complexity for > > webdevs and brwsrdevs alike, and for something that isn't really wanted or > > needed. > > I don't get this part, it seems vague. I think not mixing-and-matching is simpler. Nobody is asking for mixing-and-matching. More clear? > > > what breaks > > > > If a page is associated with an intalled/active sw, that registration may > itself > > be updating and something shows up in the 'downloading' or 'installing' state. > > But then if a new registration with a longer scope comes along, that too may > > result in a new version appearing in the 'downloading' or 'installing' state. > > This can happen anyway if there's an update pending and then there's yet another > update. So the bit about having different contexts seems orthogonal. The registration/softupdate algos ensure no two versions are in the same state at the same time for a particular registration. Having two versions both in the downloading state at the same time cannot happen unless we mix-n-match and associate multiple registrations to a single document.
We're kind of abusing this code review for discussion, but the context is here so I'll bite... In terms of this review I think the registration/scope mixing problem can be tackled in later. I think it might be a bigger issue that demands more from callers of this function. On 2014/05/27 21:10:10, michaeln1 wrote: > > > I think its the intent of the spec to not mix-n-match. > > > > This is true. > > > > > Is there a compelling > > > reason to mix registrations like that? Seems like it would add complexity > for > > > webdevs and brwsrdevs alike, and for something that isn't really wanted or > > > needed. > > > > I don't get this part, it seems vague. > > I think not mixing-and-matching is simpler. Nobody is asking for > mixing-and-matching. More clear? "Simpler" and to not "add complexity" are about equally clear for me... One thing that not mixing scopes may make more complicated for web developers is that unless they squirrel away references to each Service Worker they observe, then without mixing scopes they may lose the reference to the built-in .installing, etc. Service Worker sooner. Now, I'm not sure that makes life difficult in practice. > > > > what breaks > > > > > > If a page is associated with an intalled/active sw, that registration may > > itself > > > be updating and something shows up in the 'downloading' or 'installing' > state. > > > But then if a new registration with a longer scope comes along, that too may > > > result in a new version appearing in the 'downloading' or 'installing' > state. > > > > This can happen anyway if there's an update pending and then there's yet > another > > update. So the bit about having different contexts seems orthogonal. > > The registration/softupdate algos ensure no two versions are in the same state > at the same time for a particular registration. I understand this to be true. > Having two versions both in the > downloading state at the same time cannot happen unless we mix-n-match and > associate multiple registrations to a single document. I might not understand what "state" refers to exactly but I had thought it meant roughly the values that the .state property takes. So I am not sure what you mean by downloading. To pop up a level, I think it is worth asking why a registration has a scope and what it does. Really what I am wondering about is Service Workers with different scopes appearing in the active/installing/etc. properties. I might have taken us down a rat hole by mentioning mixing registrations.
https://codereview.chromium.org/294593002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/294593002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_storage.cc:675: std::vector<GURL> scopes; Ugh. This approach is not going to scale well: * Make a copy of all registered scope URLs * Linear search over it * Given the result index, walk over the list again! We shouldn't rush to optimize, but maybe we can leave some TODOs indicating the code/storage should be restructured - e.g. pass a Callback into FindLongestScopeMatch. (Also, maybe we should partition the maps by origin?) https://codereview.chromium.org/294593002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_storage.cc:897: std::vector<GURL> scopes; Another TODO here - we should be able to do this w/o copying everything. https://codereview.chromium.org/294593002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_utils.cc (right): https://codereview.chromium.org/294593002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_utils.cc:42: if (!found || scopes.at(*pos) < scopes.at(i)) { This use of < here is weird. Do you mean to be comparing lengths? If so, you want > so longest match wins. If you're comparing the string contents, it would fail with this: url: "foo/bar" scope1: "foo/ba*" scope2: "foo/b*" Both match, scope2 < scope1, but scope1 is longer so should win.
https://codereview.chromium.org/294593002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_utils.cc (right): https://codereview.chromium.org/294593002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_utils.cc:42: if (!found || scopes.at(*pos) < scopes.at(i)) { On 2014/06/02 18:33:41, jsbell wrote: > This use of < here is weird. Do you mean to be comparing lengths? If so, you > want > so longest match wins. Uh, ignore this. My brain is on backwards today.
https://codereview.chromium.org/294593002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_utils.cc (right): https://codereview.chromium.org/294593002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_utils.cc:42: if (!found || scopes.at(*pos) < scopes.at(i)) { Ah, I did find a counterexample: url......: "foo/b(1)" scope@pos: "foo/b*" scope@i..: "foo/b(*" The longest match is scope@i. But scope@pos > scope@i, so scope@i will not be used. You should probably compare lengths.
hiroki has a real similar cl (identical) up for review seperately. https://codereview.chromium.org/304423004/ Since this one as the review history, lets use this one.
Thank you for reviewing. To optimize matching, I introduced LongestScopeMatcher. Can you take another look? Thanks! https://codereview.chromium.org/294593002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/294593002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_storage.cc:675: std::vector<GURL> scopes; On 2014/06/02 18:33:41, jsbell wrote: > Ugh. This approach is not going to scale well: > > * Make a copy of all registered scope URLs > * Linear search over it > * Given the result index, walk over the list again! Yeah... this is really inefficient way when there is a ton of installing registrations... > We shouldn't rush to optimize, but maybe we can leave some TODOs indicating the > code/storage should be restructured - e.g. pass a Callback into > FindLongestScopeMatch. > > (Also, maybe we should partition the maps by origin?) I'm taking a different approach "LongestScopeMatcher" in the latest patchset. This can resolve 1st and 3rd items on the list. Wdyt? https://codereview.chromium.org/294593002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_storage.cc:897: std::vector<GURL> scopes; On 2014/06/02 18:33:41, jsbell wrote: > Another TODO here - we should be able to do this w/o copying everything. Ditto. I replaced this with LongestScopeMatcher. It can save the copy cost. https://codereview.chromium.org/294593002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_utils.cc (right): https://codereview.chromium.org/294593002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_utils.cc:42: if (!found || scopes.at(*pos) < scopes.at(i)) { On 2014/06/02 19:31:22, jsbell wrote: > Ah, I did find a counterexample: > > url......: "foo/b(1)" > scope@pos: "foo/b*" > scope@i..: "foo/b(*" > > The longest match is scope@i. But scope@pos > scope@i, so scope@i will not be > used. You're right! > You should probably compare lengths. I changed this to compare lengths. Btw, there seems to be another 'wildcard-match' vs. 'exact-match' problem in this way. How should we prioritize scopes which have the same length like this? url: "/foo" scope1: "/fo*" scope2: "/foo" I'm not sure how we should prioritize them. I think there are some options: a) Prioritizing "exact-match" (i.e. scope2 wins). b) Picking up a latest one. c) Comparing them in other way (e.g. dictionary order). Wdyt? I mean to file a new issue or comment on the exist one[1] for spec conformance. [1] https://github.com/slightlyoff/ServiceWorker/issues/287
lgtm! Re: "/fo*" vs. "/foo" - I agree it should be defined in the spec. I don't have a strong preference. Following dominicc's guidance that we shouldn't block on the spec then yeah - we could say that collation order wins, i.e. if the lengths are the same then also compare the strings. That's dumb but easy and avoids possibly racy behavior based on registration order. https://codereview.chromium.org/294593002/diff/200002/content/browser/service... File content/browser/service_worker/service_worker_utils.cc (right): https://codereview.chromium.org/294593002/diff/200002/content/browser/service... content/browser/service_worker/service_worker_utils.cc:40: // for a document "/foo"? Add link to github spec issue? https://codereview.chromium.org/294593002/diff/200002/content/browser/service... File content/browser/service_worker/service_worker_utils.h (right): https://codereview.chromium.org/294593002/diff/200002/content/browser/service... content/browser/service_worker/service_worker_utils.h:8: #include <vector> Not needed any more?
lgtm2
On 2014/06/03 18:48:29, jsbell wrote: > Re: "/fo*" vs. "/foo" - I agree it should be defined in the spec. I don't have a > strong preference. Following dominicc's guidance that we shouldn't block on the > spec then yeah - we could say that collation order wins, i.e. if the lengths are > the same then also compare the strings. That's dumb but easy and avoids possibly > racy behavior based on registration order. Okay, I changed it to compare candidates as strings. Thanks! https://codereview.chromium.org/294593002/diff/200002/content/browser/service... File content/browser/service_worker/service_worker_utils.cc (right): https://codereview.chromium.org/294593002/diff/200002/content/browser/service... content/browser/service_worker/service_worker_utils.cc:40: // for a document "/foo"? On 2014/06/03 18:48:30, jsbell wrote: > Add link to github spec issue? Done. https://codereview.chromium.org/294593002/diff/200002/content/browser/service... File content/browser/service_worker/service_worker_utils.h (right): https://codereview.chromium.org/294593002/diff/200002/content/browser/service... content/browser/service_worker/service_worker_utils.h:8: #include <vector> On 2014/06/03 18:48:30, jsbell wrote: > Not needed any more? Done.
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/294593002/310001
Message was sent while issue was closed.
Change committed as 275153 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
