Limit timeout for foreign fetch events when triggered by another service worker.

Limit timeout for foreign fetch events when triggered by another service worker.

BUG=648836
Committed: https://crrev.com/57c41f066cffadb2301263bda7811231d2db02f5
Cr-Commit-Position: refs/heads/master@{#436431}

[Marijn Kruisselbrink, 2016-11-18 20:32:16]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 20:33:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 23:43:20]

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

[commit-bot: I haz the power, 2016-11-18 23:43:21]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2016-12-01 20:14:06]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 20:14:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2016-12-01 20:32:22]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 20:32:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 22:03:06]

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

[commit-bot: I haz the power, 2016-12-01 22:03:07]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2016-12-02 00:12:01]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 00:12:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2016-12-02 00:15:02]

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

[Marijn Kruisselbrink, 2016-12-02 00:15:03]

https://codereview.chromium.org/2518523003/diff/40001/content/browser/service...
File content/browser/service_worker/foreign_fetch_request_handler_unittest.cc
(right):

https://codereview.chromium.org/2518523003/diff/40001/content/browser/service...
content/browser/service_worker/foreign_fetch_request_handler_unittest.cc:311: //
EXPECT_FALSE(InitializeHandler(kValidUrl, RESOURCE_TYPE_WORKER, nullptr /*
It surprised me a lot that ServiceWorkerUtils::IsMainResourceType does not
return true for dedicated workers or service workers. For whatever reason it
only considers shared workers to be main resources. Is that intentional? If so,
I guess I'll have to change my usage of it.

[commit-bot: I haz the power, 2016-12-02 02:59:43]

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

[commit-bot: I haz the power, 2016-12-02 02:59:43]

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

[Marijn Kruisselbrink, 2016-12-03 00:11:39]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 00:12:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 01:49:57]

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

[commit-bot: I haz the power, 2016-12-03 01:49:58]

Dry run: This issue passed the CQ dry run.

[falken, 2016-12-05 04:34:48]

lgtm

https://codereview.chromium.org/2518523003/diff/40001/content/browser/service...
File content/browser/service_worker/foreign_fetch_request_handler_unittest.cc
(right):

https://codereview.chromium.org/2518523003/diff/40001/content/browser/service...
content/browser/service_worker/foreign_fetch_request_handler_unittest.cc:311: //
EXPECT_FALSE(InitializeHandler(kValidUrl, RESOURCE_TYPE_WORKER, nullptr /*
On 2016/12/02 00:15:02, Marijn Kruisselbrink wrote:
> It surprised me a lot that ServiceWorkerUtils::IsMainResourceType does not
> return true for dedicated workers or service workers. For whatever reason it
> only considers shared workers to be main resources. Is that intentional? If
so,
> I guess I'll have to change my usage of it.

I think the reason is that IsMainResourceType really means: "this provider
should have a controller". SW controlling a dedicated worker doesn't look fully
supported yet, we don't actually create a provider host with TYPE_WORKER ever
(https://codereview.chromium.org/717553003) or expose navigator.serviceWorker
(https://bugs.chromium.org/p/chromium/issues/detail?id=371690). It looks like
the SW does intercept fetches from the dedicated worker though, probably loads
from the dedicated worker are plumbed to its owning page.

Service workers are never controlled by another service worker by design.

So I think as of now we still expect InitializeHandler to return false for
non-shared worker, non-frame. Would certainly be better to have comments
somewhere explaining this.

[Marijn Kruisselbrink, 2016-12-05 19:41:55]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 19:42:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marijn Kruisselbrink, 2016-12-05 20:46:52]

https://codereview.chromium.org/2518523003/diff/40001/content/browser/service...
File content/browser/service_worker/foreign_fetch_request_handler_unittest.cc
(right):

https://codereview.chromium.org/2518523003/diff/40001/content/browser/service...
content/browser/service_worker/foreign_fetch_request_handler_unittest.cc:311: //
EXPECT_FALSE(InitializeHandler(kValidUrl, RESOURCE_TYPE_WORKER, nullptr /*
On 2016/12/05 at 04:34:48, falken wrote:
> On 2016/12/02 00:15:02, Marijn Kruisselbrink wrote:
> > It surprised me a lot that ServiceWorkerUtils::IsMainResourceType does not
> > return true for dedicated workers or service workers. For whatever reason it
> > only considers shared workers to be main resources. Is that intentional? If
so,
> > I guess I'll have to change my usage of it.
> 
> I think the reason is that IsMainResourceType really means: "this provider
should have a controller". SW controlling a dedicated worker doesn't look fully
supported yet, we don't actually create a provider host with TYPE_WORKER ever
(https://codereview.chromium.org/717553003) or expose navigator.serviceWorker
(https://bugs.chromium.org/p/chromium/issues/detail?id=371690). It looks like
the SW does intercept fetches from the dedicated worker though, probably loads
from the dedicated worker are plumbed to its owning page.
> 
> Service workers are never controlled by another service worker by design.
> 
> So I think as of now we still expect InitializeHandler to return false for
non-shared worker, non-frame. Would certainly be better to have comments
somewhere explaining this.

Hmm, okay... It also somewhat looks like this IsMainResourceType is just a copy
of the same method in AppCache code. Anyway, I most certainly never intended for
foreign fetch to be able to intercept main resources for workers, so for now
I've just changed the check in ForeignFetchRequestHandler to explicitly not
handle those resource types. But it seems like it would make sense to rename
IsMainResourceType to not imply that it returns true for all main resource
types...

[Marijn Kruisselbrink, 2016-12-05 21:02:38]

The CQ bit was unchecked by mek@chromium.org

[Marijn Kruisselbrink, 2016-12-05 21:02:40]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-12-05 21:02:40]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2518523003/#ps80001
(title: "don't intercept worker main resource fetches")

[commit-bot: I haz the power, 2016-12-05 21:03:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 22:34:19]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480971760651060,
"parent_rev": "99eb6d362e1c077ef94f48c8dc6a264131ce6002", "commit_rev":
"0131faa07ae106d1e8facf8f374e686d64747231"}

[commit-bot: I haz the power, 2016-12-05 22:34:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-05 22:37:59]

Description was changed from

==========
Limit timeout for foreign fetch events when triggered by another service worker.

BUG=648836
==========

to

==========
Limit timeout for foreign fetch events when triggered by another service worker.

BUG=648836

Committed: https://crrev.com/57c41f066cffadb2301263bda7811231d2db02f5
Cr-Commit-Position: refs/heads/master@{#436431}
==========

[commit-bot: I haz the power, 2016-12-05 22:38:00]

Patchset 5 (id:??) landed as
https://crrev.com/57c41f066cffadb2301263bda7811231d2db02f5
Cr-Commit-Position: refs/heads/master@{#436431}