The default bug view has changed. See this FAQ.

Refactor xptcall into a frozen API

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XPCOM
P2
normal
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Updated

11 years ago
Assignee: nobody → benjamin
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 1

11 years ago
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.
Attachment #235073 - Flags: review?(darin)

Comment 2

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

Comment 3

11 years ago
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 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

11 years ago
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.
Attachment #235073 - Flags: review?(darin)
(Assignee)

Comment 6

11 years ago
Created attachment 242194 [details] [diff] [review]
NS_GetXPTCallStub love, rev. 2
Attachment #235073 - Attachment is obsolete: true
Attachment #242194 - Flags: review?(timeless)

Comment 7

11 years ago
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 :)
Attachment #242194 - Flags: review?(timeless) → review+
(Assignee)

Comment 8

11 years ago
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

11 years ago
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

11 years ago
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!)
(Assignee)

Comment 11

11 years ago
Checked this in, had issues with xpcshell not linking on linux. I can't reproduce. I'm a little frustrated.
(Assignee)

Comment 12

11 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Blocks: 361470

Comment 13

11 years ago
There's a patch attached to bug 361415 which should update PPC32 for this too, as well as implementing it all for PPC64.
Blocks: 361533
maybe bug 361533 would be reasonable reason to reopen this bug ?
Created attachment 246459 [details] [diff] [review]
xptcall ARM fix
Attachment #246459 - Flags: review?(timeless)
romaxa, submit your patch to bug 361533 also ;)
maybe arm patch misses something ... it builds, but crashes on launching
Created attachment 247274 [details] [diff] [review]
Tested patch on ARM Linux.
Attachment #246459 - Attachment is obsolete: true
Attachment #247274 - Flags: review?(timeless)
Attachment #246459 - Flags: review?(timeless)

Updated

10 years ago
Attachment #247274 - Flags: review?(benjamin)
(Assignee)

Updated

10 years ago
Attachment #247274 - Flags: review?(benjamin) → review+

Comment 19

10 years ago
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.

Updated

10 years ago
Blocks: 368482

Comment 20

10 years ago
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.

Updated

10 years ago
Attachment #256782 - Flags: superreview?(benjamin)
Attachment #256782 - Flags: review?(benjamin)
(Assignee)

Updated

10 years ago
Attachment #256782 - Flags: superreview?(benjamin)
Attachment #256782 - Flags: superreview+
Attachment #256782 - Flags: review?(benjamin)
Attachment #256782 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #256782 - Attachment description: Patch for Linux PPC → Patch for Linux PPC [checked in]
Depends on: 377948
Is there a particular reason for a bunch of architectures not to have been updated ?
(Assignee)

Comment 22

9 years ago
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.

Updated

9 years ago
Blocks: 443234

Comment 23

9 years ago
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
(Assignee)

Comment 24

9 years ago
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.
Attachment #247274 - Flags: review?(timeless)
You need to log in before you can comment on or make changes to this bug.