1333573 - WorkerPrivate should initialize mPrincipal and mPrincipalInfo sooner

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1293277 I am working on updating our service worker Clients API.  This API needs to create an entity based on the principal before the first network load is initiated.  Currently this is impossible because WorkerPrivate does not finish setting its mPrincipal and mPrincipalInfo values until after load when SetPrincipal() is called.

We should set the principal values to start and then ensure they only change in valid ways.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make WorkerPrivate use an nsMainThreadPtrHandle for its nsIPrincipal. r=baku

The first step is to hold the principal in an nsMainThreadPtrHandle.  This will allow us to adopt the principal in child workers even though we are not on the main thread.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Give nested workers an initial principal based on parent. r=baku

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Give nested workers an initial principal based on parent. r=baku

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Initialize mPrincipalInfo when setting WorkerPrivate's mPrincipal. r=baku

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8830470 [details] [diff] [review]
P1 Make WorkerPrivate use an nsMainThreadPtrHandle for its nsIPrincipal. r=baku

I think I see a better way to go here.  Unfortunately that means starting from scratch, though.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Factor setting the worker principal out into a separate method. r=baku

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Move principal setting to before AsyncOpen2. r=baku

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 MOZ_DIAGNOSTIC_ASSERT that the final worker script principal has not changed. r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75d3912a742ca990216164a6823e769074241bed

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8831316 [details] [diff] [review]
P1 Factor setting the worker principal out into a separate method. r=baku

Andrea, this is a refactoring that moves some logic out into a separate method.  It will help make the next patches easier to read.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8831317 [details] [diff] [review]
P2 Move principal setting to before AsyncOpen2. r=baku

Andrea, Christoph,

This patch makes us get the channel principal *before* the AsyncOpen2() call.  I believe this is safe to do because the security policies in play do not permit cross-origin redirects.

Even HSTS upgrades should be blocked because of the same-origin policy check in AsyncOpen2().  Either we're already a secure origin and the http:// script is blocked as cross-origin.  Or we are an insecure origin which means HSTS upgrades are not in play (since they apply to the whole domain).

I am making this change because I need a principal for the worker-to-be-created before the network request occurs.  This is to support the "reserved Client" concept in service workers.  The Client needs to only be accessible to same-origin things, so I need to have a principal to represent that origin.

The next patch will add a diagnostic assert verifying that such a redirect does not happen.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8831318 [details] [diff] [review]
P3 MOZ_DIAGNOSTIC_ASSERT that the final worker script principal has not changed. r=baku

This patch validates that the principal has not changed.  Either we began and ended as a null principal or our principal is exactly the same.  It seems sometimes a new null principal object can be minted by the network channel, but I believe its ok as long as we have a null principal at the end.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8831318 [details] [diff] [review]
P3 MOZ_DIAGNOSTIC_ASSERT that the final worker script principal has not changed. r=baku

Review of attachment 8831318 [details] [diff] [review]:
-----------------------------------------------------------------

Ben, I suppose this patch should be P2 (and your P2 should be P3), right? Anyway, these changes look fine from an AsyncOpen2() point of view. Especially because nothing changes outside of the worker code scope and I suppose the channel/principal for the worker is never going to change (also your assertion verifies that). I leave it up to baku to verify that once again because he knows the worker code much better than I do.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Ah, this is still not early enough for when I need it.  I will add further patches to move setting the principal even earlier.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8831316 [details] [diff] [review]
P1 Factor setting the worker principal out into a separate method. r=baku

Might need to make bigger changes here unfortunately.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

I think I have another set of patches that will work.  Will post for review later tonight or tomorrow.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Move WorkerPrivate::SetPrincipal() logic into a WorkerLoadInfo method. r=baku

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make nested worker pass WorkerLoadInfo to main thread when getting channel. r=baku

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Move logic to set principal from channel into WorkerLoadInfo. r=baku

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Set WorkerLoadInfo principal as soon as we get the channel. r=baku

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Validate that final channel load principal does not change. r=baku

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d86b9163ebb3a1c1ec9534b8dec086a88983aabe

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Need a few more patches here.

