ServiceWorker: Implement 'source' and 'origin' attributes of ExtendableMessageEvent

ServiceWorker: Implement 'source' and 'origin' attributes of ExtendableMessageEvent

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#extendablemessage-event-section
Intent-to-ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

Note: ExtendableMessageEvent is still behind a flag.

BUG=543198
Committed: https://crrev.com/0412b9e628ac0d7d5853b8b8dc41adfe6aae68f5
Cr-Commit-Position: refs/heads/master@{#380557}

[nhiroki, 2016-03-01 06:17:12]

Description was changed from

==========
ServiceWorker: Implement 'source' and 'origin' attributes of
ExtendableMessageEvent

BUG=543198
==========

to

==========
ServiceWorker: Implement 'source' and 'origin' attributes of
ExtendableMessageEvent

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

BUG=543198
==========

[nhiroki, 2016-03-01 08:23:31]

Description was changed from

==========
ServiceWorker: Implement 'source' and 'origin' attributes of
ExtendableMessageEvent

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

BUG=543198
==========

to

==========
ServiceWorker: Implement 'source' and 'origin' attributes of
ExtendableMessageEvent

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

Note: ExtendableMessageEvent is still behind a flag.

BUG=543198
==========

[nhiroki, 2016-03-01 08:29:47]

Patchset #1 (id:1) has been deleted

[nhiroki, 2016-03-01 08:29:57]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2016-03-01 08:30:07]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2016-03-01 08:30:15]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2016-03-01 08:30:28]

Patchset #1 (id:80001) has been deleted

[nhiroki, 2016-03-01 08:30:40]

Patchset #1 (id:100001) has been deleted

[nhiroki, 2016-03-01 08:31:07]

Patchset #1 (id:120001) has been deleted

[nhiroki, 2016-03-01 08:31:41]

Patchset #1 (id:140001) has been deleted

[nhiroki, 2016-03-01 08:31:51]

Patchset #1 (id:160001) has been deleted

[nhiroki, 2016-03-01 08:32:09]

Patchset #1 (id:180001) has been deleted

[nhiroki, 2016-03-01 08:33:55]

Patchset #1 (id:200001) has been deleted

[nhiroki, 2016-03-01 08:34:05]

Patchset #1 (id:220001) has been deleted

[nhiroki, 2016-03-01 08:34:16]

Patchset #1 (id:240001) has been deleted

[nhiroki, 2016-03-01 09:14:14]

Patchset #1 (id:260001) has been deleted

[nhiroki, 2016-03-01 09:28:28]

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

[nhiroki, 2016-03-01 09:28:30]

PTAL, thanks!

https://codereview.chromium.org/1701843002/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event-async-respond-with.html
(left):

https://codereview.chromium.org/1701843002/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event-async-respond-with.html:25:
frame.remove();
fyi: This frame should be removed after posting a message because |worker| is
associated with the frame and postMessage now checks its validity (see
ServiceWorker::postMessage).

https://codereview.chromium.org/1701843002/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event-respond-with-stops-propagation.html
(left):

https://codereview.chromium.org/1701843002/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-event-respond-with-stops-propagation.html:26:
frame.remove();
ditto.

[falken, 2016-03-02 09:02:48]

I think this lgtm!

https://codereview.chromium.org/1701843002/diff/300001/content/common/service...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/1701843002/diff/300001/content/common/service...
content/common/service_worker/service_worker_types.h:237:
ExtendableMessageEventSource();
Do you need this ctor?

https://codereview.chromium.org/1701843002/diff/300001/content/common/service...
content/common/service_worker/service_worker_types.h:242: 
Assuming the no-arg ctor is removed, can you comment that exactly one of these
infos is valid?

https://codereview.chromium.org/1701843002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/1701843002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp:56: }
Instead of keeping track of m_provider, would it be simpler to just compute it
here from contextExecution()?

[nhiroki, 2016-03-02 09:56:16]

Patchset #4 (id:340001) has been deleted

[nhiroki, 2016-03-02 09:57:12]

Thank you for reviewing!

https://codereview.chromium.org/1701843002/diff/300001/content/common/service...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/1701843002/diff/300001/content/common/service...
content/common/service_worker/service_worker_types.h:237:
ExtendableMessageEventSource();
On 2016/03/02 09:02:48, falken wrote:
> Do you need this ctor?

Yes. This is used by the IPC message generator.

https://codereview.chromium.org/1701843002/diff/300001/content/common/service...
content/common/service_worker/service_worker_types.h:242: 
On 2016/03/02 09:02:48, falken wrote:
> Assuming the no-arg ctor is removed, can you comment that exactly one of these
> infos is valid?

Added the comment.

