1448328 - Most URLWorker methods don't need to be proxied to the main thread anymore

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

Since URLs are threadsafe/immutable now, URLWorker just needs to keep a nsCOMPtr<nsIURI> and it can use it directly. All the getters and most of the setters do not need to be proxied to the main thread anymore.
Exceptions are SetHref and SetScheme, but only when the scheme is different from the current scheme (if the URLs have the same type, then we can just use the mutator to get a new changed URI on the same thread).

Comment 1

[Marion Daly [:mdaly]]

:asuth, p3 backlog for now unless you feel otherwise.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

I think :catalinb already optimized URLWorker for the http/https case in bug 1344751 to avoid proxying by holding an nsStandardURL in mStdURL, so I think P3 probably does make sense for now.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - No GetterRunnable/SetterRunnable

Having nsIURI thread safe is a huge simplification in URL API code. Thanks!
Wondering if we can get rid of the use of nsStandardURL. Currently, with this patch, mStdURL is useful only when setting a new protocol or the full href.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - no nsStandardURL

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3 - no TeardownURLRunnable

Completely unrelated, but it's a cleanup patch.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 3 - no TeardownURLRunnable

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8962004 [details] [diff] [review]
part 1 - No GetterRunnable/SetterRunnable

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

::: dom/url/URL.h
@@ +174,5 @@
>    void CreateSearchParamsIfNeeded();
>  
>    nsCOMPtr<nsISupports> mParent;
>    RefPtr<URLSearchParams> mSearchParams;
> +  RefPtr<net::MozURL> mURL;

I don't think we're ready to switch to MozURL just yet.
Some frotend modules use URL objects to refer to addons, resource URIs, etc... which MozURL can't handle yet.

It's OK to have an nsCOMPtr<nsIURI> instead.

::: dom/webidl/URL.webidl
@@ +34,3 @@
>             attribute USVString search;
> +  [SameObject]
> +  readonly attribute URLSearchParams searchParams;

I think someone else should review these changes. Also, some of these setters would still have to throw, I think.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8962012 [details] [diff] [review]
part 3 - no TeardownURLRunnable

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

