Add an Error class for ServiceWorkers

Add an Error class for ServiceWorkers

This class could eventually be generically exposed as WebError, and wrap DOMError.

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

[alecflett, 2013-10-15 00:23:19]

Here were move the 2nd parameter to the CallbackPromiseAdapter to be an error
message, which is mostly just a DOMError

[michaeln, 2013-10-15 01:28:47]

https://codereview.chromium.org/27278002/diff/1/Source/modules/serviceworkers...
File Source/modules/serviceworkers/ServiceWorkerError.h (right):

https://codereview.chromium.org/27278002/diff/1/Source/modules/serviceworkers...
Source/modules/serviceworkers/ServiceWorkerError.h:48: static
PassRefPtr<DOMError> from(WebType* worker)
looks like this should be returning a PassRefPtr<ServiceWorkerError>

|worker| as the param name is misleading

https://codereview.chromium.org/27278002/diff/1/public/platform/WebServiceWor...
File public/platform/WebServiceWorkerError.h (right):

https://codereview.chromium.org/27278002/diff/1/public/platform/WebServiceWor...
public/platform/WebServiceWorkerError.h:41: virtual const WebString& message() =
0;
It might be better/safer/easier-to-code-to to return the WebString on the stack
instead of a pointer/ref to it.

https://codereview.chromium.org/27278002/diff/1/public/platform/WebServiceWor...
public/platform/WebServiceWorkerError.h:42: virtual const WebString& name() = 0;
We'll have to be careful about the 'name' values.

I think error names are something that rightly should be defined in blink. Those
are very close to content facing api, a part of that api really. Given how this
is composed, chromium code has to pass in the right name. That seems sketchy and
a temptation to share string definitions would be icky from a linkage point of
view.

Should we consider a level of indirection here where we use numeric error codes
defined in WebKitAPI and have the blink/webkit layer fill in the appropiate
string value for the any underlying DOMError that gets cons'd up? In other cases
in the webkia api, in see enums used to define related set of errors.

[kinuko, 2013-10-15 01:43:09]

https://codereview.chromium.org/27278002/diff/1/Source/modules/serviceworkers...
File Source/modules/serviceworkers/ServiceWorkerError.h (right):

https://codereview.chromium.org/27278002/diff/1/Source/modules/serviceworkers...
Source/modules/serviceworkers/ServiceWorkerError.h:50: return
create(worker->name(), worker->message());
I'm a bit confused; does this imply WebServiceWorkerError's memory has to be
managed by the caller?

I.e. do you assume a usage like:

WebServiceWorkerErrorImpl error(name, message);
callbacks->onError(&error);

https://codereview.chromium.org/27278002/diff/1/public/platform/WebServiceWor...
File public/platform/WebServiceWorkerError.h (right):

https://codereview.chromium.org/27278002/diff/1/public/platform/WebServiceWor...
public/platform/WebServiceWorkerError.h:42: virtual const WebString& name() = 0;
Does this need to be a pure abstract virtual class?

https://codereview.chromium.org/27278002/diff/1/public/platform/WebServiceWor...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/27278002/diff/1/public/platform/WebServiceWor...
public/platform/WebServiceWorkerProvider.h:45: typedef
WebCallbacks<WebServiceWorker, WebServiceWorkerError> WebServiceWorkerCallbacks;
I think WebServiceWorkerCallbacks' memory management policy should be documented
here.

[kinuko, 2013-10-15 02:07:17]

https://codereview.chromium.org/27278002/diff/1/public/platform/WebServiceWor...
File public/platform/WebServiceWorkerError.h (right):

