[ServiceWorker] Disable the cache revalidation when the page is controlled by the ServiceWorker.

[ServiceWorker] Disable the cache revalidation when the page is controlled by the ServiceWorker.

The revalidation headers ("if-modified-since", "if-none-match") should not be exposed to the ServiceWorker.

BUG=429570
TEST=http/tests/serviceworker/resources/fetch-request-no-freshness-headers-iframe.html

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

[horo, 2014-11-05 06:21:49]

Patchset #1 (id:1) has been deleted

[horo, 2014-11-05 06:21:55]

Patchset #1 (id:20001) has been deleted

[horo, 2014-11-05 06:23:56]

Patchset #1 (id:40001) has been deleted

[horo, 2014-11-05 06:27:54]

Patchset #1 (id:60001) has been deleted

[horo, 2014-11-05 06:28:20]

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

[horo, 2014-11-05 06:28:21]

nhiroki@
Could you please review?

[nhiroki, 2014-11-05 07:38:35]

LGTM with nits

https://codereview.chromium.org/703813002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/fetch-request-no-freshness-headers.html
(right):

https://codereview.chromium.org/703813002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/fetch-request-no-freshness-headers.html:17:
return wait_for_state(t, sw, 'activated');
wait_for_activated()?

https://codereview.chromium.org/703813002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/fetch-request-no-freshness-headers.html:34:
});
nit: one more 2-space indent?

https://codereview.chromium.org/703813002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/fetch-request-no-freshness-headers-worker.js
(right):

https://codereview.chromium.org/703813002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/fetch-request-no-freshness-headers-worker.js:3:
self.onmessage = function(e) {
nit: Why don't you use addEventListener?

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

https://codereview.chromium.org/703813002/diff/80001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:1080: if
(existingResource->canUseCacheValidator() && !isControlledByServiceWorker())
Can we have a link to the issue and a comment about why we choose 'Reload'
policy if the request is going to be handled by SW?

[horo, 2014-11-05 08:48:03]

https://codereview.chromium.org/703813002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/fetch-request-no-freshness-headers.html
(right):

https://codereview.chromium.org/703813002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/fetch-request-no-freshness-headers.html:17:
return wait_for_state(t, sw, 'activated');
On 2014/11/05 07:38:35, nhiroki wrote:
> wait_for_activated()?

We have to use wait_for_update to get the serviceworker instance.

https://codereview.chromium.org/703813002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/fetch-request-no-freshness-headers.html:34:
});
On 2014/11/05 07:38:35, nhiroki wrote:
> nit: one more 2-space indent?

Done.

https://codereview.chromium.org/703813002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/fetch-request-no-freshness-headers-worker.js
(right):

https://codereview.chromium.org/703813002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/fetch-request-no-freshness-headers-worker.js:3:
self.onmessage = function(e) {
On 2014/11/05 07:38:35, nhiroki wrote:
> nit: Why don't you use addEventListener?

Done.

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

https://codereview.chromium.org/703813002/diff/80001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:1080: if
(existingResource->canUseCacheValidator() && !isControlledByServiceWorker())
On 2014/11/05 07:38:35, nhiroki wrote:
> Can we have a link to the issue and a comment about why we choose 'Reload'
> policy if the request is going to be handled by SW?

Done.

[horo, 2014-11-05 08:49:49]

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

[horo, 2014-11-05 08:49:50]

mkwst@
Could you please review Source/core/fetch/ResourceFetcher.cpp?

Thank you

[Mike West, 2014-11-05 09:29:32]

Makes sense to me. LGTM.

[Mike West, 2014-11-05 09:31:07]

On 2014/11/05 09:29:32, Mike West wrote:
> Makes sense to me. LGTM.

Hit send too quickly: this is good from a security perspective. You should land
it. That said, this will have performance impacts, right? Has that been raised
with Anne, Jake, Alex, et al?

[horo, 2014-11-05 10:40:43]

Thank you.

> That said, this will have performance impacts, right?
Yes.

If the page is not controlled by the ServiceWorker, the performance impacts are
very small (just calling isControlledByServiceWorker()).
If the page is controlled by the ServiceWorker, there is performance impacts
because we can't use the cache mechanism in Blink while reloading the page.
But we can use the HTTPCache in the browser process. So there is no network
performance impact.


> Has that been raised with Anne, Jake, Alex, et al?


Yes. This porblem is pointed out by jakearchibald@. crbug.com/429570

[horo, 2014-11-05 10:41:06]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-11-05 10:41:37]

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

[commit-bot: I haz the power, 2014-11-05 11:33:47]

Committed patchset #2 (id:100001) as 184864