ServiceWorker: Reject overly long scope/script URLs

ServiceWorker: Reject overly long scope/script URLs

IPC enforces a limit on receipt, but we should check on the sending side
and reject the (un)registration promise too.

BUG=386724
R=falken@chromium.org, michaeln@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278850

[jsbell, 2014-06-20 00:01:07]

michaeln@, falken@ - please take a look?

Any other suggestions welcome:
* Should it be ErrorTypeSecurity or ...?
* Is the error message actually plumbed through? (Doesn't look like it from a
quick script test.)
* Better pattern for taking ownership of the callbacks object?
* Should firing onError be done asynchronously (i.e. via a PostTask) or is
synchronous here fine?
* Would we prefer layout tests on the Blink side, or ...?

[michaeln, 2014-06-20 00:50:34]

On 2014/06/20 00:01:07, jsbell wrote:
> michaeln@, falken@ - please take a look?
> 
> Any other suggestions welcome:
> * Should it be ErrorTypeSecurity or ...?

sgtm, unknown is the only other option

> * Is the error message actually plumbed through? (Doesn't look like it from a
> quick script test.)

hmmm, looks like the string gets eaten by ServiceWorkerError::from(...)


> * Better pattern for taking ownership of the callbacks object?
> * Should firing onError be done asynchronously (i.e. via a PostTask) or is
> synchronous here fine?

I was wondering about that too. I believe there are some other early return
paths within blnk prior to calling out to the embedded so i think this should be
fine. Testing should help answer this question.

> * Would we prefer layout tests on the Blink side, or ...?

Laytout tests seems like the best bet.

[falken, 2014-06-20 03:09:03]

lgtm

On 2014/06/20 00:50:34, michaeln wrote:
> On 2014/06/20 00:01:07, jsbell wrote:
> > michaeln@, falken@ - please take a look?
> > 
> > Any other suggestions welcome:
> > * Should it be ErrorTypeSecurity or ...?
> 
> sgtm, unknown is the only other option
> 
> > * Is the error message actually plumbed through? (Doesn't look like it from
a
> > quick script test.)
> 
> hmmm, looks like the string gets eaten by ServiceWorkerError::from(...)

Yes, I was wondering why we do that and what the best behavior is. Maybe we
ought to pass the message from the embedder as the unsanitizedMessage argument
to DOMException::create?

> 
> 
> > * Better pattern for taking ownership of the callbacks object?
> > * Should firing onError be done asynchronously (i.e. via a PostTask) or is
> > synchronous here fine?
> 
> I was wondering about that too. I believe there are some other early return
> paths within blnk prior to calling out to the embedded so i think this should
be
> fine. Testing should help answer this question.

I think synchronous should work, it looks like it would just be equivalent to
the early returns in ServiceWorkerContainer::registerServiceWorker.

> > * Would we prefer layout tests on the Blink side, or ...?
> 
> Laytout tests seems like the best bet.

[falken, 2014-06-20 03:09:22]

https://codereview.chromium.org/342163005/diff/1/content/child/service_worker...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/342163005/diff/1/content/child/service_worker...
content/child/service_worker/service_worker_dispatcher.cc:87:
scoped_ptr<WebServiceWorkerError>  error(
extra space here

https://codereview.chromium.org/342163005/diff/1/content/child/service_worker...
content/child/service_worker/service_worker_dispatcher.cc:108:
scoped_ptr<WebServiceWorkerError>  error(
ditto

[jsbell, 2014-06-20 17:05:26]

Thanks. I'll look at propagating the error and a layout test over on the blink
side.

https://codereview.chromium.org/342163005/diff/1/content/child/service_worker...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/342163005/diff/1/content/child/service_worker...
content/child/service_worker/service_worker_dispatcher.cc:87:
scoped_ptr<WebServiceWorkerError>  error(
On 2014/06/20 03:09:21, falken wrote:
> extra space here

Done.

https://codereview.chromium.org/342163005/diff/1/content/child/service_worker...
content/child/service_worker/service_worker_dispatcher.cc:108:
scoped_ptr<WebServiceWorkerError>  error(
On 2014/06/20 03:09:21, falken wrote:
> ditto

Done.

https://codereview.chromium.org/342163005/diff/1/content/child/service_worker...
content/child/service_worker/service_worker_dispatcher.cc:242:
scoped_ptr<WebServiceWorkerError>  error(
And fixed it too (which was what I copy/pasted)

[jsbell, 2014-06-20 17:05:32]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-06-20 17:10:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jsbell@chromium.org/342163005/20001

[commit-bot: I haz the power, 2014-06-20 20:25:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[commit-bot: I haz the power, 2014-06-20 20:36:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 20:36:34]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[jsbell, 2014-06-20 22:51:15]

Committed patchset #2 manually as r278850 (presubmit successful).