CORS-RFC1918: Teach Workers about address spaces.

CORS-RFC1918: Teach Workers about address spaces.

https://codereview.chromium.org/1749153002 taught Documents about their
address spaces, but neglected Workers. Sorry, Workers!

This patch pipes address spaces through to WorkerGlobalState as follows:

1.  ServiceWorkers and SharedWorkers get their own address state from the
    Response which was used to create them.

2.  DedicatedWorkers and Worklets get their address state from the
    Document which created them.

BUG=591052
Committed: https://crrev.com/0cff8b37a25ec1c28aa0a388bd54522f2a47a6a2
Cr-Commit-Position: refs/heads/master@{#379292}

[Mike West, 2016-03-02 15:45:56]

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

[Mike West, 2016-03-02 15:45:56]

Hi Kinuko! Can you take a look at this patch? In particular, I'd love some
advice on testing: is there a good example of spinning up various kinds of
workers in unit tests so I could verify their properties?

Thanks!

[kinuko, 2016-03-03 02:46:34]

The code change looking good mod some nits. Reg: tests we don't have a very
convenient unig testing infra for various workers in blink right now... would
you teach me what needs to be checked/tested to see if the right addressspace is
set correctly?

https://codereview.chromium.org/1760523004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1760523004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp:884: 
Not related?

https://codereview.chromium.org/1760523004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/1760523004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:346:
document->addressSpace());
The document's completely bogus here, as it's just a shadow page created with
the same URL as the script URL-- doing the same as ServiceWorker might still
make some sense?

[Mike West, 2016-03-03 05:24:27]

On 2016/03/03 at 02:46:34, kinuko wrote:
> The code change looking good mod some nits. Reg: tests we don't have a very
convenient unig testing infra for various workers in blink right now... would
you teach me what needs to be checked/tested to see if the right addressspace is
set correctly?

Basically, I'd like to have tests that spin up various kinds of workers and
verify 1) that they have the correct address space, and 2) that the CSP
directive properly forces them into "public".

I guess I can do that with layout tests if I add some accessors to
`window.internals`, but I'd prefer not to go that route. It seems cleaner to put
things together in a unit test. *shrug*

https://codereview.chromium.org/1760523004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1760523004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp:116:
OwnPtr<WorkerThreadStartupData> startupData =
WorkerThreadStartupData::create(scriptURL, userAgent, sourceCode, nullptr,
startMode, csp->headers(), starterOrigin, m_workerClients.release(),
document->addressSpace());
This `document` is the actual document spinning up the worker, right?

https://codereview.chromium.org/1760523004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/1760523004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:346:
document->addressSpace());
On 2016/03/03 at 02:46:34, kinuko wrote:
> The document's completely bogus here, as it's just a shadow page created with
the same URL as the script URL-- doing the same as ServiceWorker might still
make some sense?

Yeah. This should probably read from the script loader. It's not clear to me
that that's the right thing to do, but I think it makes sense given that the
worker will be talking to multiple documents (and should be same-origin with
each).

[kinuko, 2016-03-03 08:15:49]

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

[kinuko, 2016-03-03 08:15:50]

> I guess I can do that with layout tests if I add some accessors to
> `window.internals`, but I'd prefer not to go that route. It seems cleaner to
put
> things together in a unit test. *shrug*

Hmm. For regular workers we usually extend WorkerThreadTest but in this case it
wouldn't cover the code that's being changed. If we had a unit test for
WorkerMessagingProxy that'd have worked but we don't.

For ServiceWorker it should be possible to write a test by extending
WebEmbeddedWorkerImplTest, but it doesn't cover other worker types...

