Reland of [Background Sync] Allow sync manager access from uncontrolled clients

[Background Sync] Allow sync manager access from uncontrolled clients

This patch removes the requirement for the caller of a BackgroundSync
manager method to be either a service worker or a controlled client.
Now, uncontrolled clients are also able to access the manager, and Blink
informs the browser whether a registration request came from a Service
Worker or not. (Service Workers are still required to have at least one
active window client in order to register sync events; this is enforced
in the browser).

This is part 3 of a four-part blink-chromium-blink dance.
Part 1: https://codereview.chromium.org/1309393003
Part 2: https://codereview.chromium.org/1324903002
Part 3: https://codereview.chromium.org/1316743002 (this patch)
Part 4: https://codereview.chromium.org/1314453003

(This was originally landed as https://crrev.com/d7c429ff6d306e720d6b0e7b64be25c8efad82d8
Cr-Commit-Position: refs/heads/master@{#346459}
but was reverted.)

BUG=518884
Committed: https://crrev.com/6371c54e48ba7b74d6cd3f0d161f52e0a69b111a
Cr-Commit-Position: refs/heads/master@{#346719}

[iclelland, 2015-08-25 18:45:21]

iclelland@chromium.org changed reviewers:
+ jkarlin@chromium.org

[iclelland, 2015-08-25 18:46:27]

jkarlin: PTAL, thanks!

[jkarlin, 2015-08-26 12:57:36]

lgtm!

[iclelland, 2015-08-27 12:55:40]

iclelland@chromium.org changed reviewers:
+ palmer@chromium.org

[iclelland, 2015-08-27 13:09:01]

+r palmer -- Could you please review the change to the mojom file? Thanks!

[iclelland, 2015-08-31 18:54:10]

iclelland@chromium.org changed reviewers:
+ tsepez@chromium.org
- palmer@chromium.org

[iclelland, 2015-08-31 18:55:12]

-r palmer
+r tsepez -- for the change to the mojo interface
Thanks :)

[Tom Sepez, 2015-08-31 18:58:13]

https://codereview.chromium.org/1316743002/diff/20001/content/common/backgrou...
File content/common/background_sync_service.mojom (right):

https://codereview.chromium.org/1316743002/diff/20001/content/common/backgrou...
content/common/background_sync_service.mojom:22: bool
requested_from_service_worker)
Is this sent by both renderers and SWs?  If so, what happens if the untrusted
processes lies about this value?

[iclelland, 2015-08-31 19:16:29]

https://codereview.chromium.org/1316743002/diff/20001/content/common/backgrou...
File content/common/background_sync_service.mojom (right):

https://codereview.chromium.org/1316743002/diff/20001/content/common/backgrou...
content/common/background_sync_service.mojom:22: bool
requested_from_service_worker)
On 2015/08/31 18:58:13, Tom Sepez wrote:
> Is this sent by both renderers and SWs?  If so, what happens if the untrusted
> processes lies about this value?

This value is sent by the renderer, based on whether the request to register is
coming from a service worker thread (true) or a document/web-worker thread
(false). To prevent abuse of the BackgroundSync API, we want to restrict
registrations coming from service workers -- only workers with a controlled
document are allowed to register sync events. This stops a service worker from
being able to "chain" one-shot syncs into de facto periodic syncs, and also
doesn't force web developers to put all of their JS code into the document.

If a compromised renderer can lie about this, it could say 'false' when sending
a request, and the browser would allow a sync event to be registered for an
origin, even if there was no associated open document window. (All of the other
relevant permissions for that origin would still be checked, though; it is just
the "not from a SW" flag that is being trusted here).

(Also, if the compromised renderer has an open document at that origin, with an
installed service worker, then there is no change in behaviour; the registration
would be allowed anyway. This only matters if the service worker is running in
the background already)

[Tom Sepez, 2015-08-31 19:20:14]

Ok, sounds like the consequences are minor. LGTM.

[iclelland, 2015-08-31 19:22:40]

On 2015/08/31 19:20:14, Tom Sepez wrote:
> Ok, sounds like the consequences are minor. LGTM.

Thanks for the quick review!

[iclelland, 2015-08-31 19:22:53]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2015-08-31 19:22:53]

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

[commit-bot: I haz the power, 2015-08-31 19:23:06]

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

[commit-bot: I haz the power, 2015-08-31 20:40:11]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-31 20:40:53]

