[ServiceWorker] Treat opaque response as a network error for frame loading.

[ServiceWorker] Treat opaque response as a network error for frame loading.

Currently we can read the body of the opaque response using iframe's innerHTML.
To prevent this we have to treat the opaque response as a network error for the frame loading.

This CL depends on https://codereview.chromium.org/625513002/ and https://codereview.chromium.org/623663002/.

BUG=411174
TEST=LayoutTests/http/tests/serviceworker/fetch-frame-resource.html

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

[horo, 2014-10-02 11:32:18]

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

[horo, 2014-10-02 11:32:18]

falken@
Could you please review this?

[Mike West, 2014-10-02 12:07:46]

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

[Mike West, 2014-10-02 12:07:46]

Code change LGTM (I only skimmed the test) % one comment.

https://codereview.chromium.org/621003002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/RespondWithObserver.cpp (right):

https://codereview.chromium.org/621003002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/RespondWithObserver.cpp:126: if (responseType ==
FetchResponseData::OpaqueType && m_frameType != WebURLRequest::FrameTypeNone) {
You probably want to check for TopLevel or Nested explicitly. An opaque response
for a new window (auxiliary) might(?) be perfectly reasonable to open.

[horo, 2014-10-03 03:43:02]

https://codereview.chromium.org/621003002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/RespondWithObserver.cpp (right):

https://codereview.chromium.org/621003002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/RespondWithObserver.cpp:126: if (responseType ==
FetchResponseData::OpaqueType && m_frameType != WebURLRequest::FrameTypeNone) {
On 2014/10/02 12:07:46, Mike West (OOO until 6th) wrote:
> You probably want to check for TopLevel or Nested explicitly. An opaque
response
> for a new window (auxiliary) might(?) be perfectly reasonable to open.

It may reasonable. But the current ServiceWorker spec doesn't say anything about
auxiliary frame type.
It just says.
> If response.type is "opaque" and the context attribute is "navigate", then:
>  Set the respond-with error flag.
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#respond-with...
So I want to keep this as is for now.

[falken, 2014-10-03 03:43:47]

https://codereview.chromium.org/621003002/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/fetch-frame-resource.html (right):

https://codereview.chromium.org/621003002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/fetch-frame-resource.html:16:
async_test(function(t) {
It's burdensome, but can you put each test in its own file (probably in a
directory seviceworker/fetch)? Particularly with fetch, we have a several SLOW
and TIMEOUT tests. I think it's because we've been stuffing them all in a single
file like this.

https://codereview.chromium.org/621003002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/fetch-frame-resource.html:69:
setTimeout(function() { resolve(frame); }, 1000);
Especially with this timeout, we'll want separate files.

https://codereview.chromium.org/621003002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/fetch-frame-resource.html:140:
setTimeout(function() { resolve(win); }, 1000);
and this one

[horo, 2014-10-03 04:14:26]

https://codereview.chromium.org/621003002/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/fetch-frame-resource.html (right):

https://codereview.chromium.org/621003002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/fetch-frame-resource.html:16:
async_test(function(t) {
On 2014/10/03 03:43:47, falken wrote:
> It's burdensome, but can you put each test in its own file (probably in a
> directory seviceworker/fetch)? Particularly with fetch, we have a several SLOW
> and TIMEOUT tests. I think it's because we've been stuffing them all in a
single
> file like this.

I think we don't need to split.
Because async_tests are executed in parallel.
So if this test file TIMEOUT, there must be a bug in ServiceWorker.

[falken, 2014-10-03 06:06:40]

lgtm

https://codereview.chromium.org/621003002/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/fetch-frame-resource.html (right):

https://codereview.chromium.org/621003002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/fetch-frame-resource.html:16:
async_test(function(t) {
On 2014/10/03 04:14:26, horo wrote:
> On 2014/10/03 03:43:47, falken wrote:
> > It's burdensome, but can you put each test in its own file (probably in a
> > directory seviceworker/fetch)? Particularly with fetch, we have a several
SLOW
> > and TIMEOUT tests. I think it's because we've been stuffing them all in a
> single
> > file like this.
> 
> I think we don't need to split.
> Because async_tests are executed in parallel.
> So if this test file TIMEOUT, there must be a bug in ServiceWorker.

Acknowledged.

[horo, 2014-10-03 06:11:43]

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

[horo, 2014-10-03 06:11:44]

tkent@
Could you please review Source/web/ServiceWorkerGlobalScopeProxy.cpp?

[tkent, 2014-10-03 06:14:08]

lgtm

[horo, 2014-10-03 06:16:20]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-03 06:16:56]

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

[horo, 2014-10-03 08:02:08]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-10-03 08:03:39]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-03 08:04:11]

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

[commit-bot: I haz the power, 2014-10-03 10:25:45]

Committed patchset #1 (id:1) as 183186