Problem 1:  For nested workers we might try to free main-thread only objects on the parent worker thread if there is a failure.  We need to doom them appropriately.

Problem 2:  A system principal loading non-resource URLs should fail.  They still do, but we now fail them synchronously instead of async like we used to.  I think this is ok and I'll just update the test.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Move ForgetMainThreadObjects() into WorkerLoadInfo. r=baku

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Release WorkerLoadInfo data on main thread if we fail nested worker creation. r=baku

These two patches fix the first problem with release objects off the main thread.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Update test_chromeWorkerJSM.xul to expect synchronous SecurityError. r=baku

This updates the test to expect earlier failure.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3cc474651d0e6282eaafa83f1ccf36aca5473d0

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832556 [details] [diff] [review]
P1 Move WorkerPrivate::SetPrincipal() logic into a WorkerLoadInfo method. r=baku

So, in order to set the principal early enough we need to operate on the WorkerLoadInfo instead of the WorkerPrivate.  Therefore we start by refactoring the code to move WorkerPrivate::SetPrincipal() to WorkerLoadInfo::SetPrincipalOnMainThread().

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832558 [details] [diff] [review]
P2 Make nested worker pass WorkerLoadInfo to main thread when getting channel. r=baku

We're going to need to set the principal immediately after the main script channel is created.  This must be done on the main thread.  Currently, though, nested workers just get the channel in a sync loop and return to the parent worker thread.

This patch modifies nested workers to pass the entire WorkerLoadInfo down in the sync runnable to the main thread.  This is used to set the channel here.  In later patches we will also set the principal.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832559 [details] [diff] [review]
P3 Move logic to set principal from channel into WorkerLoadInfo. r=baku

Currently the principal is set in a single location after the main script channel is completely loaded.  We will want to change that in later patches to happen earlier.  In addition, it will have to happen in two different places for top level workers vs nested workers.

Therefore, this patch refactors the code so setting the principal based on the channel is done in a separate method on WorkerLoadInfo.

Note, in order to avoid needing the parent WorkerPrivate we also move setting the initial principal/loadgroup in the nested worker case into its sync runnable code.  This allows us to simply assert that the principal exists in our new WorkerLoadInfo::SetPrincipalFromChannel().

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832560 [details] [diff] [review]
P4 Set WorkerLoadInfo principal as soon as we get the channel. r=baku

Set the principal as soon as the channel is created instead of waiting for it to complete loading.  As discussed in comment 10 and comment 12 this is safe because our security flags do not permit cross-origin redirects.  The next patch will add a diagnostic assertion verifying that this is the case.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832561 [details] [diff] [review]
P5 Validate that final channel load principal does not change. r=baku

Validate that the final channel principal after load matches the principal we set initially after the creation of the channel.  Again, this should be the case because our AsyncOpen2() security flags do not permit cross-origin redirects.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832603 [details] [diff] [review]
P6 Move ForgetMainThreadObjects() into WorkerLoadInfo. r=baku

Setting the principal earlier means we might fail in WorkerPrivate::GetLoadInfo() while on the worker parent thread with a main thread principal ref.  To fix this we will need to doom the WorkerLoadInfo main thread objects in these failure modes.

To prepare for that this patch refactors the code to perform most of the ForgetMainThreadObjects() logic in WorkerLoadInfo itself.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832604 [details] [diff] [review]
P7 Release WorkerLoadInfo data on main thread if we fail nested worker creation. r=baku

This dooms the WorkerLoadInfo main thread objects if we fail on the worker parent thread.  A bit of code duplication here, but not sure its worth breaking into another method for just two instances.  If we need to do this more we should make a utility method.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832608 [details] [diff] [review]
P8 Update test_chromeWorkerJSM.xul to expect synchronous SecurityError. r=baku

Finally, setting the principal before the network load has made some of our security checks occur earlier as well.  In particular, we can fail our chrome script checks and throw a SecurityError synchronously now.

Personally I think this is ok to do and not worth trying to maintain back compat with the old async behavior.

This patch fixes the test to expect the synchronous SecurityError.

Comment 34

