349002 - Refactor xptcall into a frozen API

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Benjamin Smedberg]

Currently xptcall is exported as a bunch of nonfrozen C++ symbols. This bug will track the work needed to port xptcall over to use frozen C exports. I originally began this work on BSMEDBERG_20060330_XPTCALL_BRANCH.

Comment 1

[Benjamin Smedberg]

Attachment: NS_GetXPTCallStub love, rev. 1

Darin, I did a mostly-global s/XPTC_InvokeByIndex/NS_InvokeByIndex/, which caused a little bit of whitespace to be unaligned. I can fix that before checkin if you prefer.

I have fixed up the platform-specific xptcall bits for linux x86 and x86-64, windows x86, mac-ppc, and mac-x86. After reviews, I will issue a call for additional xptcall implementations before checkin.

Comment 2

[Darin Fisher]

bsmedberg, it looks like you did more than just rename XPTC_InvokeByIndex.  Can you please list / summarize the complete set of changes?

Comment 3

[Benjamin Smedberg]

Yeah, the renaming was the least important part.

Summary:

The XPTCall API should be available entirely through frozen structures and a C API. To this end, I have replaced the nsXPTCStubBase class (which is linked via C++) with a frozen accessor NS_GetXPTCallStub() (xptcall.h has the docs).

The remaining changes should be simply changing client code to use the new API. This replaces classes that inherited from nsXPTCStubBase with a mXPTCStub member and NS_Get/DestroyXPTCallStub pair in the constructor/initializer and destructor.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 235073 [details] [diff] [review]
NS_GetXPTCallStub love, rev. 1

content/xtf/src/nsXTFInterfaceAggregator.cpp
+  if (!result || result->mXPTCStub)
     return NS_ERROR_OUT_OF_MEMORY;

that's missing a ! in the second clause of the if, right?


have you considered writing a helper class that avoids having to call NS_DestroyXPTCallStub in each destructor? (and in the constructors?)

content/xtf/src/nsXTFWeakTearoff.cpp
+  nsRefPtr<nsXTFWeakTearoff> result
+    = new nsXTFWeakTearoff(iid, obj);

isn't it more usual to put the = on the first line?

extensions/java/xpcom/nsJavaWrapper.cpp
-  nsresult invokeResult = XPTC_InvokeByIndex(realObject, methodIndex,
+  nsresult invokeResult = NS_InvokeByIndex(realObject, methodIndex,
                                              paramCount, params);

the second line now has wrong indentation :)

extensions/java/xpcom/nsJavaXPTCStub.cpp
+nsJavaXPTCStub::nsJavaXPTCStub(jobject aJavaObject, nsIInterfaceInfo *aIInfo,
+                               nsresult &rv)

might be nicer to pass rv as a pointer so that it's more obvious that it's an out parameter

xpcom/reflect/xptcall/src/xptcall.cpp
+        nsISupports* sthis = this;
+        NS_ADDREF(sthis);
+        *aInstancePtr = NS_REINTERPRET_CAST(void*, sthis);

NS_ADDREF_THIS(); ?

Comment 5

[Darin Fisher]

Comment on attachment 235073 [details] [diff] [review]
NS_GetXPTCallStub love, rev. 1

I'm sorry, I won't have time to give this a thorough review.  I suggest finding another reviewer.

Comment 6

[Benjamin Smedberg]

Attachment: NS_GetXPTCallStub love, rev. 2

Comment 7

[timeless]

Comment on attachment 242194 [details] [diff] [review]
NS_GetXPTCallStub love, rev. 2

please make this pattern not crash on oom:
new nsXPTCVariant[paramCount]
     fullPars[i].Init(params[i], paramInfo.GetType(), flags);

+    *aInstancePtr = stub->mXPTCStub;;

please watch for doubled ;'s

