Fetcher: Disable memory caching when Service Worker handles a request

Fetcher: Disable memory caching when Service Worker handles a request

MemoryCache can cache 1 resouce for 1 URL, but ServiceWorker can serve
arbitrary resources for 1 URL and pollute a memory cache entry.
As a stopgap solution, this change removes a cached resouce to be
controlled by Service Worker before requesting.

BUG=388375
TEST=manual (access a same resource from controlled/non-controlled documents on a same tab)
TEST=run_webkit_tests.py http/tests/serviceworker/chromium/memory-cache.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183534

[nhiroki, 2014-10-07 05:32:27]

Patchset #1 (id:1) has been deleted

[nhiroki, 2014-10-07 06:00:35]

Hi dominicc@ and michaeln@,

(This is JFYI and you don't have to review this for now.)

Let me share a workaround patch for the memory cache issue to reduce risk of the
P1 issue, just in case... This could degrade performance of resource requests
when Service Worker is enabled, so I'll try to make an alternative patch to
isolate memory cache entries for Service Worker as per the discussion on the
issue.

Thanks!

[nhiroki, 2014-10-08 13:50:49]

On 2014/10/07 06:00:35, nhiroki wrote:
> Let me share a workaround patch for the memory cache issue to reduce risk of
the
> P1 issue, just in case... This could degrade performance of resource requests
> when Service Worker is enabled, so I'll try to make an alternative patch to
> isolate memory cache entries for Service Worker as per the discussion on the
> issue.

Let me resume this patch... I'm making the patch to separate entries
(https://codereview.chromium.org/640463003/), but I think it needs more time to
implement and review carefully.

[nhiroki, 2014-10-08 14:22:09]

Patchset #3 (id:60001) has been deleted

[nhiroki, 2014-10-08 14:22:24]

Patchset #2 (id:40001) has been deleted

[nhiroki, 2014-10-08 14:26:28]

nhiroki@chromium.org changed reviewers:
+ japhet@chromium.org

[nhiroki, 2014-10-08 14:30:29]

Hi Nate, could you review this?

This is my first patch for this area, so I'd greatly appreciate any suggestion
or information. Thanks.

[nhiroki, 2014-10-08 14:31:01]

nhiroki@chromium.org changed reviewers:
+ gavinp@chromium.org, michaeln@chromium.org

[nhiroki, 2014-10-08 14:33:52]

+michaeln@ and gavinp@ for review from Service Worker POV.

[Nate Chapin, 2014-10-08 17:04:05]

LGTM, I guess.

I'm not thrilled with having ServiceWorker special cases down here, but I don't
see a good alternative.

Can we test this with a ResourceFetcherTest?

[michaeln, 2014-10-09 00:35:26]

Looks like this ensures pages using a sw will in fact get the requests handled
by that sw.

Does this prevent a serviceworker from poisoning the cache with a value that is
later used by a non-sw-controlled page?

https://codereview.chromium.org/632213002/diff/80001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/632213002/diff/80001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:773: if (frame() &&
frame()->loader().client()->isControlledByServiceWorker()) {
ouch, this is gonna hurt performance

[nhiroki, 2014-10-09 02:34:15]

Thank you for reviewing!

On 2014/10/09 00:35:26, michaeln wrote:
> Looks like this ensures pages using a sw will in fact get the requests handled
> by that sw.
> 
> Does this prevent a serviceworker from poisoning the cache with a value that
is
> later used by a non-sw-controlled page?

Yes, it does. This removes an existing cache(*) and doesn't cache a resource
controlled by SW, so non-sw-controlled page never use a cache populated by SW.

(*) I'm not sure we can fetch a resource to be controlled by SW with Load/Reload
policy without deleting an existing cache. If possible, we could avoid
performance impact on non-sw-controlled page.


Nate: Let me ask you questions...
1.) Do you have any ideas about (*)?
2.) Is there any way to measure performance of resource fetching and memory
caching?