https://codereview.chromium.org/1701843002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/1701843002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorker.cpp:56: }
On 2016/03/02 09:02:48, falken wrote:
> Instead of keeping track of m_provider, would it be simpler to just compute it
> here from contextExecution()?

Good point. Changed!

[nhiroki, 2016-03-02 09:57:26]

Description was changed from

==========
ServiceWorker: Implement 'source' and 'origin' attributes of
ExtendableMessageEvent

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

Note: ExtendableMessageEvent is still behind a flag.

BUG=543198
==========

to

==========
ServiceWorker: Implement 'source' and 'origin' attributes of
ExtendableMessageEvent

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

Note: ExtendableMessageEvent is still behind a flag.

BUG=543198
==========

[nhiroki, 2016-03-02 09:57:26]

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

[nhiroki, 2016-03-02 09:58:41]

+mkwst@, can you review following files?

- content/common/service_worker/service_worker_messages.h
- third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h
- third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp

Thanks

[Mike West, 2016-03-02 10:03:23]

The IPC changes look reasonable. One question about the origin.

https://codereview.chromium.org/1701843002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1701843002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:112:
RefPtr<SecurityOrigin> origin = SecurityOrigin::create(KURL(KURL(),
source->url()));
This seems like it's going to give you the wrong result in the case of sandboxed
documents. Have you considered passing a SecurityOrigin up through
`WebServiceWorkerClientInfo` rather than a URL?

[nhiroki, 2016-03-07 07:40:52]

Thank you for reviewing and sorry for my delayed reply...

https://codereview.chromium.org/1701843002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1701843002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:112:
RefPtr<SecurityOrigin> origin = SecurityOrigin::create(KURL(KURL(),
source->url()));
On 2016/03/02 10:03:23, Mike West wrote:
> This seems like it's going to give you the wrong result in the case of
sandboxed
> documents. Have you considered passing a SecurityOrigin up through
> `WebServiceWorkerClientInfo` rather than a URL?

Are you concerned about sandboxed documents that don't have 'allow-same-origin'
permission?

Such sandboxed documents cannot access to the entry point of Service Worker (ie.
navigator.serviceWorker) and cannot retrieve a ServiceWorker object to send a
message[1]. In addition, changing the attribute after the frame is loaded does
not take effect[2]. So, I think the current code would be safe (Am I missing
something?)

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[2]
https://html.spec.whatwg.org/multipage/embedded-content.html#attr-iframe-sandbox

[nhiroki, 2016-03-07 07:43:10]

falken@, can you take another look? I slightly changed the patchset to improve
spec conformance ("Client" -> "WindowClient"). I ran the tests on Firefox and
confirmed compatibility. Thanks.

[falken, 2016-03-07 09:40:32]

delta from patchset 3 still lgtm

[Mike West, 2016-03-08 11:02:03]

On 2016/03/07 at 07:40:52, nhiroki wrote:
> Thank you for reviewing and sorry for my delayed reply...
> 
>
https://codereview.chromium.org/1701843002/diff/360001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):
> 
>
https://codereview.chromium.org/1701843002/diff/360001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:112:
RefPtr<SecurityOrigin> origin = SecurityOrigin::create(KURL(KURL(),
source->url()));
> On 2016/03/02 10:03:23, Mike West wrote:
> > This seems like it's going to give you the wrong result in the case of
sandboxed
> > documents. Have you considered passing a SecurityOrigin up through
> > `WebServiceWorkerClientInfo` rather than a URL?
> 
> Are you concerned about sandboxed documents that don't have
'allow-same-origin' permission?

Yes.

