Closed Bug 1333573 Opened 3 years ago Closed 3 years ago

WorkerPrivate should initialize mPrincipal and mPrincipalInfo sooner

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Iteration:
54.2 - Feb 20
Tracking Status
firefox54 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 8 obsolete files)

4.35 KB, patch
baku
: review+
Details | Diff | Splinter Review
5.14 KB, patch
baku
: review+
Details | Diff | Splinter Review
13.18 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.94 KB, patch
baku
: review+
Details | Diff | Splinter Review
6.68 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.43 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.15 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.83 KB, patch
baku
: review+
Details | Diff | Splinter Review
9.37 KB, patch
baku
: review+
Details | Diff | Splinter Review
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.
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 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.
Attachment #8830470 - Attachment is obsolete: true
Attachment #8830531 - Attachment is obsolete: true
Attachment #8830861 - Attachment is obsolete: true
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.
Attachment #8831316 - Flags: review?(amarchesini)
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.
Attachment #8831317 - Flags: review?(ckerschb)
Attachment #8831317 - Flags: review?(amarchesini)
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.
Attachment #8831318 - Flags: review?(ckerschb)
Attachment #8831318 - Flags: review?(amarchesini)
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.
Attachment #8831318 - Flags: review?(ckerschb) → review+
Attachment #8831317 - Flags: review?(ckerschb) → review+
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 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.
Attachment #8831316 - Flags: review?(amarchesini)
Attachment #8831317 - Flags: review?(amarchesini)
Attachment #8831318 - Flags: review?(amarchesini)
I think I have another set of patches that will work.  Will post for review later tonight or tomorrow.
Attachment #8831316 - Attachment is obsolete: true
Attachment #8831317 - Attachment is obsolete: true
Attachment #8831318 - Attachment is obsolete: true
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.
These two patches fix the first problem with release objects off the main thread.
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().
Attachment #8832556 - Flags: review?(amarchesini)
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.
Attachment #8832558 - Flags: review?(amarchesini)
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().
Attachment #8832559 - Flags: review?(amarchesini)
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.
Attachment #8832560 - Flags: review?(amarchesini)
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.
Attachment #8832561 - Flags: review?(amarchesini)
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.
Attachment #8832603 - Flags: review?(amarchesini)
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.
Attachment #8832604 - Flags: review?(amarchesini)
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.
Attachment #8832608 - Flags: review?(amarchesini)
Attachment #8832556 - Flags: review?(amarchesini) → review+
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?
Attachment #8832558 - Flags: review?(amarchesini) → review+
Attachment #8832559 - Flags: review?(amarchesini) → review+
Attachment #8832560 - Flags: review?(amarchesini) → review+
Attachment #8832561 - Flags: review?(amarchesini) → review+
Attachment #8832603 - Flags: review?(amarchesini) → review+
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.
Attachment #8832604 - Flags: review?(amarchesini) → review+
Attachment #8832608 - Flags: review?(amarchesini) → review+
(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.
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+.
Attachment #8834113 - Flags: review?(amarchesini)
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.
Attachment #8834122 - Flags: review?(amarchesini)
Attachment #8834122 - Flags: review?(amarchesini) → review+
Pushed by bkelly@mozilla.com:
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
Blocks: 1112134
Depends on: 1337522
Blocks: 1337522
No longer depends on: 1337522
Depends on: 1338523
Depends on: 1338299
No longer blocks: 1337522
Iteration: --- → 54.2 - Feb 20
You need to log in before you can comment on or make changes to this bug.