https://codereview.chromium.org/27278002/diff/1/public/platform/WebServiceWor...
public/platform/WebServiceWorkerError.h:42: virtual const WebString& name() = 0;
On 2013/10/15 01:28:48, michaeln wrote:
> We'll have to be careful about the 'name' values.
> 
> I think error names are something that rightly should be defined in blink.
Those
> are very close to content facing api, a part of that api really. Given how
this
> is composed, chromium code has to pass in the right name. That seems sketchy
and
> a temptation to share string definitions would be icky from a linkage point of
> view.
> 
> Should we consider a level of indirection here where we use numeric error
codes
> defined in WebKitAPI and have the blink/webkit layer fill in the appropiate
> string value for the any underlying DOMError that gets cons'd up? In other
cases
> in the webkia api, in see enums used to define related set of errors.

+1 to have numeric error codes (unless we have other better scheme which is
known to be non-error-prone and simple).  It's a bit old-fashioned, but
imagining how we'll be handling errors in blink (other than just passing it to
scripts via reject()) usually numeric error code works just fine, and especially
while we're in early development phase I'd like to have simple and solid error
reporting mechanism.

[alecflett, 2013-10-15 15:47:10]

Yeah I agree with pretty much everything here. Minor notes:

I debated the numeric codes, ended up tossing a coin for what you see here..
I'll switch it.

The reason I made ::from() return a DOMError is that there's already a
toV8(DOMError*) that we can leverage - we can do the numeric code ->
DOMError.name mapping inside there. It makes the ServiceWorkerError class seem
unnecessary right now, but I'm pretty sure we'll be constructing them blink-side
as well.

I don't have a strong preference on ownership which is why I left it ambiguous
here - it *seems* like the WK API tends to pass ownership but if kinuko's
example is ok, I'm ok with the embedder owning the error object.

new patch on the way.

[alecflett, 2013-10-15 16:34:01]

ok new patch is up, PTAL

One other minor note: In case it isn't obvious, I am explicitly choosing to
share the enums by exposing them in public/platform/WebServiceWorkerProvider.h
and just use them directly on the blink side. 

In IDB we had two sets of enums (blink side and WebKit API side) and managing
them was a big hassle for very little gain. (i.e. we still had to make 3-sided
patches to change them)

[kinuko, 2013-10-15 23:00:35]

On 2013/10/15 15:47:10, alecflett wrote:
> I don't have a strong preference on ownership which is why I left it ambiguous
> here - it *seems* like the WK API tends to pass ownership but if kinuko's
> example is ok, I'm ok with the embedder owning the error object.

Oops, sorry to be unclear, it was for just asking.
I also think passing ownership to blink would be simpler (and more common) here.

[kinuko, 2013-10-15 23:44:43]

https://codereview.chromium.org/27278002/diff/9001/Source/modules/servicework...
File Source/modules/serviceworkers/ServiceWorkerError.cpp (right):

https://codereview.chromium.org/27278002/diff/9001/Source/modules/servicework...
Source/modules/serviceworkers/ServiceWorkerError.cpp:39:
DEFINE_STATIC_LOCAL(AtomicString, registrationError, ("RegistrationError",
AtomicString::ConstructFromLiteral));
This is not thread safe, I think we could simply use const char* string
constants + switch/case (or const char* string array + array access by enum
code).

https://codereview.chromium.org/27278002/diff/9001/Source/modules/servicework...
File Source/modules/serviceworkers/ServiceWorkerError.h (right):

https://codereview.chromium.org/27278002/diff/9001/Source/modules/servicework...
Source/modules/serviceworkers/ServiceWorkerError.h:50: return
create(webError->errorType(), webError->message());
If this class takes ownership of webError we should delete webError after
creating (or pass it as PassOwnPtr to the instance).

https://codereview.chromium.org/27278002/diff/9001/public/platform/WebService...
File public/platform/WebServiceWorkerError.h (right):

https://codereview.chromium.org/27278002/diff/9001/public/platform/WebService...
public/platform/WebServiceWorkerError.h:45: virtual const WebString message() {
return WebString(); }
Do we want to have implementation of this interface in the embedder?  I'm
feeling that this could just provide real implementation and ctor like
WebServiceWorkerError(ErrorType error, const WebString& message).  Then chromium
side can just use the exported ctor & methods.  Some error code (like
WebSocketStreamError, WebGeolocationError (in public/web)) follows this pattern.

