Closed
Bug 450914
Opened 15 years ago
Closed 15 years ago
Create a threadsafe URI object for the null principal to use
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b1
People
(Reporter: sdwilsh, Assigned: sdwilsh)
References
Details
Attachments
(1 file, 5 obsolete files)
11.70 KB,
patch
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
Shawn has kindly agreed to write a patch to make XPConnect proxy its null principal objects to the main thread.
Assignee: nobody → sdwilsh
Assignee | ||
Comment 2•15 years ago
|
||
Let's see how my reviewers like this...
Attachment #334399 -
Flags: superreview?
Attachment #334399 -
Flags: review?(mrbkap)
Assignee | ||
Updated•15 years ago
|
Attachment #334399 -
Flags: superreview? → superreview?(bzbarsky)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [has patch][needs review mrbkap][needs sr bz]
![]() |
||
Comment 3•15 years ago
|
||
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-
Assignee | ||
Comment 4•15 years ago
|
||
(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•15 years ago
|
||
> 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.
Assignee | ||
Comment 6•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #334496 -
Flags: superreview? → superreview?(bzbarsky)
![]() |
||
Comment 7•15 years ago
|
||
That's still creating this extra nsNullPrincipalURI object.... Why is that needed?
Assignee | ||
Comment 8•15 years ago
|
||
(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•15 years ago
|
||
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.
Assignee | ||
Comment 11•15 years ago
|
||
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)
Assignee | ||
Comment 12•15 years ago
|
||
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
Assignee | ||
Comment 13•15 years ago
|
||
My clone method forgot to add the : between the scheme and path. I've fixed this locally and am running the tests again.
Assignee | ||
Comment 14•15 years ago
|
||
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 16•15 years ago
|
||
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.
Assignee | ||
Comment 17•15 years ago
|
||
(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•15 years ago
|
||
> How can I do that without adding a new interface?
The same way nsSimpleURI does: by cheating and QIing to a CID.
Assignee | ||
Comment 19•15 years ago
|
||
Do you know if there are macros to pass a IID for an interface to, or do I have to write QueryInterface by hand?
Assignee | ||
Comment 20•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #335137 -
Flags: review? → review?(mrbkap)
![]() |
||
Comment 21•15 years ago
|
||
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+
Assignee | ||
Comment 22•15 years ago
|
||
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]
Updated•15 years ago
|
Attachment #335137 -
Flags: review?(mrbkap) → review+
Assignee | ||
Updated•15 years ago
|
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
Assignee | ||
Comment 23•15 years ago
|
||
Addresses review comments; ready for checkin.
Attachment #335137 -
Attachment is obsolete: true
Assignee | ||
Comment 24•15 years ago
|
||
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/e0b00df22a47
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Whiteboard: [has patch][has review][has sr]
Updated•15 years ago
|
QA Contact: content
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•