Patchset 2 (id:??) landed as
https://crrev.com/d7c429ff6d306e720d6b0e7b64be25c8efad82d8
Cr-Commit-Position: refs/heads/master@{#346459}

[Stephen Chennney, 2015-08-31 21:18:33]

On 2015/08/31 20:40:53, commit-bot: I haz the power wrote:
> Patchset 2 (id:??) landed as
> https://crrev.com/d7c429ff6d306e720d6b0e7b64be25c8efad82d8
> Cr-Commit-Position: refs/heads/master@{#346459}

This patch is causing the Blink layout test
http/tests/background_sync/oneshot.html to fail all over. Please investigate.

[Stephen Chennney, 2015-08-31 21:20:31]

On 2015/08/31 21:18:33, Stephen Chenney wrote:
> On 2015/08/31 20:40:53, commit-bot: I haz the power wrote:
> > Patchset 2 (id:??) landed as
> > https://crrev.com/d7c429ff6d306e720d6b0e7b64be25c8efad82d8
> > Cr-Commit-Position: refs/heads/master@{#346459}
> 
> This patch is causing the Blink layout test
> http/tests/background_sync/oneshot.html to fail all over. Please investigate.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...

Actually, it seems like just a few places for now, but the result is saying
FAIL.

[iclelland, 2015-08-31 22:06:28]

Thanks for the heads up -- I'll take a look at that and fix it tonight; I'm
pretty sure the test just needs to be updated, but I'll take care of it one
way or another.
On 31 Aug 2015 5:20 pm, <schenney@chromium.org> wrote:

> On 2015/08/31 21:18:33, Stephen Chenney wrote:
>
>> On 2015/08/31 20:40:53, commit-bot: I haz the power wrote:
>> > Patchset 2 (id:??) landed as
>> > https://crrev.com/d7c429ff6d306e720d6b0e7b64be25c8efad82d8
>> > Cr-Commit-Position: refs/heads/master@{#346459}
>>
>
> This patch is causing the Blink layout test
>> http/tests/background_sync/oneshot.html to fail all over. Please
>> investigate.
>>
>
>
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...
>
> Actually, it seems like just a few places for now, but the result is saying
> FAIL.
>
> https://codereview.chromium.org/1316743002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[iclelland, 2015-09-01 00:01:18]

On 2015/08/31 22:06:28, iclelland wrote:
> Thanks for the heads up -- I'll take a look at that and fix it tonight; I'm
> pretty sure the test just needs to be updated, but I'll take care of it one
> way or another.
> On 31 Aug 2015 5:20 pm, <mailto:schenney@chromium.org> wrote:
> 
> > On 2015/08/31 21:18:33, Stephen Chenney wrote:
> >
> >> On 2015/08/31 20:40:53, commit-bot: I haz the power wrote:
> >> > Patchset 2 (id:??) landed as
> >> > https://crrev.com/d7c429ff6d306e720d6b0e7b64be25c8efad82d8
> >> > Cr-Commit-Position: refs/heads/master@{#346459}
> >>
> >
> > This patch is causing the Blink layout test
> >> http/tests/background_sync/oneshot.html to fail all over. Please
> >> investigate.
> >>
> >
> >
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...
> >
> > Actually, it seems like just a few places for now, but the result is saying
> > FAIL.
> >
> > https://codereview.chromium.org/1316743002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

The layout test which was failing is now fixed in the third commit in this
series (https://codereview.chromium.org/1314453003/), which I should be able to
commit soon.

[mlamouri (slow - plz ping), 2015-09-01 10:12:00]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1317813003/ by mlamouri@chromium.org.

The reason for reverting is: This is breaking background_sync LayoutTests:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/53395

I guess part 3 would have fixed that but it doesn't seem to be ready to land..

[iclelland, 2015-09-01 19:59:49]

The CQ bit was checked by iclelland@chromium.org

[commit-bot: I haz the power, 2015-09-01 20:00:14]

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

[commit-bot: I haz the power, 2015-09-01 20:07:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-01 20:08:15]

Patchset 2 (id:??) landed as
https://crrev.com/6371c54e48ba7b74d6cd3f0d161f52e0a69b111a
Cr-Commit-Position: refs/heads/master@{#346719}

[iclelland, 2015-09-01 20:16:06]

On 2015/09/01 20:08:15, commit-bot: I haz the power wrote:
> Patchset 2 (id:??) landed as
> https://crrev.com/6371c54e48ba7b74d6cd3f0d161f52e0a69b111a
> Cr-Commit-Position: refs/heads/master@{#346719}

Relanded after https://codereview.chromium.org/1324903002 removed the failing
test. A test for the new behaviour is added in
https://codereview.chromium.org/1314453003.