Closed Bug 467865 Opened 11 years ago Closed 11 years ago

[@ js_RemoveRoot - ... - XPCThrower::ThrowExceptionObject] xpconnect is misusing nsCOMPtr

Categories

(Core :: XPConnect, defect, critical)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- unaffected
status1.9.1 --- wanted

People

(Reporter: timeless, Assigned: mozilla+ben)

References

()

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files, 2 obsolete files)

js3250!js_RemoveRoot(struct JSRuntime * rt = 0x00000001, void * rp = 0x020ee07c)+0x7
js3250!JS_RemoveRootRT(struct JSRuntime * rt = 0x00000001, void * rp = 0x020ee07c)+0x10
xpc3250!nsAutoJSValHolder::Release(void)+0x61
xpc3250!nsXPCException::StealThrownJSVal(long * vp = 0x01ede664)+0x21
xpc3250!XPCThrower::ThrowExceptionObject(struct JSContext * cx = 0x00df2400, class nsIException * e = 0x020edad0)+0x58

I suspect someone could write a simple testcase if they wanted to.

i have code which generates an nsXPCException and then creates a js image of that and then throws that.

This should be legal, but xpconnect is amusing nsCOMPtr here:
274 XPCThrower::ThrowExceptionObject(JSContext* cx, nsIException* e)
275 {
276     JSBool success = JS_FALSE;
277     if(e)
278     {
279         nsCOMPtr<nsXPCException> xpcEx; 

I think a testcase would generate an nsXPCException, capture it, mirror it to a js object, throw the exception on a thread and play games with gc.
I think the following is fairly similar to what i was doing:
load("c:\\proxifier.js"); work(); while (task.state<2) { addWork(10); for (x = 0; (task.state<2) && (x < 20); ++x) work(); }

a bit more depth for my stack trace:
js_RemoveRoot
JS_RemoveRootRT
nsAutoJSValHolder::Release
nsXPCException::StealThrownJSVal
XPCThrower::ThrowExceptionObject(struct JSContext * cx = 0x00df2400, class nsIException * e = 0x020edad0)+0x58
XPCThrower::CheckForPendingException(unsigned int result = 0x80004005, struct JSContext * cx = 0x00df2400)+0xd6
XPCThrower::ThrowBadResult(unsigned int rv = 0x80570004, unsigned int result = 0x80004005, class XPCCallContext * ccx = 0x01edea08)+0x18
ThrowBadResult
XPCWrappedNative::CallMethod
XPC_WN_CallMethod
js_Invoke
ben, ben: please note that you broke a holy law of xpcom:

Thou shalt not assume structure layout for the result of a QI.

If you need a private interface to your private object then you need to create such a private interface and QI to it.
Assignee: nobody → bnewman
Blocks: 462389, 446584
Severity: normal → critical
Keywords: crash
Flags: blocking1.9.1?
Keywords: regression
Assigning bugs to people could discourage others from jumping in if they think someone else is already working on it. We'll find someone to work on this if it is marked blocking+.
Assignee: bnewman → nobody
Attached patch something like this? (obsolete) — Splinter Review
timeless, can you see if this addresses your concerns (and make any other comments you like)?
Comment on attachment 351648 [details] [diff] [review]
something like this?

not really. interfaces should be interfaces, i.e. abstract, with no implementation. you shouldn't be subclassing something to steal a couple of method impls.

just create a proper interface (so that a python or java impl could cooperate) 

class nsXPCException
{
...
NS_DECL_ISUPPORTS
NS_DECL_NSIEXCEPTION
NS_DECL_NSIXPCEXCEPTION
+NS....

you should add a decl line there which replaces the raw method decls.

-    nsAutoJSValHolder mThrownJSVal;

you shouldn't move this stuff out of the class

-        nsCOMPtr<nsXPCException> xpcEx;
+        nsCOMPtr<nsPIXPCException> xpcEx;

this is fine, although the name is not great. try to describe what it actually does. I don't see anything here that precludes java/python from conspiring with this class.

define it with idl. you don't have to mark the class scriptable, although you could, because the methods themselves won't be scriptable, as long as you rv check method calls (the ones that can't be implemented by scripts - i.e. all of them will return a failure rv if you call a method that couldn't be implemented by script
Attachment #351648 - Attachment is obsolete: true
per discussion with jonas, jst, and peterv
Attachment #352243 - Flags: superreview?(jst)
Attachment #352243 - Flags: review?(jonas)
Comment on attachment 352243 [details] [diff] [review]
adding {steal,stow}jsval methods to the nsixpcexception interface

>+%{ C++
>+#include "jspubtd.h"
>+%}

Why is this needed? It'd be nice not to have to and let users #include it if they need it.

r=me with that.
Attachment #352243 - Flags: review?(jonas) → review+
(In reply to comment #7)
> (From update of attachment 352243 [details] [diff] [review])
> >+%{ C++
> >+#include "jspubtd.h"
> >+%}
> 
> Why is this needed? It'd be nice not to have to and let users #include it if
> they need it.

it's for JSContext and jsval, but I agree it's better to make users #include it
Attachment #352243 - Flags: superreview?(jst) → superreview+
Comment on attachment 352243 [details] [diff] [review]
adding {steal,stow}jsval methods to the nsixpcexception interface

sr=jst, but yeah, make the users of this #include that file.
Not blocking, but we should take this patch anyways as it makes our code safer and more correct, and the patch is done.
Flags: blocking1.9.1? → blocking1.9.1-
Assignee: nobody → bnewman
Attachment #352243 - Flags: approval1.9.1+
So...  If that #include is removed then any code anywhere that uses nsIXPConnect will need to #include jspubtd before #including nsIXPConnect.  Is that really what we want?
(In reply to comment #11)
> So...  If that #include is removed then any code anywhere that uses
> nsIXPConnect will need to #include jspubtd before #including nsIXPConnect.  Is
> that really what we want?

Judging from my recent efforts to remove the #include and fix clients of the interface, I for one would vote to leave it in.
anyone have more thoughts?  what is the next step for this one?
(In reply to comment #13)
> anyone have more thoughts?  what is the next step for this one?

The only issue blocking the patch was the #include question.  To me it seems that it's not really worthwhile to remove it.  This patch doesn't change behavior otherwise, so I think it's safe to land.  Tossing it over to the tryserver right now.
Attachment #352243 - Attachment is obsolete: true
Attachment #370076 - Flags: review+
ping?
(In reply to comment #15)
> ping?

It's still ready to go.  Compiles on all platforms, just checked.  I'll land as soon as the tree turns green, barring any final objections.

Also, other IDL files that have jsvals in their method signatures #include "jspubtd.h", so at least there's precedent:

http://mxr.mozilla.org/mozilla-central/search?string=native.*jsval&regexp=on&case=on&find=.idl%24&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central
landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/639af68a74e3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Is this still wanted for 1.9.1?
Comment on attachment 352243 [details] [diff] [review]
adding {steal,stow}jsval methods to the nsixpcexception interface

Reducing churn as we close down for 3.5, so removing approval.
Attachment #352243 - Flags: approval1.9.1+ → approval1.9.1-
Flags: wanted1.9.1.x+
Is this hurting anything in practice on the 1.9.1 branch? Please comment if you think we still need to push this in.
status1.9.1: --- → ?
Comment on attachment 370076 [details] [diff] [review]
catching up to tip

yes, we should take this, it's safe and correct, and leaving it broken is bad.
Attachment #370076 - Flags: approval1.9.1.10?
Comment on attachment 370076 [details] [diff] [review]
catching up to tip

Changing the UUID in the .idl file tells me that this patch is not branch-safe.

We'd approve a branch-safe one, though!
Attachment #370076 - Flags: approval1.9.1.10? → approval1.9.1.10-
(also, this is 1.9.1 only, as we fixed it in 1.9.2)
Crash Signature: [@ js_RemoveRoot - ... - XPCThrower::ThrowExceptionObject]
You need to log in before you can comment on or make changes to this bug.