[Andrea Marchesini [:baku]]

Comment on attachment 8832558 [details] [diff] [review]
P2 Make nested worker pass WorkerLoadInfo to main thread when getting channel. r=baku

Review of attachment 8832558 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ScriptLoader.cpp
@@ +1789,5 @@
>  
>  class ChannelGetterRunnable final : public WorkerMainThreadRunnable
>  {
>    const nsAString& mScriptURL;
> +  WorkerLoadInfo* mLoadInfo;

I prefer to have: const WorkerLoadInfo& mLoadInfo;

::: dom/workers/ScriptLoader.h
@@ +45,5 @@
>  nsresult
>  ChannelFromScriptURLWorkerThread(JSContext* aCx,
>                                   WorkerPrivate* aParent,
>                                   const nsAString& aScriptURL,
> +                                 WorkerLoadInfo* aLoadInfo);

Can you pass it as a reference?

Comment 35

[Andrea Marchesini [:baku]]

Comment on attachment 8832604 [details] [diff] [review]
P7 Release WorkerLoadInfo data on main thread if we fail nested worker creation. r=baku

Review of attachment 8832604 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/WorkerPrivate.cpp
@@ +4531,5 @@
> +      loadInfo.ForgetMainThreadObjects(doomed);
> +      nsCOMPtr<nsILoadGroup> loadGroupToCancel;
> +      RefPtr<MainThreadReleaseRunnable> runnable =
> +        new MainThreadReleaseRunnable(doomed, loadGroupToCancel);
> +      MOZ_ALWAYS_SUCCEEDS(aParent->DispatchToMainThread(runnable.forget()));

can you make an helper method/function for this?
Or better, an RAII class that does this.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #34)
> > +  WorkerLoadInfo* mLoadInfo;
> 
> I prefer to have: const WorkerLoadInfo& mLoadInfo;

I can make this a reference, but not const.  We need to modify the contents of the WorkerLoadInfo when we populate the channel and principal.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Refactor worker proxy release of main thread objects to reduce duplicate code. r=baku

The refactor to remove duplication in the proxy release patch ended up being a bit bigger than expected.  Asking for review instead of including in the P7 r+.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc320eb1bcf9410eab065d2940e82084d9d92c60

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Refactor worker proxy release of main thread objects to reduce duplicate code. r=baku

Try static analysis showed a problem.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e617f12f624eddb6e3aabf51bb61bf00f4c9a61

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8834122 [details] [diff] [review]
P9 Refactor worker proxy release of main thread objects to reduce duplicate code. r=baku

This patch reduces the duplication of code proxy releasing the main thread objects.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/845310066699
P1 Move WorkerPrivate::SetPrincipal() logic into a WorkerLoadInfo method. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/b42d5fed4729
P2 Make nested worker pass WorkerLoadInfo to main thread when getting channel. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/d32c5ac40db9
P3 Move logic to set principal from channel into WorkerLoadInfo. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/db60240c8647
P4 Set WorkerLoadInfo principal as soon as we get the channel. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/fad843873cf7
P5 Validate that final channel load principal does not change. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/d60942e5c631
P6 Move ForgetMainThreadObjects() into WorkerLoadInfo. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd9da392d93
P7 Release WorkerLoadInfo data on main thread if we fail nested worker creation. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f07bddd55a
P8 Update test_chromeWorkerJSM.xul to expect synchronous SecurityError. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/a71128968bee
P9 Refactor worker proxy release of main thread objects to reduce duplicate code. r=baku

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/845310066699
https://hg.mozilla.org/mozilla-central/rev/b42d5fed4729
https://hg.mozilla.org/mozilla-central/rev/d32c5ac40db9
https://hg.mozilla.org/mozilla-central/rev/db60240c8647
https://hg.mozilla.org/mozilla-central/rev/fad843873cf7
https://hg.mozilla.org/mozilla-central/rev/d60942e5c631
https://hg.mozilla.org/mozilla-central/rev/7dd9da392d93
https://hg.mozilla.org/mozilla-central/rev/22f07bddd55a
https://hg.mozilla.org/mozilla-central/rev/a71128968bee