Last Comment Bug 349002 - Refactor xptcall into a frozen API
: Refactor xptcall into a frozen API
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.9alpha1
Assigned To: Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
:
Mentors:
Depends on: 342311 377948
Blocks: 361470 361533 368482 443234
  Show dependency treegraph
 
Reported: 2006-08-17 07:08 PDT by Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
Modified: 2010-04-12 11:35 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
NS_GetXPTCallStub love, rev. 1 (153.31 KB, patch)
2006-08-23 07:45 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
no flags Details | Diff | Splinter Review
NS_GetXPTCallStub love, rev. 2 (150.56 KB, patch)
2006-10-13 08:56 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
timeless: review+
Details | Diff | Splinter Review
NS_GetXPTCallStub love, rev. 2.1 (134.24 KB, patch)
2006-11-07 13:15 PST, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
no flags Details | Diff | Splinter Review
Corresponding OS/2 changes (3.20 KB, patch)
2006-11-14 10:49 PST, Peter Weilbacher
no flags Details | Diff | Splinter Review
xptcall ARM fix (3.71 KB, patch)
2006-11-24 04:38 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Tested patch on ARM Linux. (3.09 KB, patch)
2006-12-02 09:51 PST, Oleg Romashin (:romaxa)
benjamin: review+
Details | Diff | Splinter Review
Tested patch on Alpha Linux. (3.04 KB, patch)
2006-12-11 16:28 PST, falk
no flags Details | Diff | Splinter Review
Patch for Linux PPC [checked in] (4.37 KB, patch)
2007-02-28 05:31 PST, Willie Walker
benjamin: review+
benjamin: superreview+
Details | Diff | Splinter Review
XPTCallStub changes to make WSDL working again (5.50 KB, patch)
2008-08-07 12:30 PDT, Max Stepanov
no flags Details | Diff | Splinter Review

Description Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2006-08-17 07:08:47 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2006-08-23 07:45:21 PDT
Created attachment 235073 [details] [diff] [review]
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 2006-08-23 11:19:09 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2006-08-23 11:47:47 PDT
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) 2006-09-04 17:52:55 PDT
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 2006-10-12 00:07:18 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2006-10-13 08:56:05 PDT
Created attachment 242194 [details] [diff] [review]
NS_GetXPTCallStub love, rev. 2
Comment 7 timeless 2006-10-24 11:07:30 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2006-11-07 13:15:36 PST
Created attachment 244923 [details] [diff] [review]
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 2006-11-14 10:15:22 PST
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 2006-11-14 10:49:11 PST
Created attachment 245586 [details] [diff] [review]
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2006-11-16 13:19:39 PST
Checked this in, had issues with xpcshell not linking on linux. I can't reproduce. I'm a little frustrated.
Comment 12 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2006-11-20 14:08:23 PST
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 2006-11-22 04:22:17 PST
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) 2006-11-24 03:54:12 PST
maybe bug 361533 would be reasonable reason to reopen this bug ?
Comment 15 Oleg Romashin (:romaxa) 2006-11-24 04:38:51 PST
Created attachment 246459 [details] [diff] [review]
xptcall ARM fix
Comment 16 Antonio Gomes (tonikitoo) 2006-11-24 04:47:08 PST
romaxa, submit your patch to bug 361533 also ;)
Comment 17 Antonio Gomes (tonikitoo) 2006-11-27 04:04:06 PST
maybe arm patch misses something ... it builds, but crashes on launching
Comment 18 Oleg Romashin (:romaxa) 2006-12-02 09:51:19 PST
Created attachment 247274 [details] [diff] [review]
Tested patch on ARM Linux.
Comment 19 falk 2006-12-11 16:28:24 PST
Created attachment 248318 [details] [diff] [review]
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 2007-02-28 05:31:43 PST
Created attachment 256782 [details] [diff] [review]
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] 2008-04-21 00:24:49 PDT
Is there a particular reason for a bunch of architectures not to have been updated ?
Comment 22 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2008-04-21 05:48:23 PDT
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 2008-08-07 12:30:42 PDT
Created attachment 332774 [details] [diff] [review]
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2008-08-07 12:33:08 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.