Service Worker: Obey content settings when deciding to control a page

Service Worker: Obey content settings when deciding to control a page

When AllowServiceWorker is false, don't let a worker control a new page.
Existing controllees remain controlled until their tab closes.

BUG=419282
Committed: https://crrev.com/32f683f577e7155103f329a0a266b1db9126bc97
Cr-Commit-Position: refs/heads/master@{#299852}

[falken, 2014-10-08 09:04:13]

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

[falken, 2014-10-08 09:04:14]

Michael, PTAL. This depends on https://codereview.chromium.org/625533002/

[michaeln, 2014-10-10 22:26:32]

lgtm

https://codereview.chromium.org/637243003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/637243003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_controllee_request_handler.cc:192:
"Status", status);
I wonder if it makes sense to call host->SetAllowAssociation(false) here too and
leave it set that way for the duration, so later registration jobs won't be able
to graft a registration to this doc?

[falken, 2014-10-15 04:39:36]

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

[falken, 2014-10-15 04:39:36]

falken@chromium.org changed required reviewers:
+ jochen@chromium.org

[falken, 2014-10-15 04:39:37]

jochen: can you please review as content/browser OWNER?

https://codereview.chromium.org/637243003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/637243003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_controllee_request_handler.cc:192:
"Status", status);
On 2014/10/10 22:26:31, michaeln wrote:
> I wonder if it makes sense to call host->SetAllowAssociation(false) here too
and
> leave it set that way for the duration, so later registration jobs won't be
able
> to graft a registration to this doc?

Hmm, good question. But if AllowServiceWorker is on when register() is called,
it seems reasonable to let it associate with the doc?

Anyway the scenario seems unlikely: AllowServiceWorker would have to flip
between here and when register() is called (or register was already called and
queued up the job, then AllowServiceWorker flipped false, then the page load
starts, then the job runs). And this all happens before page load finishes.

Also the job only associates when it creates a new registration, which it
shouldn't since there's an existing one at this point.

[jochen (gone - plz use gerrit), 2014-10-15 15:26:09]

jochen@chromium.org changed required reviewers:
- jochen@chromium.org

[jochen (gone - plz use gerrit), 2014-10-15 15:26:10]

lgtm

https://codereview.chromium.org/637243003/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/637243003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_request_handler.cc:32:
ServiceWorkerRequestInterceptor(ResourceContext* resource_context)
explicit

[michaeln1, 2014-10-15 19:33:04]

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

[michaeln1, 2014-10-15 19:33:05]

https://codereview.chromium.org/637243003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/637243003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_controllee_request_handler.cc:192:
"Status", status);
On 2014/10/15 04:39:37, falken wrote:
> On 2014/10/10 22:26:31, michaeln wrote:
> > I wonder if it makes sense to call host->SetAllowAssociation(false) here too
> and
> > leave it set that way for the duration, so later registration jobs won't be
> able
> > to graft a registration to this doc?
>
> Also the job only associates when it creates a new registration, which it
> shouldn't since there's an existing one at this point.

Right, it doesn't make sense to do that.

[falken, 2014-10-16 01:41:41]

Thanks for the reviews. I've added explicit and also gave better TRACE_ log
output when its blocked.

[falken, 2014-10-16 01:41:47]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-10-16 01:43:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637243003/80001

[commit-bot: I haz the power, 2014-10-16 04:09:44]

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

[commit-bot: I haz the power, 2014-10-16 04:09:45]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[falken, 2014-10-16 04:18:09]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-10-16 04:18:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637243003/80001

[commit-bot: I haz the power, 2014-10-16 05:34:38]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-10-16 05:35:16]

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