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

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[timeless]

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.

Comment 1

[timeless]

Attachment: snapshot of the code i was using at the time

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

Comment 2

[timeless]

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.

Comment 3

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

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+.

Comment 4

[Ben Newman (:bnewman) (:benjamn)]

Attachment: something like this?

timeless, can you see if this addresses your concerns (and make any other comments you like)?

Comment 5

[timeless]

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

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

Attachment: adding {steal,stow}jsval methods to the nsixpcexception interface

per discussion with jonas, jst, and peterv

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 8

[Ben Newman (:bnewman) (:benjamn)]

(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

Comment 9

[Johnny Stenback (:jst)]

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.

Comment 10

[Johnny Stenback (:jst)]

Not blocking, but we should take this patch anyways as it makes our code safer and more correct, and the patch is done.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[Ben Newman (:bnewman) (:benjamn)]

(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.

Comment 13

[chris hofmann]

anyone have more thoughts?  what is the next step for this one?

Comment 14

[Ben Newman (:bnewman) (:benjamn)]

Attachment: catching up to tip

(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.

Comment 15

[Dão Gottwald [:dao]]

ping?

Comment 16

[Ben Newman (:bnewman) (:benjamn)]

(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

Comment 17

[Ben Newman (:bnewman) (:benjamn)]

landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/639af68a74e3

Comment 18

[Karl Tomlinson (:karlt)]

Is this still wanted for 1.9.1?

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 20

[Daniel Veditz [:dveditz]]

Is this hurting anything in practice on the 1.9.1 branch? Please comment if you think we still need to push this in.

Comment 21

[timeless]

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.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

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!

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

(also, this is 1.9.1 only, as we fixed it in 1.9.2)