+nsJavaXPTCStub::nsJavaXPTCStub(jobject 
aIInfo->GetIIDShared(&iid);

please assert that this doesn't fail.

+ * A helper class which initializes an xptcall helper at construction

'that' i think.

this link is bad, how about bonsai pinning it?
 The <b><i>invoke</i></b> functionality requires the implementation of the
 following on each platform (from <a href="http://lxr.mozilla.org/mozilla/source/xpcom/reflect/xptcall/public/xptcall.h#131">xptcall/public/xptcall.h</a>):
 
 <pre>
 XPTC_PUBLIC_API(nsresult)
+NS_InvokeByIndex(nsISupports* that, PRUint32 methodIndex,

same goes for the next place in this file

+ * @note No objects are addrefed on return of this function.

/awk/. rewrite :)

+ * Destroys an XPTCall stub previous created with NS_GetXPTCallStub.

previously.

+NS_GetXPTCallStub(REFNSIID aIID, nsIXPTCProxy* aOuter,

someone should either ASSERT or ENSURE aOuter.

bsmedberg of course promises to try to fix the indentation even in the cases where they weren't his fault :)

Comment 8

[Benjamin Smedberg]

Attachment: NS_GetXPTCallStub love, rev. 2.1

This is the patch for checkin. I'm going to post to the newsgroups asking for volunteers to fixup the xptcall ports, and try to commit next Monday.

Comment 9

[Peter Weilbacher]

Benjamin, you reference nsXPTCUtils.h in your patch several times, but it doesn't seem to be included. Does that come in from work in another bug/patch or is it missing from this one (or did I mess up my tree again)?

Comment 10

[Peter Weilbacher]

Attachment: Corresponding OS/2 changes

These are the changes to the relevant files for OS/2. It builds xpcom\reflect\xptcall\src\md\os2\ fine with this patch and the one from attachment 244923 [details] [diff] [review], but without the missing header file I cannot build a final product to test if it still works. (Btw, thanks for the heads-up in the newsgroup that pointed to this bug!)

Comment 11

[Benjamin Smedberg]

Checked this in, had issues with xpcshell not linking on linux. I can't reproduce. I'm a little frustrated.

Comment 12

[Benjamin Smedberg]

Fixed on trunk. Turns out the Linux x86 error was a remaining reference to XPTC_InvokeByIndex in the arch-specific assembly file.

Port owners, please file new bugs to get port-specific fixes in as a result of this change. I am happy to review.

Comment 13

[David Woodhouse]

There's a patch attached to bug 361415 which should update PPC32 for this too, as well as implementing it all for PPC64.

Comment 14

[Antonio Gomes (tonikitoo)]

maybe bug 361533 would be reasonable reason to reopen this bug ?

Comment 15

[Oleg Romashin (:romaxa)]

Attachment: xptcall ARM fix

Comment 16

[Antonio Gomes (tonikitoo)]

romaxa, submit your patch to bug 361533 also ;)

Comment 17

[Antonio Gomes (tonikitoo)]

maybe arm patch misses something ... it builds, but crashes on launching

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: Tested patch on ARM Linux.

Comment 19

[falk]

Attachment: Tested patch on Alpha Linux.

This patch fixes compilation for Alpha Linux. Note that I have no clue as to what I'm doing, I just copied the changes from the Arm patch and tested them.

Comment 20

[Willie Walker]

Attachment: Patch for Linux PPC [checked in]

Patch for Linux PPC.  I'm a Mozilla internals neophyte and have done this patch with the guidance of David Bolter, Christian Biesinger, Aaron Leventhal, and Ginn Chen (thanks guys!).  I have no clue if what I've done here is correct, but I'm submitting this patch using Firefox built from CVS HEAD on my Mac PowerBook G4.

Comment 21

[Mike Hommey [:glandium]]

Is there a particular reason for a bunch of architectures not to have been updated ?

Comment 22

[Benjamin Smedberg]

I did not update architectures which didn't have tinderboxes and which I didn't have build environments for. I did post to the newsgroups notifying the port owners that a change was needed.

Comment 23

[Max Stepanov]

Attachment: XPTCallStub changes to make WSDL working again

Hi Benjamin,

I've been trying to restore WSDL support for Jaxer server (based on Firefox 3 sources) and came with the following patch. Existing WSDL tests are working fine. So I would like to contribute it to Mozilla and get some feedback/review.

All changes are around an additional call NS_GetXPTCallStubII() which creates nsXPTCStubBase from an nsIInterfaceInfo*. 

Thanks for your attention,
Max

Comment 24

[Benjamin Smedberg]

Max, kindly file a new bug indicating what the bug is you're trying to fix and attach your patch to the new bug with the appropriate review flags.