450914 - Create a threadsafe URI object for the null principal to use

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Shawn Wilsher :sdwilsh]

With the test case in bug 429988, we trigger an assertion that nsSimpleURI is not threadsafe with the following backtrace:

nsSimpleURI::Internal::Release() (nsUnicharUtils.cpp:)
nsSimpleURI::Release() (nsUnicharUtils.cpp:)
nsCOMPtr<nsIURI>::~nsCOMPtr() (/usr/include/ctype.h:)
nsCOMPtr<nsIURI>::~nsCOMPtr() (/usr/include/ctype.h:)
nsNullPrincipal::~nsNullPrincipal() (/usr/include/ctype.h:)
nsNullPrincipal::Release() (/usr/include/ctype.h:)
nsCOMPtr<nsIPrincipal>::~nsCOMPtr() (/usr/include/c++/4.0.0/new:)
nsCOMPtr<nsIPrincipal>::~nsCOMPtr() (/usr/include/c++/4.0.0/new:)
PrincipalHolder::~PrincipalHolder() (/usr/include/c++/4.0.0/new:)
PrincipalHolder::Release() (/usr/include/c++/4.0.0/new:)
nsCOMPtr<nsIScriptObjectPrincipal>::~nsCOMPtr() (/usr/include/c++/4.0.0/new:)
nsCOMPtr<nsIScriptObjectPrincipal>::~nsCOMPtr() (/usr/include/c++/4.0.0/new:)
XPCWrappedNativeScope::~XPCWrappedNativeScope() (/usr/include/c++/4.0.0/new:)
XPCWrappedNativeScope::KillDyingScopes() (/usr/include/c++/4.0.0/new:)
XPCWrappedNativeScope::FinishedFinalizationPhaseOfGC(JSContext*) (/usr/include/c++/4.0.0/new:)
XPCJSRuntime::GCCallback(JSContext*, JSGCStatus) (/usr/include/c++/4.0.0/new:)
js_GC (/Code/moz/central/mozilla/obj-i386-apple-darwin9.4.0/js/src/jsautokw.h:)
JS_GC (/Code/moz/central/mozilla/obj-i386-apple-darwin9.4.0/js/src/jsautokw.h:)
main (/Code/moz/central/mozilla/xpcom/glue/nsComponentManagerUtils.cpp:)
_start (/Code/moz/central/mozilla/xpcom/glue/nsComponentManagerUtils.cpp:)
start (/Code/moz/central/mozilla/xpcom/glue/nsComponentManagerUtils.cpp:)

So, it seems that the principal object is being destroyed on a different thread that it was made (that test cases uses the thread manager in JS to create a thread and use it).

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Shawn has kindly agreed to write a patch to make XPConnect proxy its null principal objects to the main thread.

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Let's see how my reviewers like this...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 334399 [details] [diff] [review]
v1.0

This proxies the create and release but not the actual access...  that's bad.

