Closed Bug 450914 Opened 12 years ago Closed 12 years ago

Create a threadsafe URI object for the null principal to use

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 5 obsolete files)

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).
Shawn has kindly agreed to write a patch to make XPConnect proxy its null principal objects to the main thread.
Assignee: nobody → sdwilsh
Attached patch v1.0 (obsolete) — Splinter Review
Let's see how my reviewers like this...
Attachment #334399 - Flags: superreview?
Attachment #334399 - Flags: review?(mrbkap)
Attachment #334399 - Flags: superreview? → superreview?(bzbarsky)
Whiteboard: [has patch][needs review mrbkap][needs sr bz]
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.
Attachment #334399 - Flags: superreview?(bzbarsky) → superreview-
(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);
> 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.
Attached patch v1.1 (obsolete) — Splinter Review
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.
Attachment #334399 - Attachment is obsolete: true
Attachment #334496 - Flags: superreview?
Attachment #334496 - Flags: review?(mrbkap)
Attachment #334399 - Flags: review?(mrbkap)
Attachment #334496 - Flags: superreview? → superreview?(bzbarsky)
That's still creating this extra nsNullPrincipalURI object.... Why is that needed?
(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.
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?
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.
Attached patch v2.0 (obsolete) — Splinter Review
This creates a threadsafe non-mutable nsIURI object that the null principal can use.
Attachment #334496 - Attachment is obsolete: true
Attachment #334753 - Flags: superreview?(bzbarsky)
Attachment #334753 - Flags: review?(mrbkap)
Attachment #334496 - Flags: superreview?(bzbarsky)
Attachment #334496 - Flags: review?(mrbkap)
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
My clone method forgot to add the : between the scheme and path.  I've fixed this locally and am running the tests again.
Attached patch v2.1 (obsolete) — Splinter Review
The one passes make check :)
Attachment #334753 - Attachment is obsolete: true
Attachment #334766 - Flags: superreview?(bzbarsky)
Attachment #334766 - Flags: review?(mrbkap)
Attachment #334753 - Flags: superreview?(bzbarsky)
Attachment #334753 - Flags: review?(mrbkap)
> +  _path = nsDependentCString(mPath);

You want _path.Assign(mPath.get()) to actually copy.
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.
(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.
> How can I do that without adding a new interface? 

The same way nsSimpleURI does: by cheating and QIing to a CID.
Do you know if there are macros to pass a IID for an interface to, or do I have to write QueryInterface by hand?
Attached patch v2.2 (obsolete) — Splinter Review
I just used the macros with a small bit of code on my own.
Attachment #334766 - Attachment is obsolete: true
Attachment #335137 - Flags: superreview?(bzbarsky)
Attachment #335137 - Flags: review?
Attachment #334766 - Flags: superreview?(bzbarsky)
Attachment #334766 - Flags: review?(mrbkap)
Attachment #335137 - Flags: review? → review?(mrbkap)
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!
Attachment #335137 - Flags: superreview?(bzbarsky) → superreview+
I didn't even know our string API could do that.  I'll address that after mrbkap's review.
Status: NEW → ASSIGNED
Whiteboard: [has patch][needs review mrbkap][needs sr bz] → [has patch][needs review mrbkap][has sr]
Attachment #335137 - Flags: review?(mrbkap) → review+
Flags: in-testsuite-
Flags: in-litmus-
Summary: "ASSERTION: nsSimpleURI not thread-safe" during principal destruction → Create a threadsafe URI object for the null principal to use
Whiteboard: [has patch][needs review mrbkap][has sr] → [has patch][has review][has sr]
Target Milestone: --- → mozilla1.9.1b1
Attached patch v2.3Splinter Review
Addresses review comments; ready for checkin.
Attachment #335137 - Attachment is obsolete: true
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/e0b00df22a47
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has sr]
QA Contact: content
Component: Content → DOM
QA Contact: content → general
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.