ServiceWorker: Abort register() and getRegistration() on non-http(s) page

ServiceWorker: Abort register() and getRegistration() on non-http(s) page

Currently the ServiceWorker backend does not support register() and
getRegistration() calls on non-HTTP(s) page (eg, "file://"). Such calls
trigger a renderer crash because SWProviderHost does not have a valid
document URL and treats a request as a bad message.

This patch makes SWContainer abort such calls before sending an IPC message.


BUG=432048
TEST=run-webkit-tests fast/serviceworker/access-container-on-local-file.html

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

[nhiroki, 2014-11-12 08:47:45]

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

[nhiroki, 2014-11-12 08:48:01]

Hi falken@, can you review this? Thanks!

[falken, 2014-11-12 09:04:41]

I'm not sure the test is testing the new codepath? In fact I'm not sure a layout
test in http/ is possible since the resources will be served over http, no?

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
File LayoutTests/fast/serviceworker/access-container-on-local-file.html (right):

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
LayoutTests/fast/serviceworker/access-container-on-local-file.html:7: var script
= 'no-such-worker';
Doesn't this need to be a "file:///" URL? But then I think the test would pass
anyway by hitting the origin-mismatch code.

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
LayoutTests/fast/serviceworker/access-container-on-local-file.html:10:
assert_unreached('A returned promise should be rejected');
More grammatical is "The returned promise should be rejected", probably more
clear is just "register should fail"

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
LayoutTests/fast/serviceworker/access-container-on-local-file.html:11: },
function(e) {
Maybe we should assert on the error message contents, to be sure this test is
testing what it's intending to test?

[falken, 2014-11-12 09:05:21]

On 2014/11/12 09:04:41, falken wrote:
> I'm not sure the test is testing the new codepath? In fact I'm not sure a
layout
> test in http/ is possible since the resources will be served over http, no?

... I just realized this test is NOT in http/, sorry!

[falken, 2014-11-12 09:07:03]

LGTM

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
File LayoutTests/fast/serviceworker/access-container-on-local-file.html (right):

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
LayoutTests/fast/serviceworker/access-container-on-local-file.html:7: var script
= 'no-such-worker';
On 2014/11/12 09:04:41, falken wrote:
> Doesn't this need to be a "file:///" URL? But then I think the test would pass
> anyway by hitting the origin-mismatch code.

Please disregard.

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
LayoutTests/fast/serviceworker/access-container-on-local-file.html:11: },
function(e) {
On 2014/11/12 09:04:41, falken wrote:
> Maybe we should assert on the error message contents, to be sure this test is
> testing what it's intending to test?

I still think this may be a good idea, WDYT?

[nhiroki, 2014-11-12 09:42:09]

Thank you for reviewing!

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
File LayoutTests/fast/serviceworker/access-container-on-local-file.html (right):

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
LayoutTests/fast/serviceworker/access-container-on-local-file.html:10:
assert_unreached('A returned promise should be rejected');
On 2014/11/12 09:04:41, falken wrote:
> More grammatical is "The returned promise should be rejected", probably more
> clear is just "register should fail"

Done.

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
LayoutTests/fast/serviceworker/access-container-on-local-file.html:11: },
function(e) {
On 2014/11/12 09:07:03, falken wrote:
> On 2014/11/12 09:04:41, falken wrote:
> > Maybe we should assert on the error message contents, to be sure this test
is
> > testing what it's intending to test?
> 
> I still think this may be a good idea, WDYT?

+1! Added the check.

[nhiroki, 2014-11-12 14:05:08]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-11-12 14:06:15]

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

[commit-bot: I haz the power, 2014-11-12 15:06:24]

Committed patchset #3 (id:40001) as 185210

[falken, 2014-11-13 08:27:55]

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
File LayoutTests/fast/serviceworker/access-container-on-local-file.html (right):

https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
LayoutTests/fast/serviceworker/access-container-on-local-file.html:11: },
function(e) {
On 2014/11/12 09:42:09, nhiroki wrote:
> On 2014/11/12 09:07:03, falken wrote:
> > On 2014/11/12 09:04:41, falken wrote:
> > > Maybe we should assert on the error message contents, to be sure this test
> is
> > > testing what it's intending to test?
> > 
> > I still think this may be a good idea, WDYT?
> 
> +1! Added the check.

Ah, sorry. I just realized this was bad advice since other browsers won't use
the same error message.

[nhiroki, 2014-11-13 08:36:49]

On 2014/11/13 08:27:55, falken wrote:
>
https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
> File LayoutTests/fast/serviceworker/access-container-on-local-file.html
(right):
> 
>
https://codereview.chromium.org/722583002/diff/1/LayoutTests/fast/servicework...
> LayoutTests/fast/serviceworker/access-container-on-local-file.html:11: },
> function(e) {
> On 2014/11/12 09:42:09, nhiroki wrote:
> > On 2014/11/12 09:07:03, falken wrote:
> > > On 2014/11/12 09:04:41, falken wrote:
> > > > Maybe we should assert on the error message contents, to be sure this
test
> > is
> > > > testing what it's intending to test?
> > > 
> > > I still think this may be a good idea, WDYT?
> > 
> > +1! Added the check.
> 
> Ah, sorry. I just realized this was bad advice since other browsers won't use
> the same error message.

Ah, good point. I'll make a patch to remove those checks.