519614 - Having to QI javascript scriptable helpers to nsIXPCScriptable is silly

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This code in XPCWrappedNative::GatherProtoScriptableCreateInfo:

935     nsCOMPtr<nsISupports> possibleHelper;
936     nsresult rv = classInfo->GetHelperForLanguage(
937                                     nsIProgrammingLanguage::JAVASCRIPT,
938                                     getter_AddRefs(possibleHelper));
939     if(NS_SUCCEEDED(rv) && possibleHelper)
940     {
941         nsCOMPtr<nsIXPCScriptable> helper(do_QueryInterface(possibleHelper));

has a QI that should never fail, I assume, and ends up doing various refcounty things that might be nice to avoid.

Not sure what we can replace it with, though.  This function is about 12% of slimwrapper creation time.

Comment 1

[Peter Van der Beken [:peterv]]

I had just been looking at this, I have a patch with a nsXPCClassInfo class that one can QI to and that inherits from nsIClassInfo and nsIXPCScriptable. Making nsDOMClassInfo inherit from nsXPCClassInfo allows us to remove 3 addref/release pairs on nsDOMClassInfo from slim wrapper construction. Not sure it'll be a big improvement in numbers.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I dunno about "big", but just shaving off 0.5-1% here and there will get us to big eventually (about 50 patches later).  ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

And note that it's not just the addref/release pairs but the QI calls themselves.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v2

This adds a new interface inheriting from both nsIClassInfo and nsIXPCScriptable, though I didn't use an nsI* prefix since it's not really a normal interface (let me know if you disagree). I've made nsDOMClassInfo inherit from it, and moved the code to preserve wrappers to it. I've also made slim wrappers require the new interface. For normal XPCWrappedNatives we'll still QI twice (once to nsIClassInfo and once to nsXPCClassInfo). If that's really a big issue we could use a flag on nsIClassInfo and cast instead, but hopefully most wrappers are slim wrappers anyway. I've made GatherScriptableCreateInfo return a reference to the XPCNativeScriptableCreateInfo that should be used for the wrapper. Right now, callers pass in two XPCNativeScriptableCreateInfo arguments (one for proto and one for wrapper) and we set the fields of the second to the same values as the first if the DontAskInstanceForScriptable flag is set. That actually addrefs the callback again, so instead of doing that I just pass the reference to the first argument back to the caller.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 434333 [details] [diff] [review]
v2

Is it documented anywhere that if you implement the wrapper cache, you have to implement the new interface too?

One comment:
> XPCWrappedNative::GatherProtoScriptableCreateInfo(
>                         nsIClassInfo* classInfo,
>-                        XPCNativeScriptableCreateInfo* sciProto)
>+                        XPCNativeScriptableCreateInfo& sciProto)
> {
>     NS_ASSERTION(classInfo, "bad param");
>-    NS_ASSERTION(sciProto && !sciProto->GetCallback(), "bad param");
>+    NS_ASSERTION(sciProto.GetCallback(), "bad param");
>+
>+    nsXPCClassInfo *classInfoHelper = nsnull;
>+    CallQueryInterface(classInfo, &classInfoHelper);
>+    if(classInfoHelper)
>+    {
>+        nsCOMPtr<nsIXPCScriptable> helper =
>+          dont_AddRef(static_cast<nsIXPCScriptable*>(classInfoHelper));

Why not:

nsRefPtr<XPCClassInfo> classInfoHelper = do_QueryInterface(classInfo);
if (classInfoHelper) {
    nsCOMPtr<nsIXPCScriptable> helper = classInfoHelper.forget();
    ...
}

or something like that (I'm not sure if that suggestion exactly compiles).

Comment 7

[[e-mail]]

(In reply to comment #6)
> Why not:
> nsRefPtr<XPCClassInfo> classInfoHelper = do_QueryInterface(classInfo);
> if (classInfoHelper) {
>     nsCOMPtr<nsIXPCScriptable> helper = classInfoHelper.forget();
>     ...
> }
> or something like that (I'm not sure if that suggestion exactly compiles).
Probably not without the helper code in bug 538964 (still awaiting review).
nxRefPtr<XPCClassInfo> classInfoHelper = do_QueryObject(classInfo);

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to comment #6)
> nsRefPtr<XPCClassInfo> classInfoHelper = do_QueryInterface(classInfo);
> if (classInfoHelper) {
>     nsCOMPtr<nsIXPCScriptable> helper = classInfoHelper.forget();
>     ...
> }
> 
> or something like that (I'm not sure if that suggestion exactly compiles).

Right, that doesn't compile. I tried all kind of combinations of swap and forget, but the raw pointer and CallQI is the least ugly unfortunately.

http://hg.mozilla.org/mozilla-central/rev/c19b69a502ea