InternalHeaders::Get() may throw NS_ERROR_TYPE_ERR with an associated message. The semantics of ErrorResult dictate that the message needs to be consumed by the time that the object gets destroyed, so we need to clear it before returning in these two places.

The Vary header may include one or more HTTP header field names, so we need to extract those names here, similar to the way that the nsHttpChannel::ResponseWouldVary() function consumes the Vary header.

The Vary header may contain invalid header name values. We should just ignore such values as opposed to propagating them to the caller. Before this patch, attempts to add a request with such a Vary header for example would fail, since the internal QueryCache() call when executing the CachePutAllAction would fail.

Comment on attachment 8578675 [details] [diff] [review] Part 2: Support Vary headers including multiple header names in DOM Cache Review of attachment 8578675 [details] [diff] [review]: ----------------------------------------------------------------- Unfortunately this logic is duplicated in FetchPut.cpp because AddAll() has to do an in-memory implementation of the matching algorithm. (It does this to determine if a later request in the AddAll() will overwrite an earlier request in the AddAll()). We need to fix it there as well. ::: dom/cache/DBSchema.cpp @@ +819,5 @@ > for (uint32_t i = 0; i < varyValues.Length(); ++i) { > + // Extract the header names inside the Vary header value. > + nsAutoCString varyValue(varyValues[i]); > + char* rawBuffer = varyValue.BeginWriting(); > + char* token = nsCRT::strtok(rawBuffer, NS_HTTP_HEADER_SEPS, &rawBuffer); Can we unify this parsing code with the same logic in nsHttpChannel.cpp? Perhaps in nsNetUtil somewhere? Also, this logic needs to be fixed in the AddAll() case in FetchPut.cpp. @@ +825,5 @@ > + for (; token; > + token = nsCRT::strtok(rawBuffer, NS_HTTP_HEADER_SEPS, &rawBuffer)) { > + nsDependentCString header(token); > + if (header.EqualsLiteral("*")) { > + continue; This matches the spec, but is not what nsHttpChannel does. I question if the SW spec is correct now.

Comment on attachment 8578676 [details] [diff] [review] Part 3: Do not propagate errors in getting the headers to the outside world Review of attachment 8578676 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/DBSchema.cpp @@ +833,5 @@ > nsAutoCString queryValue; > queryHeaders->Get(header, queryValue, errorResult); > if (errorResult.Failed()) { > errorResult.ClearMessage(); > + continue; I think it would be easier to understand if we just set queryValue and cachedValue to EmptyCString() in this case. Then the comparison at the end of the loop does the right thing without trying to understand the extra branching. Right now you have to assume that both headers->Get() calls will fail for this to be correct.

Comment on attachment 8578677 [details] [diff] [review] Part 4: Add tests for handling of the Vary header in DOM Cache Review of attachment 8578677 [details] [diff] [review]: ----------------------------------------------------------------- I'm sorry, but I'm finding it really hard to follow this test. I think we need to be clearer about what is in the cache when we perform a query/mutation. For example, I find the way blink does vary testing easier to understand: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/cache-match-worker.js It clearly separates Cache population from the test steps. Its easy to understand whats in the Cache and reason about the expected result. I don't want to bikeshed, but I think we need something more like this for the vary tests. ::: dom/cache/test/mochitest/test_cache_match_vary.js @@ +39,5 @@ > +}).then(function(text) { > + responseStarUAText = text; > + return fetch(new Request(requestURL, {headers: {"WhatToVary": "Foo/Bar,User-Agent"}})); > +}).then(function(r) { > + responseStarFBUA = r; nit: Should this be called responseFBUA? There is no start in the header here. @@ +42,5 @@ > +}).then(function(r) { > + responseStarFBUA = r; > + return responseStarFBUA.text(); > +}).then(function(text) { > + responseStarFBUAText = text; nit: Remove Star from name? @@ +49,5 @@ > + responseRAE = r; > + return responseRAE.text(); > +}).then(function(text) { > + responseRAEText = text; > + return caches.open(name); nit: For clarity, can you indicate everything above this line is preparation of expected values and not actually testing the cache?

Comment on attachment 8578729 [details] [diff] [review] Part 5: Add tests for handling of ignoreVary Review of attachment 8578729 [details] [diff] [review]: ----------------------------------------------------------------- Dropping review here as I basically asked for a re-write on the last patch.

The Vary header may include one or more HTTP header field names, so we need to extract those names here, similar to the way that the nsHttpChannel::ResponseWouldVary() function consumes the Vary header.

The Vary header may contain invalid header name values. We should just ignore such values as opposed to propagating them to the caller. Before this patch, attempts to add a request with such a Vary header for example would fail, since the internal QueryCache() call when executing the CachePutAllAction would fail.

Comment on attachment 8578729 [details] [diff] [review] Part 5: Add tests for handling of ignoreVary I'll fold this back to the main test.

Not sure if this is exactly what you were looking for, but I hope you'd like this better.

Comment on attachment 8578968 [details] [diff] [review] Part 2: Support Vary headers including multiple header names in DOM Cache Review of attachment 8578968 [details] [diff] [review]: ----------------------------------------------------------------- I think we could factor these two methods out into a common one, but I won't make you do it now. Thanks for fixing this!

Comment on attachment 8578971 [details] [diff] [review] Part 3: Do not propagate errors in getting the headers to the outside world Review of attachment 8578971 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments. ::: dom/cache/DBSchema.cpp @@ +833,5 @@ > nsAutoCString queryValue; > queryHeaders->Get(header, queryValue, errorResult); > if (errorResult.Failed()) { > errorResult.ClearMessage(); > + queryValue.Truncate(); Can this simply be MOZ_ASSERT(queryValue.IsEmpty())? It seems headers should not touch it on error. ::: dom/cache/FetchPut.cpp @@ +385,5 @@ > nsAutoCString value; > + requestHeaders->Get(header, value, headerRv); > + if (NS_WARN_IF(headerRv.Failed())) { > + headerRv.ClearMessage(); > + value.Truncate(); MOZ_ASSERT(value.IsEmpty()) if possible.

Comment on attachment 8579006 [details] [diff] [review] Part 4: Add tests for handling of the Vary header in DOM Cache Review of attachment 8579006 [details] [diff] [review]: ----------------------------------------------------------------- There are probably some more combinations we are missing, but this is a huge improvement over the current situation. Thanks! ::: dom/cache/test/mochitest/test_cache_match_vary.js @@ +45,5 @@ > +} > + > +function testBasics() { > + var test; > + return setupTest({"WhatToVary": "Cookie"}) It would be nice to have a test case with: setupTest({"WhatToVary": "Cookie", "Cookie":"foo=bar" }) Right now it seems we are only checking the positive matching case when * is in the mix.

(In reply to Ben Kelly [:bkelly] from comment #16) > Comment on attachment 8578971 [details] [diff] [review] > Part 3: Do not propagate errors in getting the headers to the outside world > > Review of attachment 8578971 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with comments. > > ::: dom/cache/DBSchema.cpp > @@ +833,5 @@ > > nsAutoCString queryValue; > > queryHeaders->Get(header, queryValue, errorResult); > > if (errorResult.Failed()) { > > errorResult.ClearMessage(); > > + queryValue.Truncate(); > > Can this simply be MOZ_ASSERT(queryValue.IsEmpty())? It seems headers > should not touch it on error. Sure. I did this since you asked me to in comment 7. ;-)

(In reply to Ben Kelly [:bkelly] from comment #17) > It would be nice to have a test case with: > > setupTest({"WhatToVary": "Cookie", "Cookie":"foo=bar" }) > > Right now it seems we are only checking the positive matching case when * is > in the mix. Will do.

https://hg.mozilla.org/integration/mozilla-inbound/rev/958151e0cfd2 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea234420460 https://hg.mozilla.org/integration/mozilla-inbound/rev/edf516516fb3 https://hg.mozilla.org/integration/mozilla-inbound/rev/53bb870dcb5d https://hg.mozilla.org/integration/mozilla-inbound/rev/4c65990d4c58

https://hg.mozilla.org/mozilla-central/rev/958151e0cfd2 https://hg.mozilla.org/mozilla-central/rev/3ea234420460 https://hg.mozilla.org/mozilla-central/rev/edf516516fb3 https://hg.mozilla.org/mozilla-central/rev/53bb870dcb5d https://hg.mozilla.org/mozilla-central/rev/4c65990d4c58