705333 - Use IDL for nsJSCID::{CreateInstance,GetService}

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 576965 [details] [diff] [review]
Patch v1

I'm really sorry for totally failing on my promise of speedy reviews. :-( Thanks for doing this stuff!

>-    nsISupports createInstance();
>-    nsISupports getService();
>+    [implicit_jscontext,optional_argc] jsval createInstance([optional] in jsval iid);
>+    [implicit_jscontext,optional_argc] jsval getService([optional] in jsval iid);

It's good that you're using jsvals here, because I just wrote a patch that makes XPConnect throw on null IIDs: bug 705875. ;-)

>-    rv = xpc->WrapNativeToJSVal(cx, obj, inst, nsnull, iid, true, vp, nsnull);
>-    if (NS_FAILED(rv) || JSVAL_IS_PRIMITIVE(*vp))
>+    rv = nsXPConnect::GetXPConnect()->WrapNativeToJSVal(aCx, obj, inst, nsnull, iid, true, aRetval, nsnull);
>+    if (NS_FAILED(rv) || JSVAL_IS_PRIMITIVE(*aRetval))

Can nsXPConnect::GetXPConnect() ever return null? We check for it before, and with this patch we don't. We should either keep checking or be very sure that it's safe. Same thing in getInterface().

r=bholley once that's figured out.

Comment 2

[Bobby Holley (:bholley)]

So, I realized that as much as I like the aFoo naming convention, it isn't consistent with js-style, and XPConnect is supposed to be js-style. So r+ contingent on changing that as well. :-(

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

nsXPConnect::GetXPConnect() certainly can't be null at that point; we would already have returned when the helper failed. (I doubt it can be null otherwise; maybe on shutdown?)

https://hg.mozilla.org/mozilla-central/rev/f7c8894bbdae