Bypass ServiceWorker when the request originates from isolated world.

Bypass ServiceWorker when the request originates from isolated world.

After https://codereview.chromium.org/1192013003, when Chrome extension
sends XHR request to "chrome-extension://*" to get the contents in the
extension, the request goes to the page's ServiceWorker.

This is breaking some extensions. (crbug.com/517181)
And also it is causing ASSERT failure. (crbug.com/528818)

The requests initiated from extensions should not go to the ServiceWorker.
So this CL checks DOMWrapperWorld.isIsolatedWorld and sets the
skipServiceWorker flag to bypass the SW.

BUG=528818, 517181
Committed: https://crrev.com/1eedddee100558e29c08a599d4274609c581dc2a
Cr-Commit-Position: refs/heads/master@{#360533}

[horo, 2015-10-07 05:18:32]

horo@chromium.org changed reviewers:
+ hiroshige@chromium.org

[horo, 2015-10-07 05:19:59]

hiroshige@
Could you please review this?

[hiroshige, 2015-10-08 14:28:09]

Looks good. Can we add tests?

[horo, 2015-10-09 09:50:56]

On 2015/10/08 14:28:09, hiroshige wrote:
> Looks good. Can we add tests?

Added.

[horo, 2015-10-15 02:25:41]

On 2015/10/09 09:50:56, horo wrote:
> On 2015/10/08 14:28:09, hiroshige wrote:
> > Looks good. Can we add tests?
> 
> Added.

Ping?

[hiroshige, 2015-10-15 08:30:02]

lgtm with a nit.

https://codereview.chromium.org/1390943003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1390943003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:185: if
(!document.securityOrigin()->isSameSchemeHostPort(securityOrigin())) {
nit: Using |m_document| instead of |document| might be better.
|document| is only used for setting |m_document| and other code in
DocumentThreadableLoader uses |m_document|.

https://codereview.chromium.org/1390943003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:185: if
(!document.securityOrigin()->isSameSchemeHostPort(securityOrigin())) {
Just for memo:
This condition is also true for file:// origin, but I expect this is OK.

[hiroshige, 2015-10-15 08:31:32]

FYI.
blink_presubmit fails:
Found a bad license header in these files:
 
chrome/test/data/extensions/api_test/service_worker/content_script_fetch/background.js
\
 
chrome/test/data/extensions/api_test/service_worker/content_script_fetch/content_script.js
\
 
chrome/test/data/extensions/api_test/service_worker/content_script_fetch/controlled_page/sw.js

[horo, 2015-10-16 06:02:12]

horo@chromium.org changed reviewers:
+ asargent@chromium.org

[horo, 2015-10-16 06:02:12]

asargent@
Could you please review chrome/browser/extensions/*?

[asargent_no_longer_on_chrome, 2015-10-16 17:58:48]

lgtm

https://codereview.chromium.org/1390943003/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/service_worker/content_script_fetch/controlled_page/index.html
(right):

https://codereview.chromium.org/1390943003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/service_worker/content_script_fetch/controlled_page/index.html:1:
<script>
optional suggestion: you might consider moving all this code into a .js file so
that someday if someone were to work on tightening up the default
content-security policy to disallow inline script, they wouldn't have to modify
this file

[horo, 2015-10-19 03:54:39]

https://codereview.chromium.org/1390943003/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/service_worker/content_script_fetch/controlled_page/index.html
(right):

https://codereview.chromium.org/1390943003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/service_worker/content_script_fetch/controlled_page/index.html:1:
<script>
On 2015/10/16 17:58:48, Antony Sargent wrote:
> optional suggestion: you might consider moving all this code into a .js file
so
> that someday if someone were to work on tightening up the default
> content-security policy to disallow inline script, they wouldn't have to
modify
> this file

Done.

[horo, 2015-10-19 03:55:16]

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

[horo, 2015-10-19 03:55:17]

mkwst@
Could you please review
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp?

[Mike West, 2015-10-19 11:55:18]

https://codereview.chromium.org/1390943003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1390943003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:188: if
(!m_document.securityOrigin()->isSameSchemeHostPort(securityOrigin())) {
1. This doesn't have anything to do with isolated worlds, does it? You're just
comparing origins.

2. Why 'isSameSchemeHostPort'? What is the expected behavior for sandboxed
documents, for instance?

[horo, 2015-10-19 12:49:32]

https://codereview.chromium.org/1390943003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1390943003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:188: if
(!m_document.securityOrigin()->isSameSchemeHostPort(securityOrigin())) {
On 2015/10/19 11:55:18, Mike West (slow) wrote:
> 1. This doesn't have anything to do with isolated worlds, does it? You're just
> comparing origins.
> 
> 2. Why 'isSameSchemeHostPort'? What is the expected behavior for sandboxed
> documents, for instance?

I thought that if the fetch (and XHR) is called from the different isolated
world, m_resourceLoaderOptions.securityOrigin is different from the
SecurityOrigin of the document.
When XHR is called from Chrome extension's content script,
m_resourceLoaderOptions.securityOrigin is like "chrome-extension://xxxxx".

Could you please let me know what is the best way to know whether the request is
from the different isolated world?

[kinuko (google), 2015-10-19 14:57:30]

kinuko@google.com changed reviewers:
+ kinuko@google.com

[kinuko (google), 2015-10-19 14:57:30]

https://codereview.chromium.org/1390943003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1390943003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:188: if
(!m_document.securityOrigin()->isSameSchemeHostPort(securityOrigin())) {
On 2015/10/19 12:49:32, horo wrote:
> On 2015/10/19 11:55:18, Mike West (slow) wrote:
> > 1. This doesn't have anything to do with isolated worlds, does it? You're
just
> > comparing origins.
> > 
> > 2. Why 'isSameSchemeHostPort'? What is the expected behavior for sandboxed
> > documents, for instance?
> 
> I thought that if the fetch (and XHR) is called from the different isolated
> world, m_resourceLoaderOptions.securityOrigin is different from the
> SecurityOrigin of the document.
> When XHR is called from Chrome extension's content script,
> m_resourceLoaderOptions.securityOrigin is like "chrome-extension://xxxxx".
> 
> Could you please let me know what is the best way to know whether the request
is
> from the different isolated world?

Drive-by... I don't think isolated world has anything to do with security
origin, while it's possible to attach different security origin to an isolated
world. It should be possible to create an isolated script world that has the
same security origin, and it's vague what we want to do in that case from this
patch's description and comment.

[horo, 2015-11-05 03:36:46]

Description was changed from

==========
Bypass ServiceWorker when the request originates from the content script.

After https://codereview.chromium.org/1192013003, when Chrome extension
sends XHR request to "chrome-extension://*" to get the contents in the
extension, the request goes to the page's ServiceWorker.

This is breaking some extensions. (crbug.com/517181)
And also it is causing ASSERT failure. (crbug.com/528818)

The requests initiated from extensions should not go to the ServiceWorker.
So this CL checks the securityOrigin of the document and the script.
And if they are different, sets the skipServiceWorker flag to bypass the SW.

BUG=528818,517181
==========

to

==========
Bypass ServiceWorker when the request originates from isolated world.

After https://codereview.chromium.org/1192013003, when Chrome extension
sends XHR request to "chrome-extension://*" to get the contents in the
extension, the request goes to the page's ServiceWorker.

This is breaking some extensions. (crbug.com/517181)
And also it is causing ASSERT failure. (crbug.com/528818)

The requests initiated from extensions should not go to the ServiceWorker.
So this CL checks DOMWrapperWorld.isIsolatedWorld and sets the
skipServiceWorker flag to bypass the SW.

BUG=528818,517181
==========

[horo, 2015-11-05 03:39:14]

On 2015/10/19 14:57:30, kinuko (google) wrote:
>
https://codereview.chromium.org/1390943003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
(right):
> 
>
https://codereview.chromium.org/1390943003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:188: if
> (!m_document.securityOrigin()->isSameSchemeHostPort(securityOrigin())) {
> On 2015/10/19 12:49:32, horo wrote:
> > On 2015/10/19 11:55:18, Mike West (slow) wrote:
> > > 1. This doesn't have anything to do with isolated worlds, does it? You're
> just
> > > comparing origins.
> > > 
> > > 2. Why 'isSameSchemeHostPort'? What is the expected behavior for sandboxed
> > > documents, for instance?
> > 
> > I thought that if the fetch (and XHR) is called from the different isolated
> > world, m_resourceLoaderOptions.securityOrigin is different from the
> > SecurityOrigin of the document.
> > When XHR is called from Chrome extension's content script,
> > m_resourceLoaderOptions.securityOrigin is like "chrome-extension://xxxxx".
> > 
> > Could you please let me know what is the best way to know whether the
request
> is
> > from the different isolated world?
> 
> Drive-by... I don't think isolated world has anything to do with security
> origin, while it's possible to attach different security origin to an isolated
> world. It should be possible to create an isolated script world that has the
> same security origin, and it's vague what we want to do in that case from this
> patch's description and comment.

I thougt that isolated world is only used for Chrome extension's content script.
But it is wrong.
And comparing origins is not a good way to distinguish the isolated worlds.

I uploaded Patch Set 5 which checks DOMWrapperWorld.isIsolatedWorld() and call
request.setSkipServiceWorker().
Could you please review this?

[Mike West, 2015-11-05 08:27:27]

https://codereview.chromium.org/1390943003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1390943003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchManager.cpp:560:
request.setSkipServiceWorker(m_isIsolatedWorld);
Doesn't this override the setting in cases where we don't want to override it?
Like, we skip the SW for CORS preflights, right? We should continue skipping
those even when (especially when!) the preflight comes from a request in the
main world.

[horo, 2015-11-05 08:47:10]

https://codereview.chromium.org/1390943003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1390943003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchManager.cpp:560:
request.setSkipServiceWorker(m_isIsolatedWorld);
On 2015/11/05 08:27:27, Mike West wrote:
> Doesn't this override the setting in cases where we don't want to override it?
> Like, we skip the SW for CORS preflights, right? We should continue skipping
> those even when (especially when!) the preflight comes from a request in the
> main world.

No.
DocumentThreadableLoader calls createAccessControlPreflightRequest() to create a
request for CORS preflight, and createAccessControlPreflightRequest() set the
skip flag.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[horo, 2015-11-17 04:01:16]

ping?

[Mike West, 2015-11-17 18:51:17]

On 2015/11/17 at 04:01:16, horo wrote:
> ping?

Sorry for the delay, this fell off my radar completely. I've taken another pass,
and I think this is a pretty reasonable approach. LGTM.

[horo, 2015-11-18 01:43:31]

The CQ bit was checked by horo@chromium.org

[horo, 2015-11-18 01:43:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1390943003/#ps100001
(title: "rebase")

[horo, 2015-11-18 01:43:32]

The CQ bit was unchecked by horo@chromium.org

[horo, 2015-11-18 01:44:15]

The CQ bit was checked by horo@chromium.org

[horo, 2015-11-18 01:44:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1390943003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-18 01:46:23]

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

[commit-bot: I haz the power, 2015-11-18 02:02:33]

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

[commit-bot: I haz the power, 2015-11-18 02:02:33]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)

[horo, 2015-11-18 06:29:45]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-11-18 06:29:57]

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

[commit-bot: I haz the power, 2015-11-18 06:39:02]

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

[commit-bot: I haz the power, 2015-11-18 06:39:03]

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

[horo, 2015-11-18 08:06:31]

The CQ bit was checked by horo@chromium.org

[horo, 2015-11-18 08:06:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, mkwst@chromium.org, asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1390943003/#ps120001
(title: "add License comments")

[commit-bot: I haz the power, 2015-11-18 08:07:00]

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

[commit-bot: I haz the power, 2015-11-18 09:50:33]

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

[commit-bot: I haz the power, 2015-11-18 09:50:34]

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

[horo, 2015-11-18 22:40:39]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-11-18 22:42:46]

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

[commit-bot: I haz the power, 2015-11-18 23:38:51]

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

[commit-bot: I haz the power, 2015-11-18 23:38:52]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, no build URL)

[horo, 2015-11-18 23:52:53]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-11-18 23:54:16]

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

[commit-bot: I haz the power, 2015-11-19 01:02:47]

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

[commit-bot: I haz the power, 2015-11-19 01:02:48]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, no build URL)

[horo, 2015-11-19 01:39:22]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-11-19 01:40:51]

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

[commit-bot: I haz the power, 2015-11-19 02:00:08]

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

[commit-bot: I haz the power, 2015-11-19 02:00:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, no build URL)

[horo, 2015-11-19 02:58:49]

Patchset #8 (id:140001) has been deleted

[horo, 2015-11-19 03:12:55]

The CQ bit was checked by horo@chromium.org

[horo, 2015-11-19 03:12:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, mkwst@chromium.org, asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1390943003/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2015-11-19 03:13:36]

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

[commit-bot: I haz the power, 2015-11-19 05:59:34]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-11-19 06:00:40]

Patchset 8 (id:??) landed as
https://crrev.com/1eedddee100558e29c08a599d4274609c581dc2a
Cr-Commit-Position: refs/heads/master@{#360533}