https://codereview.chromium.org/632213002/diff/80001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/632213002/diff/80001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:773: if (frame() &&
frame()->loader().client()->isControlledByServiceWorker()) {
On 2014/10/09 00:35:26, michaeln wrote:
> ouch, this is gonna hurt performance 

Yes, that's a sore point of this change. I'm going to make sure the performance
impact.

[dominicc (has gone to gerrit), 2014-10-09 02:45:23]

dominicc@chromium.org changed reviewers:
+ dominicc@chromium.org

[dominicc (has gone to gerrit), 2014-10-09 02:45:24]

I support this change.

https://codereview.chromium.org/632213002/diff/80001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/632213002/diff/80001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:773: if (frame() &&
frame()->loader().client()->isControlledByServiceWorker()) {
On 2014/10/09 02:34:15, nhiroki wrote:
> On 2014/10/09 00:35:26, michaeln wrote:
> > ouch, this is gonna hurt performance 
> 
> Yes, that's a sore point of this change. I'm going to make sure the
performance
> impact.

Actually... why don't we land this as-is and concurrently investigate
performance and alternatives? It represents forward progress. The change is
precise. Changing it or backing it out later won't be hard. This is less subtle
than what AppCache does.

All we need is a good regression test which will support future efforts.

[nhiroki, 2014-10-09 14:42:07]

nhiroki@chromium.org changed reviewers:
+ horo@chromium.org

[nhiroki, 2014-10-09 14:44:29]

Added layout test instead of ResourceFetcherTest because apparently the unittest
can only test pre-filled resources and cannot make sure the fetcher never cache
a resource served by Service Worker (we could have a mock component for better
testing?).

+horo@: I think you are an appropriate person to review the test. Can you take a
look?

On 2014/10/09 02:45:24, dominicc wrote:
> Actually... why don't we land this as-is and concurrently investigate
> performance and alternatives? It represents forward progress. The change is
> precise. Changing it or backing it out later won't be hard. This is less
subtle
> than what AppCache does.
> 
> All we need is a good regression test which will support future efforts.

+1. It seems to be better to avoid a potential risk first because I guess this
performance issue could appear only in a few cases, that is, ServiceWorker is
controlling, browsing on the same renderer process and caching policy is 'Use'.

[jsbell, 2014-10-09 16:47:51]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2014-10-09 16:47:51]

I also support this change. :) Just a couple drive-by notes on the test.

Also, @gavinp's at an offsite, so he's unlikely to see this until tomorrow.

https://codereview.chromium.org/632213002/diff/140001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/memory-cache.html (right):

https://codereview.chromium.org/632213002/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/memory-cache.html:35: var
messageChannel = new MessageChannel();
Is this "ack" step actually necessary for the test, or can we just run the
controlled getJSON, and then the non-controlled getJSON?

https://codereview.chromium.org/632213002/diff/140001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/resources/memory-cache.json
(right):

https://codereview.chromium.org/632213002/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/resources/memory-cache.json:1:
callback({ src : 'network' })
Since this isn't actually JSON can we rename the file '.jsonp' or something, so
that it doesn't confuse editors/validators?

[Nate Chapin, 2014-10-09 17:56:22]

https://codereview.chromium.org/632213002/diff/140001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/632213002/diff/140001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:782:
memoryCache()->remove(resource.get());
Re: "(*) I'm not sure we can fetch a resource to be controlled by SW with
Load/Reload
policy without deleting an existing cache. If possible, we could avoid
performance impact on non-sw-controlled page."

Load means there is nothing to replace, so there's no harmful eviction happening
to begin with.

For Reload, you *might* be able to get the behavior you want by keeping a
ResourcePtr to the old resource, and re-add()ing it here. I haven't tested it,
though, so it's quite possible that will have undesirable side effects.

[michaeln, 2014-10-09 19:47:14]

> > Does this prevent a serviceworker from poisoning the cache with a value that
> > is later used by a non-sw-controlled page?
> 
> Yes, it does. This removes an existing cache(*) and doesn't cache a resource
> controlled by SW, so non-sw-controlled page never use a cache populated by SW.

wunderbar, ty!

[jsbell, 2014-10-09 23:24:59]

On 2014/10/09 16:47:51, jsbell wrote:
> Also, @gavinp's at an offsite, so he's unlikely to see this until tomorrow.

Oops, more than that - he's OOO for another whole week. Definitely ask other
reviewers if you had specific questions you were hoping gavin could answer.