[alecflett, 2013-10-16 00:23:33]

So this new patch uses a struct rather than an abstract class, with concrete and
POD members.

It makes sense to me to pass ownership, so I'll get that in the next revision.
(along with the string issue)

I've got the chromium side of this working now too:
https://codereview.chromium.org/26442004

[alecflett, 2013-10-16 01:09:25]

ok, issues addressed.

[michaeln, 2013-10-16 01:19:15]

https://codereview.chromium.org/27278002/diff/9001/Source/modules/servicework...
File Source/modules/serviceworkers/ServiceWorkerError.cpp (right):

https://codereview.chromium.org/27278002/diff/9001/Source/modules/servicework...
Source/modules/serviceworkers/ServiceWorkerError.cpp:39:
DEFINE_STATIC_LOCAL(AtomicString, registrationError, ("RegistrationError",
AtomicString::ConstructFromLiteral));
this function looks iffy from a threading point of view, the references to the
objects returned have thread affinity :(

maybe...

String GetErrorName(...) {
  switch(e) {
  case FooError:
      return String("foo error name");
  }
}

https://codereview.chromium.org/27278002/diff/18001/public/platform/WebServic...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/27278002/diff/18001/public/platform/WebServic...
public/platform/WebServiceWorkerProvider.h:45: // In callbacks, the
WebServiceWorker and WebServiceWorkerError are owned by the caller and should
not be freed by the implementation of this class.
This interface is not making sense to me?

The error is a simple struct that wraps a number and a string, i get how that
works, struct goes on stack, error callback is invoked, boom done fine and good.

The worker ptr is a different matter. The class currently has no methods defined
on it, but that will change such that the class will have virtual methods
defined in the webkitapi and those methods will be overriden and implemented by
chrome... is that right? And the impl will have some state in it, at a minimum
the 'id' that idenfies the worker instance... is that right?

I don't see how chrome possibly knows when to delete it, i'm off to find other
code to see how this fits together?

[michaeln, 2013-10-16 01:24:42]

On 2013/10/16 01:19:15, michaeln wrote:
>
https://codereview.chromium.org/27278002/diff/9001/Source/modules/servicework...
> File Source/modules/serviceworkers/ServiceWorkerError.cpp (right):
> 
>
https://codereview.chromium.org/27278002/diff/9001/Source/modules/servicework...
> Source/modules/serviceworkers/ServiceWorkerError.cpp:39:
> DEFINE_STATIC_LOCAL(AtomicString, registrationError, ("RegistrationError",
> AtomicString::ConstructFromLiteral));
> this function looks iffy from a threading point of view, the references to the
> objects returned have thread affinity :(
> 
> maybe...
> 
> String GetErrorName(...) {
>   switch(e) {
>   case FooError:
>       return String("foo error name");
>   }
> }
> 
>
https://codereview.chromium.org/27278002/diff/18001/public/platform/WebServic...
> File public/platform/WebServiceWorkerProvider.h (right):
> 
>
https://codereview.chromium.org/27278002/diff/18001/public/platform/WebServic...
> public/platform/WebServiceWorkerProvider.h:45: // In callbacks, the
> WebServiceWorker and WebServiceWorkerError are owned by the caller and should
> not be freed by the implementation of this class.
> This interface is not making sense to me?
> 
> The error is a simple struct that wraps a number and a string, i get how that
> works, struct goes on stack, error callback is invoked, boom done fine and
good.
> 
> The worker ptr is a different matter. The class currently has no methods
defined
> on it, but that will change such that the class will have virtual methods
> defined in the webkitapi and those methods will be overriden and implemented
by
> chrome... is that right? And the impl will have some state in it, at a minimum
> the 'id' that idenfies the worker instance... is that right?
> 
> I don't see how chrome possibly knows when to delete it, i'm off to find other
> code to see how this fits together?

I see, the comment threw me off...
   static PassRefPtr<ServiceWorker> create(PassOwnPtr<WebKit::WebServiceWorker>
worker)

[kinuko, 2013-10-16 01:44:17]

Ok, let me give an earlier lgtm this time

https://codereview.chromium.org/27278002/diff/23001/Source/modules/servicewor...
File Source/modules/serviceworkers/ServiceWorkerError.h (right):

https://codereview.chromium.org/27278002/diff/23001/Source/modules/servicewor...
Source/modules/serviceworkers/ServiceWorkerError.h:51: delete webError;
Raw ptr + delete is usually disliked... you can catch the webError as OwnPtr at
the beginning of the method and let it delete.

OwnPtr<WebType> error = adoptPtr(webError);

https://codereview.chromium.org/27278002/diff/23001/Source/modules/servicewor...
Source/modules/serviceworkers/ServiceWorkerError.h:55: static String
GetErrorString(WebKit::WebServiceWorkerError::ErrorType);
nit: GetErrorString -> getErrorString

[kinuko, 2013-10-16 01:46:30]

more nits..

https://codereview.chromium.org/27278002/diff/23001/public/platform/WebServic...
File public/platform/WebServiceWorkerError.h (right):

https://codereview.chromium.org/27278002/diff/23001/public/platform/WebServic...
public/platform/WebServiceWorkerError.h:40: RegistrationError = 0,
This looks to be more like 'error category' than 'error type'.

Should this be instead ServiceDisabledError, NetworkError etc?

[alecflett, 2013-10-23 21:21:29]

abarth@ - need a quick review for public/platform

[abarth-chromium, 2013-10-23 21:25:28]

Why do you need to subclass DOMError?  I don't understand.

[abarth-chromium, 2013-10-23 21:25:59]

Without tests, it's difficult to understand what problem you're solving. 
Generally, you don't need to subclass DOMError.

[alecflett, 2013-10-24 23:07:44]

On 2013/10/23 21:25:59, abarth wrote:
> Without tests, it's difficult to understand what problem you're solving. 

Actually with this CL, it's the first time we can write a useful Layout Test.
Added to this patch.

> Generally, you don't need to subclass DOMError.

Oh good point. I guess I was anticipating that, but it's true at the moment we
don't need to.

(Going forward I suspect we will want to subclass DOMError for promise rejection
in lots of APIs in order to propagate additional data about the error)

[alecflett, 2013-10-24 23:08:49]

On 2013/10/24 23:07:44, alecflett wrote:
> On 2013/10/23 21:25:59, abarth wrote:
> > Without tests, it's difficult to understand what problem you're solving. 
> 
> Actually with this CL, it's the first time we can write a useful Layout Test.
> Added to this patch.
> 
> > Generally, you don't need to subclass DOMError.
> 
> Oh good point. I guess I was anticipating that, but it's true at the moment we
> don't need to.
> 
> (Going forward I suspect we will want to subclass DOMError for promise
rejection
> in lots of APIs in order to propagate additional data about the error)

Note that this does mean ServiceWorkerError is much more akin to a namespace
than a class, but AFAIK you can't use namespaces like classes in templates (i.e.
can't have a ServiceWorkerError::WebType if ServiceWorkerError is a namespace)

[michaeln, 2013-10-24 23:26:55]

https://codereview.chromium.org/27278002/diff/263001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/registration.html (right):

https://codereview.chromium.org/27278002/diff/263001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/registration.html:9: function
test_registerBasics() {
I'd suggest we not add this particular positive test until we're farther along.
The other error condition tests looks good for now since we have real code to
actually detect those error conditions.

Might want to make this registration_error_conditions.html?

In general, I'd vote to rely more heavily on unit test initially than on layout
tests which quickly turn into full blown system integration tests. We just have
no system yet to test.

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:122: if
(!documentOrigin->canRequest(scriptURL)) {
canRequest() doesn't look like the right test for these cases, this should be
some kind of origin equality test

[michaeln, 2013-10-25 00:45:00]

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:122: if
(!documentOrigin->canRequest(scriptURL)) {
On 2013/10/24 23:26:55, michaeln wrote:
> canRequest() doesn't look like the right test for these cases, this should be
> some kind of origin equality test

Was just looking thru the review comments in which this test landed. As coded a
document with universal access can register a service worker whose pattern camps
in one origin and whose script is retrieved from another. Maybe we can agree
that's not a desired outcome?

I think we want to use the isSameSchemeHostPort() here along the lines of:

  documentOrigin->isSameSchemeHostPort(SecurityOrigin::create(scriptSrc))

The semantics of that method most closely match the "same origin" policy which
is what we want here.

[abarth-chromium, 2013-10-25 15:37:15]

LGTM

We can continue to iterate on the security check in future CLs.  That issue
doesn't need to block this CL.

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
File Source/modules/serviceworkers/NavigatorServiceWorker.cpp (right):

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
Source/modules/serviceworkers/NavigatorServiceWorker.cpp:122: if
(!documentOrigin->canRequest(scriptURL)) {
On 2013/10/25 00:45:00, michaeln wrote:
> On 2013/10/24 23:26:55, michaeln wrote:
> > canRequest() doesn't look like the right test for these cases, this should
be
> > some kind of origin equality test
> 
> Was just looking thru the review comments in which this test landed. As coded
a
> document with universal access can register a service worker whose pattern
camps
> in one origin and whose script is retrieved from another. Maybe we can agree
> that's not a desired outcome?
> 
> I think we want to use the isSameSchemeHostPort() here along the lines of:
> 
>   documentOrigin->isSameSchemeHostPort(SecurityOrigin::create(scriptSrc))
> 
> The semantics of that method most closely match the "same origin" policy which
> is what we want here.

There is no such thing as equality comparison between origins.  You need to
think about what you want to happen in the asymmetric case because the
asymmetric case exists.

What security property are you trying to achieve?

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerError.cpp (right):

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerError.cpp:34: using
WebKit::WebServiceWorkerError;
Why do you have this using directive and then use the type with the explicit
WebKit:: namespace.  Either is fine, but doing both is redundant.

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerError.h (right):

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerError.h:48: return error;
error.release()

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerError.h:51: static String
getErrorString(WebKit::WebServiceWorkerError::ErrorType);
getErrorString -> errorString

Can this function be private?

[michaeln, 2013-10-25 21:59:06]

> We can continue to iterate on the security check in future CLs.  That issue
> doesn't need to block this CL.

No doubt.

> There is no such thing as equality comparison between origins.  You need to
> think about what you want to happen in the asymmetric case because the
> asymmetric case exists.

What is the asymmetric case?

> What security property are you trying to achieve?

The most significant property is that the url of invoking page, the pattern, and
the script must all have the same scheme/host/port. Any possible deviations from
that are special cases (which we can iterate on later).

[alecflett, 2013-10-25 23:05:32]

Thanks!
(I can't land this until https://codereview.chromium.org/25008006/ lands)

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerError.cpp (right):

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerError.cpp:34: using
WebKit::WebServiceWorkerError;
On 2013/10/25 15:37:16, abarth wrote:
> Why do you have this using directive and then use the type with the explicit
> WebKit:: namespace.  Either is fine, but doing both is redundant.

Done. (oops, oversight)

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerError.h (right):

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerError.h:48: return error;
On 2013/10/25 15:37:16, abarth wrote:
> error.release()

Done. (I was actually surprised to see the non-release() even compiled, now that
I think about it - would this have leaked, or does the explicit release just
avoid refcount churn?)

https://codereview.chromium.org/27278002/diff/263001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerError.h:51: static String
getErrorString(WebKit::WebServiceWorkerError::ErrorType);
On 2013/10/25 15:37:16, abarth wrote:
> getErrorString -> errorString
> 
> Can this function be private?

Done.

[abarth-chromium, 2013-10-26 15:44:09]

On 2013/10/25 21:59:06, michaeln wrote:
> > There is no such thing as equality comparison between origins.  You need to
> > think about what you want to happen in the asymmetric case because the
> > asymmetric case exists.
> 
> What is the asymmetric case?

The asymmetric case is when origin A is allowed to access origin B but origin B
isn't allowed to access origin A.  A simple example is Chrome extensions. 
There's a permission that grants an extension access to other origins via "host
permissions."

> > What security property are you trying to achieve?
> 
> The most significant property is that the url of invoking page, the pattern,
and
> the script must all have the same scheme/host/port. Any possible deviations
from
> that are special cases (which we can iterate on later).

What do you want to happen in cases where there isn't a scheme, host, or port
(e.g., in a sandboxed document with a unique origin)?  What if the authority
portion of the URL doesn't correspond to a host (e.g., for the chrome-extension
scheme it corresponds to a public key)?

Generally, it can be easier to describe security properties in terms of things
you want to prevent from happening.  What bad thing are you trying to prevent?

[abarth-chromium, 2013-10-26 15:44:36]

On 2013/10/25 23:05:32, alecflett wrote:
> Done. (I was actually surprised to see the non-release() even compiled, now
that
> I think about it - would this have leaked, or does the explicit release just
> avoid refcount churn?)

Just refcount churn.

[michaeln, 2013-10-28 20:28:36]

> What bad thing are you trying to prevent?

Pathological inputs that get written to disk. For the time being we can can
probably restrict this to support http and https only where its real easy to pin
down what same-origin means.

To answer your specific questions...

origin is unique --> security error
page is loaded via the chrome-extension scheme --> security error

The extension case is a good special case that we will probably want to cater to
in some way, but how exactly is not real clear just yet (doesn't need to be).

> The asymmetric case is when origin A is allowed to access origin B but origin
B
> isn't allowed to access origin A.  A simple example is Chrome extensions. 
> There's a permission that grants an extension access to other origins via
"host
> permissions."
> 
> > > What security property are you trying to achieve?
> > 
> > The most significant property is that the url of invoking page, the pattern,
> and
> > the script must all have the same scheme/host/port. Any possible deviations
> from
> > that are special cases (which we can iterate on later).
> 
> What do you want to happen in cases where there isn't a scheme, host, or port
> (e.g., in a sandboxed document with a unique origin)?  What if the authority
> portion of the URL doesn't correspond to a host (e.g., for the
chrome-extension
> scheme it corresponds to a public key)?
> 
> Generally, it can be easier to describe security properties in terms of things
> you want to prevent from happening.

[alecflett, 2013-10-28 20:37:28]

On 2013/10/28 20:28:36, michaeln wrote:
> > What bad thing are you trying to prevent?
> 
> Pathological inputs that get written to disk. For the time being we can can
> probably restrict this to support http and https only where its real easy to
pin
> down what same-origin means.

For the record, (not blocking this CL) this is something Michael and I disagree
on right now. I do not feel the need to restrict it in this way.

> 
> To answer your specific questions...
> 
> origin is unique --> security error

We disagree here as well. I think as long as the script and pattern are both in
the same origin, then it's fine if it's unique.

> page is loaded via the chrome-extension scheme --> security error
> 
> The extension case is a good special case that we will probably want to cater
to
> in some way, but how exactly is not real clear just yet (doesn't need to be).
> 

I agree about not catering to extensions yet, but expect to use and iterate on
the security checks we've just added.

Alec

[michaeln, 2013-10-28 21:02:30]

I'm not sure what security properties alec would like to achieve?

[abarth-chromium, 2013-10-29 05:29:07]

Ok, we should probably talk more in person, likely with a whiteboard.  It's only
going to be a one line change one way or another.  We just have to figure out
what exactly the correct line is.  :)

[kinuko, 2013-10-29 05:42:52]

On 2013/10/29 05:29:07, abarth wrote:
> Ok, we should probably talk more in person, likely with a whiteboard.  It's
only
> going to be a one line change one way or another.  We just have to figure out
> what exactly the correct line is.  :)

It's great if you guys could post a short discussion summary after you reach
consensus.  Fwiw my take is that starting with stricter same-scheme-host-port
model sounds ok/reasonable while canRequest looks to be the common model, and I
vote for throwing security error for unique origins (and I'm interested in why
Alec disagrees with this).  Thx,

[abarth-chromium, 2013-10-29 05:49:46]

My guess, by the way, is that you want to
SecurityOrigin::createFromURL(urlOfWorker)->canAccess(executionContext()).  We
just need to make a chart of the various cases and make sure that does what you
want in each case.

[kinuko, 2013-10-29 06:38:32]

On 2013/10/29 05:49:46, abarth wrote:
> My guess, by the way, is that you want to
> SecurityOrigin::createFromURL(urlOfWorker)->canAccess(executionContext()).  We
> just need to make a chart of the various cases and make sure that does what
you
> want in each case.

To me canRequest() looked mostly fine, except that: 1) allowing documents with
universal access to create a service worker for 'any' script worries me, and 2)
allowing cached blob URL won't make much sense in SW use cases.  canAccess
doesn't allow 2) so it looks closer but I'm not sure if we want to specifically
handle document.domain effect there (as documented in the code).  For universal
access it looks it's only used for testing and by trusted pepper API?  I need to
learn more about when this could get set.  (Yes, we should draw a chart-- pls
take a picture of the chart on whiteboard and share it with me later :))

[alecflett, 2013-10-29 22:05:15]

On 2013/10/29 05:42:52, kinuko wrote:
> On 2013/10/29 05:29:07, abarth wrote:
> > Ok, we should probably talk more in person, likely with a whiteboard.  It's
> only
> > going to be a one line change one way or another.  We just have to figure
out
> > what exactly the correct line is.  :)
> 
> It's great if you guys could post a short discussion summary after you reach
> consensus.  Fwiw my take is that starting with stricter same-scheme-host-port
> model sounds ok/reasonable while canRequest looks to be the common model, and
I
> vote for throwing security error for unique origins (and I'm interested in why
> Alec disagrees with this).  Thx,

I'm sorry I misunderstood the use of unique origins. It was my understanding
that a document with a unique origin could have subresources in that unique
origin. 

Now that I know that's not the case, I rescind that request.

the security property I'm looking for is for being as permissive as possible
without introducing security holes: 
- a document in one origin should not be able to register a service worker with
a pattern that refers to a "different" origin. 
- The script needs to be in a "compatible" origin with the pattern - meaning
from my perspective, the script should be allowed to be things like: 
  - a data url that was created in the document origin (is that even possible
though? not sure)
  - a filesystem url in the document origin
  - a bloburl for a blob that was created in the origin, etc. I just don't want
to willfully restrict this until we have to.
  - any other data that "belongs" in the origin - I don't know if this means
chrome extensions or not, but I wouldn't want to exclude them explicitly

[alecflett, 2013-10-29 22:09:12]

Spun this discussion off here:

https://github.com/slightlyoff/ServiceWorker/issues/113

[commit-bot: I haz the power, 2013-10-30 01:58:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/27278002/483001

[commit-bot: I haz the power, 2013-10-30 03:00:00]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-04 19:05:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/27278002/713001

[commit-bot: I haz the power, 2013-11-04 19:11:15]

Failed to trigger a try job on win_layout
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-11-04 19:11:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/27278002/763001

[commit-bot: I haz the power, 2013-11-04 20:20:31]

Change committed as 161275