> Such sandboxed documents cannot access to the entry point of Service Worker
(ie. navigator.serviceWorker) and cannot retrieve a ServiceWorker object to send
a message[1]. In addition, changing the attribute after the frame is loaded does
not take effect[2]. So, I think the current code would be safe (Am I missing
something?)
> 
> [1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> [2]
https://html.spec.whatwg.org/multipage/embedded-content.html#attr-iframe-sandbox

We accept Content-Security-Policy headers via meta tags, which can sandbox a
document at runtime (that is, `<meta http-equiv="Content-Security-Policy"
content="sandbox">` could be injected via JavaScript. Passing the actual origin
up as an origin rather than a URL would help you deal with this case.

[nhiroki, 2016-03-10 08:21:11]

Patchset #7 (id:420001) has been deleted

[nhiroki, 2016-03-10 08:54:22]

mkwst@: The latest patchset deals with sandboxed clients by passing client's
SecurityOrigin. Can you take another look? Thanks!

On 2016/03/08 11:02:03, Mike West wrote:
> On 2016/03/07 at 07:40:52, nhiroki wrote:
> > Thank you for reviewing and sorry for my delayed reply...
> > 
> >
>
https://codereview.chromium.org/1701843002/diff/360001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/1701843002/diff/360001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:112:
> RefPtr<SecurityOrigin> origin = SecurityOrigin::create(KURL(KURL(),
> source->url()));
> > On 2016/03/02 10:03:23, Mike West wrote:
> > > This seems like it's going to give you the wrong result in the case of
> sandboxed
> > > documents. Have you considered passing a SecurityOrigin up through
> > > `WebServiceWorkerClientInfo` rather than a URL?
> > 
> > Are you concerned about sandboxed documents that don't have
> 'allow-same-origin' permission?
> 
> Yes.
> 
> > Such sandboxed documents cannot access to the entry point of Service Worker
> (ie. navigator.serviceWorker) and cannot retrieve a ServiceWorker object to
send
> a message[1]. In addition, changing the attribute after the frame is loaded
does
> not take effect[2]. So, I think the current code would be safe (Am I missing
> something?)
> > 
> > [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > [2]
>
https://html.spec.whatwg.org/multipage/embedded-content.html#attr-iframe-sandbox
> 
> We accept Content-Security-Policy headers via meta tags, which can sandbox a
> document at runtime (that is, `<meta http-equiv="Content-Security-Policy"
> content="sandbox">` could be injected via JavaScript. Passing the actual
origin
> up as an origin rather than a URL would help you deal with this case.

Ah, I didn't know that meta-tag injection can immediately take effect. Thank you
for the explanation!

[Mike West, 2016-03-10 09:20:07]

IPC LGTM, thank you for taking another pass.

[nhiroki, 2016-03-10 14:39:57]

Patchset #9 (id:480001) has been deleted

[nhiroki, 2016-03-10 14:41:50]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2016-03-10 14:41:51]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1701843002/#ps500001
(title: "clean up tests")

[commit-bot: I haz the power, 2016-03-10 14:42:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701843002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701843002/500001

[commit-bot: I haz the power, 2016-03-10 15:25:35]

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

[commit-bot: I haz the power, 2016-03-10 15:25:37]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nhiroki, 2016-03-10 23:21:12]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-03-10 23:21:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701843002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701843002/500001

[commit-bot: I haz the power, 2016-03-11 00:27:55]

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

[commit-bot: I haz the power, 2016-03-11 00:27:57]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nhiroki, 2016-03-11 00:47:57]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-03-11 00:48:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701843002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701843002/500001

[commit-bot: I haz the power, 2016-03-11 01:54:51]

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

[commit-bot: I haz the power, 2016-03-11 01:54:53]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nhiroki, 2016-03-11 02:14:37]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-03-11 02:15:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701843002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701843002/500001

[commit-bot: I haz the power, 2016-03-11 03:20:49]

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

[commit-bot: I haz the power, 2016-03-11 03:20:51]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nhiroki, 2016-03-11 04:05:10]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-03-11 04:05:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701843002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701843002/500001

[commit-bot: I haz the power, 2016-03-11 05:14:45]

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

[commit-bot: I haz the power, 2016-03-11 05:14:46]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nhiroki, 2016-03-11 05:31:34]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-03-11 05:32:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1701843002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1701843002/500001

[commit-bot: I haz the power, 2016-03-11 06:33:40]

Description was changed from

==========
ServiceWorker: Implement 'source' and 'origin' attributes of
ExtendableMessageEvent

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

Note: ExtendableMessageEvent is still behind a flag.

BUG=543198
==========

to

==========
ServiceWorker: Implement 'source' and 'origin' attributes of
ExtendableMessageEvent

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

Note: ExtendableMessageEvent is still behind a flag.

BUG=543198
==========

[commit-bot: I haz the power, 2016-03-11 06:33:41]

Committed patchset #9 (id:500001)

[commit-bot: I haz the power, 2016-03-11 06:36:02]

Description was changed from

==========
ServiceWorker: Implement 'source' and 'origin' attributes of
ExtendableMessageEvent

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

Note: ExtendableMessageEvent is still behind a flag.

BUG=543198
==========

to

==========
ServiceWorker: Implement 'source' and 'origin' attributes of
ExtendableMessageEvent

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
Intent-to-ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fUPpDcnOi64

Note: ExtendableMessageEvent is still behind a flag.

BUG=543198

Committed: https://crrev.com/0412b9e628ac0d7d5853b8b8dc41adfe6aae68f5
Cr-Commit-Position: refs/heads/master@{#380557}
==========

[commit-bot: I haz the power, 2016-03-11 06:36:03]

Patchset 9 (id:??) landed as
https://crrev.com/0412b9e628ac0d7d5853b8b8dc41adfe6aae68f5
Cr-Commit-Position: refs/heads/master@{#380557}