[horo, 2014-10-10 02:25:13]

https://codereview.chromium.org/632213002/diff/140001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/memory-cache.html (right):

https://codereview.chromium.org/632213002/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/memory-cache.html:8: var callback;
Please write comments about |callback|.
"This callback function is called by appending a script element"

[nhiroki, 2014-10-10 06:16:48]

Patchset #7 (id:280001) has been deleted

[nhiroki, 2014-10-10 06:20:37]

Thank you, guys! Updated.

https://codereview.chromium.org/632213002/diff/140001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/memory-cache.html (right):

https://codereview.chromium.org/632213002/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/memory-cache.html:8: var callback;
On 2014/10/10 02:25:12, horo wrote:
> Please write comments about |callback|.
> "This callback function is called by appending a script element"

Done.

https://codereview.chromium.org/632213002/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/memory-cache.html:35: var
messageChannel = new MessageChannel();
On 2014/10/09 16:47:51, jsbell wrote:
> Is this "ack" step actually necessary for the test, or can we just run the
> controlled getJSON, and then the non-controlled getJSON? 

This messaging mechanism seems no longer necessary and I removed it.

At first, I think this is necessary to make sure...

1) the order of calling getJSON, and
2) making the subsequent request reuse the memory cache entry because some
resource type (eg. RawResource requested by XHR) is removed immediately after
requesting (I used XHR in my initial patch). As far as I tested, JSONP is not
applicable to that case.

https://codereview.chromium.org/632213002/diff/140001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/resources/memory-cache.json
(right):

https://codereview.chromium.org/632213002/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/resources/memory-cache.json:1:
callback({ src : 'network' })
On 2014/10/09 16:47:51, jsbell wrote:
> Since this isn't actually JSON can we rename the file '.jsonp' or something,
so
> that it doesn't confuse editors/validators?

Done.

https://codereview.chromium.org/632213002/diff/140001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/632213002/diff/140001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:782:
memoryCache()->remove(resource.get());
On 2014/10/09 17:56:22, Nate Chapin wrote:
> Re: "(*) I'm not sure we can fetch a resource to be controlled by SW with
> Load/Reload
> policy without deleting an existing cache. If possible, we could avoid
> performance impact on non-sw-controlled page."
> 
> Load means there is nothing to replace, so there's no harmful eviction
happening
> to begin with.
> 
> For Reload, you *might* be able to get the behavior you want by keeping a
> ResourcePtr to the old resource, and re-add()ing it here. I haven't tested it,
> though, so it's quite possible that will have undesirable side effects.

Thank you for your detailed answer! I'll take a look in a separate CL.

[horo, 2014-10-10 06:27:39]

lgtm with nit

https://codereview.chromium.org/632213002/diff/310001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/memory-cache.html (right):

https://codereview.chromium.org/632213002/diff/310001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/memory-cache.html:10: function
getJSON(url) {
nit: getJSONP is better.

https://codereview.chromium.org/632213002/diff/310001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/resources/memory-cache-controlled.html
(right):

https://codereview.chromium.org/632213002/diff/310001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/resources/memory-cache-controlled.html:4:
function getJSON(url) {
nit: getJSONP is better

[nhiroki, 2014-10-10 07:33:13]

Thanks! I'll push this into the CQ. Please let me know if there is anything you
concerned.

https://codereview.chromium.org/632213002/diff/310001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/memory-cache.html (right):

https://codereview.chromium.org/632213002/diff/310001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/memory-cache.html:10: function
getJSON(url) {
On 2014/10/10 06:27:38, horo wrote:
> nit: getJSONP is better.

Done.

https://codereview.chromium.org/632213002/diff/310001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/chromium/resources/memory-cache-controlled.html
(right):

https://codereview.chromium.org/632213002/diff/310001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/resources/memory-cache-controlled.html:4:
function getJSON(url) {
On 2014/10/10 06:27:38, horo wrote:
> nit: getJSONP is better

Done.

[nhiroki, 2014-10-10 07:33:25]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-10-10 07:34:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/632213002/520001

[commit-bot: I haz the power, 2014-10-10 11:11:58]

Committed patchset #8 (id:520001) as 183534