What you want to do is probably to proxy the create, then create an XPCOM proxy for the object (if we're not on the main thread) and store that as the URI.  On the main thread, just create the object.

We don't want the 8-byte overhead that this is imposing on the main thread, by the way.  There, just create the URI like we used to do.

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> (From update of attachment 334399 [details] [diff] [review])
> This proxies the create and release but not the actual access...  that's bad.
> 
> What you want to do is probably to proxy the create, then create an XPCOM proxy
> for the object (if we're not on the main thread) and store that as the URI.  On
> the main thread, just create the object.
Err, what is this doing if it's not creating the proxy for the URI?
>+      nsCOMPtr<nsIURI> proxiedURI;
>+      rv = NS_GetProxyForObject(NS_PROXY_TO_MAIN_THREAD, NS_GET_IID(nsIURI),
>+                                uri, NS_PROXY_SYNC, getter_AddRefs(proxiedURI));
>+      NS_ENSURE_SUCCESS(rv, rv);

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Err, what is this doing if it's not creating the proxy for the URI?

Huh.  I missed that part somehow...

In any case, the point about not having the wrapper object stands.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

I have the code still going through nsNullPrincipalURI::Create, but it just returns the URI object before wrapping and proxying it.  This keeps the code for creating the URI in one spot instead of two.

Comment 7

[Boris Zbarsky [:bzbarsky]]

That's still creating this extra nsNullPrincipalURI object.... Why is that needed?

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> That's still creating this extra nsNullPrincipalURI object.... Why is that
> needed?
The patch only creates it if it's not on the main thread.

Unless you mean that the nsNullPrincipalURI object exists in general.  This is needed because we have to ensure that the nsSimpleURI is created, accessed, and destroyed on only the main thread.  Proxying the object will only ensure that the object is accessed on the target thread, but any calls to nsISupports can happen on any thread that is using it.  That means we still hit AddRef and Release issues.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow...  Say the principal holds the proxy object, right?  Then addref/release/QI calls will happen on the proxy object.  The addref/release will just addref/release the proxy itself (which is always holding only one ref to the real object).  If the proxy refcount goes to 0, it will handle doing the release of the real object on the thread the proxy was created on.

I guess we could still run into issues if someone QIs the proxy to an interface other than nsIURI and nsISupports and the URI implements that interface?

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Currently the POM only supports objects with threadsafe addref/release impls (see bug 434273), so creating a proxy of an nsSimpleURI is not sufficient. You'll need something else in the middle.

jst, mrbkap, and I were talking about another way of maybe solving this yesterday using xpconnect's thread destructor so maybe we should hold off on this for just a little longer until jst gets in today and we can chat a little more about this.

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

This creates a threadsafe non-mutable nsIURI object that the null principal can use.

Comment 12

[Shawn Wilsher :sdwilsh]

By the way - the conclusion of yesterday's talk was to make the threadsafe uri object.

However, my current patch triggers its own assertion in make check:
###!!! ASSERTION: Malformed URI!: 'dividerPosition != -1', file /Code/moz/central/mozilla/caps/src/nsNullPrincipalURI.cpp, line 51

Comment 13

[Shawn Wilsher :sdwilsh]

My clone method forgot to add the : between the scheme and path.  I've fixed this locally and am running the tests again.

Comment 14

[Shawn Wilsher :sdwilsh]

Attachment: v2.1

The one passes make check :)

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

> +  _path = nsDependentCString(mPath);

You want _path.Assign(mPath.get()) to actually copy.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 334766 [details] [diff] [review]
v2.1

>+++ b/caps/src/nsNullPrincipalURI.cpp
>+  // We want to give a full copy of the string and not share a string buffer

Why, exactly?

>+nsNullPrincipalURI::Equals(nsIURI *aOther, PRBool *_equals)

Ideally this would test class identity just like nsSimpleURI does.

>+++ b/caps/src/nsNullPrincipalURI.h
>+ * This wraps nsSimpleURI so that all calls to it are done on the main thread.

Fix the comment?

>+#include "nsAutoPtr.h"

No need to include that.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> >+  // We want to give a full copy of the string and not share a string buffer
> Why, exactly?
I was orginally thinking that we didn't do atomic refcounting on string buffers, but it turns out that we do.

> >+nsNullPrincipalURI::Equals(nsIURI *aOther, PRBool *_equals)
> 
> Ideally this would test class identity just like nsSimpleURI does.
How can I do that without adding a new interface?  This does the same test that nsSimpleURI does, but in a bit more verbose ordering.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> How can I do that without adding a new interface? 

The same way nsSimpleURI does: by cheating and QIing to a CID.

Comment 19

[Shawn Wilsher :sdwilsh]

Do you know if there are macros to pass a IID for an interface to, or do I have to write QueryInterface by hand?

Comment 20

[Shawn Wilsher :sdwilsh]

Attachment: v2.2

I just used the macros with a small bit of code on my own.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 335137 [details] [diff] [review]
v2.2

>+    *_equals = (0 == strcmp(mScheme.get(), otherURI->mScheme.get()) &&
>+                0 == strcmp(mPath.get(), otherURI->mPath.get()));

*_equals = (mScheme == otherURI->mScheme) &&
           (mPath == otherURI->mPath);

now that we have the concrete type, right?

sr=bzbarsky with that.  Thanks for fixing this!

Comment 22

[Shawn Wilsher :sdwilsh]

I didn't even know our string API could do that.  I'll address that after mrbkap's review.

Comment 23

[Shawn Wilsher :sdwilsh]

Attachment: v2.3

Addresses review comments; ready for checkin.

Comment 24

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/e0b00df22a47