Sorry for not being able to give a good answer here :( (CC-ing falken@ as he
might possibly have ideas)

https://codereview.chromium.org/1760523004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1760523004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp:116:
OwnPtr<WorkerThreadStartupData> startupData =
WorkerThreadStartupData::create(scriptURL, userAgent, sourceCode, nullptr,
startMode, csp->headers(), starterOrigin, m_workerClients.release(),
document->addressSpace());
On 2016/03/03 05:24:26, Mike West wrote:
> This `document` is the actual document spinning up the worker, right?

Yes, this one is the right parent one.

[Mike West, 2016-03-03 12:14:04]

I've added some layout tests and pulled out some of the refactoring which makes
them possible into https://codereview.chromium.org/1754713006. WDYT?

[Mike West, 2016-03-04 05:52:31]

mkwst@chromium.org changed reviewers:
+ philipj@opera.com

[Mike West, 2016-03-04 05:52:32]

The bots are red because of an exciting patch failure. Rebase before uploading,
kids!

I'll rebase and reupload when I'm at the office later this morning.

+philipj@, as he had questions about the worker side of the patch this one
depends on.

[kinuko, 2016-03-04 09:01:12]

Thanks for adding layout tests, the changes lgtm once bots become happy

https://codereview.chromium.org/1760523004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-sharedworker-basic.html
(right):

https://codereview.chromium.org/1760523004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/cors-rfc1918/addressspace-sharedworker-basic.html:60:

nit: trailing empty lines that look unnecessary

[philipj_slow, 2016-03-04 10:28:31]

Took a quick look, trust the other reviewers to handle this :)

[Mike West, 2016-03-04 14:30:13]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-03-04 14:30:14]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1760523004/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-03-04 14:30:25]

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

[Mike West, 2016-03-04 14:37:42]

Description was changed from

==========
CORS-RFC1918: Teach Workers about address spaces.

https://codereview.chromium.org/1749153002 taught Documents about their
address spaces, but neglected Workers. Sorry, Workers!

This patch pipes address spaces through to WorkerGlobalState as follows:

1.  ServiceWorkers get their own address state from the Response which
    was used to create them.

2.  SharedWorkers get their address state from the Document which created
    them. This might or might not be the right thing to do, but it seems
    reasonable.

3.  DedicatedWorkers and Worklets get their address state from the
    Document which created them. This is certainly the right thing to do.

BUG=591052
==========

to

==========
CORS-RFC1918: Teach Workers about address spaces.

https://codereview.chromium.org/1749153002 taught Documents about their
address spaces, but neglected Workers. Sorry, Workers!

This patch pipes address spaces through to WorkerGlobalState as follows:

1.  ServiceWorkers and SharedWorkers get their own address state from the
    Response which was used to create them.

2.  DedicatedWorkers and Worklets get their address state from the
    Document which created them.

BUG=591052
==========

[Mike West, 2016-03-04 14:37:55]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2016-03-04 14:38:01]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2016-03-04 14:38:15]

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

[commit-bot: I haz the power, 2016-03-04 15:55:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-04 15:57:20]

Description was changed from

==========
CORS-RFC1918: Teach Workers about address spaces.

https://codereview.chromium.org/1749153002 taught Documents about their
address spaces, but neglected Workers. Sorry, Workers!

This patch pipes address spaces through to WorkerGlobalState as follows:

1.  ServiceWorkers and SharedWorkers get their own address state from the
    Response which was used to create them.

2.  DedicatedWorkers and Worklets get their address state from the
    Document which created them.

BUG=591052
==========

to

==========
CORS-RFC1918: Teach Workers about address spaces.

https://codereview.chromium.org/1749153002 taught Documents about their
address spaces, but neglected Workers. Sorry, Workers!

This patch pipes address spaces through to WorkerGlobalState as follows:

1.  ServiceWorkers and SharedWorkers get their own address state from the
    Response which was used to create them.

2.  DedicatedWorkers and Worklets get their address state from the
    Document which created them.

BUG=591052

Committed: https://crrev.com/0cff8b37a25ec1c28aa0a388bd54522f2a47a6a2
Cr-Commit-Position: refs/heads/master@{#379292}
==========

[commit-bot: I haz the power, 2016-03-04 15:57:22]

Patchset 4 (id:??) landed as
https://crrev.com/0cff8b37a25ec1c28aa0a388bd54522f2a47a6a2
Cr-Commit-Position: refs/heads/master@{#379292}