Closed Bug 1448328 Opened 6 years ago Closed 6 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: valentin, Assigned: baku)

References

Details

Attachments

(3 files, 4 obsolete files)

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).
Priority: -- → P2
Priority: P2 → P3
:asuth, p3 backlog for now unless you feel otherwise.
Flags: needinfo?(bugmail)
Component: DOM: Service Workers → DOM: Workers
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.
Flags: needinfo?(bugmail)
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.
Assignee: nobody → amarchesini
Attachment #8962004 - Flags: review?(valentin.gosu)
Attachment #8962004 - Attachment description: url.patch → part 1 - No GetterRunnable/SetterRunnable
Attached patch part 2 - no nsStandardURL (obsolete) — Splinter Review
Attachment #8962005 - Flags: review?(valentin.gosu)
Attached patch part 3 - no TeardownURLRunnable (obsolete) — Splinter Review
Completely unrelated, but it's a cleanup patch.
Attachment #8962006 - Flags: review?(valentin.gosu)
Attached patch part 3 - no TeardownURLRunnable (obsolete) — Splinter Review
Attachment #8962006 - Attachment is obsolete: true
Attachment #8962006 - Flags: review?(valentin.gosu)
Attachment #8962012 - Flags: review?(valentin.gosu)
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.
Attachment #8962004 - Flags: review?(valentin.gosu)
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.
Attachment #8962012 - Flags: review?(valentin.gosu)
This was the wrong patch.
Attachment #8962004 - Attachment is obsolete: true
Attachment #8962175 - Flags: review?(valentin.gosu)
Attachment #8962012 - Attachment is obsolete: true
Attachment #8962176 - Flags: review?(valentin.gosu)
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.
Attachment #8962005 - Flags: review?(valentin.gosu)
> 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?
> 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.
Flags: needinfo?(valentin.gosu)
Attachment #8962176 - Flags: review?(valentin.gosu) → review+
(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.
Flags: needinfo?(valentin.gosu)
Attachment #8962005 - Attachment is obsolete: true
Attachment #8962175 - Flags: review?(valentin.gosu) → review+
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.
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.
Flags: needinfo?(valentin.gosu)
nsIURIWithPrincipal only has readonly attributes. But indeed, nsIPrincipal is not yet thread-safe. If we could get rid of it that would be great.
Flags: needinfo?(valentin.gosu)
Priority: P3 → P2
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.
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]
(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.
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?
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
Depends on: 1453633
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.
Attachment #8967675 - Flags: review?(bugs)
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
I see, comment 23.
Need to review all principal handling and whether this all is safe.
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.
Attachment #8967675 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.