Closed Bug 519614 Opened 10 years ago Closed 10 years ago

Having to QI javascript scriptable helpers to nsIXPCScriptable is silly

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: peterv)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
I dunno about "big", but just shaving off 0.5-1% here and there will get us to big eventually (about 50 patches later).  ;)
And note that it's not just the addref/release pairs but the QI calls themselves.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attached patch v2Splinter Review
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.
Attachment #417549 - Attachment is obsolete: true
Attachment #434333 - Flags: review?(mrbkap)
Blocks: 533637
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).
Attachment #434333 - Flags: review?(mrbkap) → review+
(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);
(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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.