::: dom/url/URLWorker.cpp
@@ +454,5 @@
>  URLWorker::~URLWorker()
>  {
>    if (mURLProxy) {
>      mWorkerPrivate->AssertIsOnWorkerThread();
> +    NS_ReleaseOnMainThreadSystemGroup("URLWorker::URLProxy",

nsIURI can now be released on any thread. So even if the Worker/Proxy hold the last ref, we don't need to release it on the main thread anymore.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 1 - No GetterRunnable/SetterRunnable

This was the wrong patch.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 3 - no TeardownURLRunnable

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8962005 [details] [diff] [review]
part 2 - no nsStandardURL

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

::: dom/url/URLWorker.cpp
@@ -486,5 @@
> -      return;
> -    }
> -  }
> -
> -  if (scheme.EqualsLiteral("http") || scheme.EqualsLiteral("https")) {

So we had this workaround because you couldn't NS_NewURI on a worker thread. That is still true, so we might want to keep this around.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

> So we had this workaround because you couldn't NS_NewURI on a worker thread.
> That is still true, so we might want to keep this around.

This was not clear to me.  I thought "threadafe nsIURI" included the initial parsing of the URL.  What is blocking that?

Comment 13

[Andrea Marchesini [:baku]]

> So we had this workaround because you couldn't NS_NewURI on a worker thread.
> That is still true, so we might want to keep this around.

Correct. But with this patch we are going to create a nsIURI on main-thread each time a new URI is created. Plus, when setHref and setProtocol is called. For the rest we stay on the current thread. This seems acceptable to me.

I also have a patch that merges URLWorker and URLMainThread. We just need specific Contructor, SetProtocol and SetHref method, but for the rest, the code base can be the same if we get rid of nsStdURL.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

(In reply to Andrea Marchesini [:baku] from comment #13)
> > So we had this workaround because you couldn't NS_NewURI on a worker thread.
> > That is still true, so we might want to keep this around.
> 
> Correct. But with this patch we are going to create a nsIURI on main-thread
> each time a new URI is created. Plus, when setHref and setProtocol is
> called. For the rest we stay on the current thread. This seems acceptable to
> me.

That is technically correct, but it may not be optimal going to the main thread all the time. For http(s) URLs we know the implementation is an nsStandardURL so we can just go ahead and create it OMT.

> I also have a patch that merges URLWorker and URLMainThread. We just need
> specific Contructor, SetProtocol and SetHref method, but for the rest, the
> code base can be the same if we get rid of nsStdURL.

If you want to implement the optimization in the followup patch, that is fine with me.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

FWIW, I think we still need the optimization to avoid main thread completely for http/https URLs.  That was specifically added in to avoid main thread jank in service workers during navigation where the fetch event handler uses URL.

Comment 16

[Andrea Marchesini [:baku]]

I still see issues with nsIURI when used with nsIURIWithPrincipal (aka blob URL).
nsIURIWithPrincipal is not thread-safe, and that makes all these patches fragile.

At some point I wrote several patches to get rid of nsIURIWithPrincipal for blob URLs, but I haven't found the time to finish them.

Comment 17

[Valentin Gosu [:valentin] (he/him)]

nsIURIWithPrincipal only has readonly attributes. But indeed, nsIPrincipal is not yet thread-safe. If we could get rid of it that would be great.

Comment 18

[Marion Daly [:mdaly]]

Actively being worked on= priority is automatically P2. 

In the future, please give reason for working on backlog items as to why it needs to be higher in priority.

Comment 19

[Andrea Marchesini [:baku]]

Valentin, it seems we need to get rid of nsIURIWithPrincipal before considering nsIURI thread-safe.
When we call NS_MutableURI(), we clone nsIURI, but nsIURIWithPrincipal needs to create a new ref of nsIPrincipal. This is not allowed out of main-thread.

Assertion failure: NS_IsMainThread(), at /builds/worker/workspace/build/src/caps/nsJSPrincipals.cpp:28
#01: nsCOMPtr<nsIPrincipal>::assign_with_AddRef [xpcom/base/nsCOMPtr.h:1165]
#02: nsHostObjectURI::CloneInternal [dom/file/nsHostObjectURI.cpp:189] 
#03: nsHostObjectURI::Mutate [s3:gecko-generated-sources-l1:ff34d643745a62a2f21f8ae027d6f650f113afa4b05d26076011b42fb1af3a79daead6ff36e74ea1d0f73635ad9f8900ed59aafdf745752898fda6d85b53c40d/dist/include/nsIURIMutator.h::49]
#04: NS_MutateURI::NS_MutateURI [netwerk/base/nsIURIMutatorUtils.cpp:11]

Comment 20

[Valentin Gosu [:valentin] (he/him)]

(In reply to Andrea Marchesini [:baku] from comment #19)
> Valentin, it seems we need to get rid of nsIURIWithPrincipal before
> considering nsIURI thread-safe.
> When we call NS_MutableURI(), we clone nsIURI, but nsIURIWithPrincipal needs
> to create a new ref of nsIPrincipal. This is not allowed out of main-thread.
> 
> Assertion failure: NS_IsMainThread(), at
> /builds/worker/workspace/build/src/caps/nsJSPrincipals.cpp:28
> #01: nsCOMPtr<nsIPrincipal>::assign_with_AddRef [xpcom/base/nsCOMPtr.h:1165]
> #02: nsHostObjectURI::CloneInternal [dom/file/nsHostObjectURI.cpp:189] 
> #03: nsHostObjectURI::Mutate
> [s3:gecko-generated-sources-l1:
> ff34d643745a62a2f21f8ae027d6f650f113afa4b05d26076011b42fb1af3a79daead6ff36e74
> ea1d0f73635ad9f8900ed59aafdf745752898fda6d85b53c40d/dist/include/
> nsIURIMutator.h::49]
> #04: NS_MutateURI::NS_MutateURI [netwerk/base/nsIURIMutatorUtils.cpp:11]

You're right. We're definitely not thread-safe yet.
I'm guessing we should file a separate bug to remove/fix the nsIURIWithPrincipal issue. Or fix bug 1443925, but I'm guessing that could take longer than expected.

Comment 21

[Valentin Gosu [:valentin] (he/him)]

So, do we have a plan to move this forward? Or are we waiting for bug 1443925?
Is there something I can do to help here?

Comment 22

[Andrea Marchesini [:baku]]

My approach would be to get rid of nsIURIWithPrincipal entirely, but that is not a small task. I would like to work on it, but I'm too busy at the moment. I'll love to jump on this task as soon as possible, but if bug 1443925 lands before, we can directly land these 2 patches without any extra work.

The way I would like to remove nsIURIWithPrincipal is:

1. Each process should keep a list of blobURL+blobImpl for sandboxes. These blobURLs are not supposed to be broadcasted to the parent process or to other processes. Any other blobURL is registered onto the parent process. Here we should use ContentPrincipal with the blobURL as codebase.

2. The origin should be taken from the blobURL path instead of using nsIURIWithPrincipal.

4. NullPrincipal/ContentPrincipal::MayLoadInternal should always allow the loading of blobURLs. When a nsIChannel is created from a blobURL, we should check if it can be loaded by the loading principal. This check must be async and done using the per-process list of blobURLs and the list of blobURLs in the parent process.

5. We need to audit all the uses of NullPrincipal/ContentProcess::MayLoadInternal to see if compatible with the previous point.

Doing this, we can avoid the broadcasting of BlobURLs to each process each time a new one is created.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 2 - nsContentUtils::GetUTFOrigin doesn't need to use nsIURIWithPrincipal for blobs

There are 2 approaches here:

1. I can use a sync runnable from worker to the main-thread to call nsContentUtils::GetUTFOrigin.

2. We can get rid of nsIURIWithPrincipal in nsContentUtils::GetUTFOrigin. I decided to use this approach because: we already support unknown blobs and the path of a blob must be the owning URL by spec.

Plus, nsIURIWithPrincipal will be removed completely eventually.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8967675 [details] [diff] [review]
part 2 - nsContentUtils::GetUTFOrigin doesn't need to use nsIURIWithPrincipal for blobs

I don't understand this.
Why we can remove the code which was added for https://bugzilla.mozilla.org/show_bug.cgi?id=1101584

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

I see, comment 23.
Need to review all principal handling and whether this all is safe.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8967675 [details] [diff] [review]
part 2 - nsContentUtils::GetUTFOrigin doesn't need to use nsIURIWithPrincipal for blobs

ok, document.domain stuff should affect to this stuff, since principal's url doesn't change.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93ef66259e21
Use thread-safe nsIURI in the URI API - part 1 - No getter/setter runnables, r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdea2aeaec36
Use thread-safe nsIURI in the URI API - part 2 - No teardown runnable, r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/b05199040819
Implement getOrigin for blobURL without nsIURIWithPrincipal, r=smaug

Comment 28

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/93ef66259e21
https://hg.mozilla.org/mozilla-central/rev/fdea2aeaec36
https://hg.mozilla.org/mozilla-central/rev/b05199040819