Closed Bug 125466 Opened 22 years ago Closed 22 years ago
Add CString and UTF8String support to XPCOM's proxy event mechanism
Target Milestone: mozilla0.9.9 → mozilla1.0
Met with jband and dougt on this today. We've decided to break up the work as follows: a) Do simple string copying for the new string types across the thread boundary for the sync proxy case (nisheeth) b) Assume that only in params are passed into the async proxy. Assert on "dipper" params (AString and DOMString) where the caller allocates the object and the callee fills in the object with data. (nisheeth) c) Add code that checks method invocations on async proxied interfaces for out and in-out parameters. If found, such method invocations should fail with a well known error code. (dougt) d) Fix places in the code that fail after c) is implemented. a) and b) willl be tracked in this bug. I've created bug 128560 on dougt to track c) and d). Doug, jband, please update if I've missed something. Thanks!
Status: NEW → ASSIGNED
In comment 2, task d) is owned by dougt. :-)
Setting default QA -
QA Contact: pschwartau → scc
Plussing for 1.0. We expect these new UTF8 and CString types to be used in networking code, such as LDAP, which depends heavily on cross-thread calling.
Here's a 1st try at fixing this. Doug, please review. I don't need to do anything for the sync case, right? For the async case, I've added appropriate case statements and reworked the code layout a bit. Also, I had to move the temporary utf8 related string defines to xpt_struct.h from nsVariant.h so that they can be used by the variant *and* the proxy code. Jag, I'm using the C++ new operator to create and the delete operator to delete nsString, nsCString, and nsUTF8String (currently maps to nsCString). Should I try to use the nsMemory API instead? Thanks for your help!
Nah, new/delete are fine.
Comment on attachment 72543 [details] [diff] [review] 1st attempt at fix I don't know about moving stuff from nsVariant.h to xpt_struct.h. Is that okay with jband? could you use nsCRT::free instead of Recycle? or do a PL_strdup/PL_strfree instead? Assuming it all works and everything, r=dougt.
Sure, I'll use PL_strdup/PL_strfree. The PL_strdup/Recycle combination was there before my patch so I didn't wanna change it. Jag has already reviewed the string allocations/deletions for the new string types. So, now onwards to an sr.
JBand, would you please sr= this patch? Thanks!
Attachment #72543 - Attachment is obsolete: true
Comment on attachment 72729 [details] [diff] [review] V 2.0 of patch that incorporates dougt's feedback > I don't know about moving stuff from nsVariant.h > to xpt_struct.h. Is that okay with jband? No. I was going to bitch about this even before seeing this comment. xpt_struct.h knows nothing about those types. Moving the hacky defines there just because it is commonly included is not good. Put the hacky defines in the the cpp files that may have to change anyway when the real types appear. > + void *ptr = mParameterList[i].val.p; I think it is clearer if you do this *first* (before the null check) and fix the so many places that mParameterList[i].val.p is used on the RHS to use ptr instead. I'd like explicit 'default:'s with a comment saying that other types are ignored. This helps make the code readable and has no cost.
Attachment #72729 - Flags: needs-work+
I think there is value to having the defines that need to be removed in just one place rather than copying them out into multiple places. At least for the xpcom/proxy case, the usage of the utf8string type is simple enough that it will very likely *not* need to change when jag lands the utf8string implementation. But I'm not gonna argue it out with you. Its easier just to make the change you suggest. I've moved the defines back to nsVariant.h where they were put earlier on a patch sr'd by you. And I've copied a subset of them into nsProxyEvent.cpp. I'm about to attach a revised patch with this and the other changes suggestd by you. Please sr. Thanks!
Here we go! Jband, please sr.
Attachment #72729 - Attachment is obsolete: true
Comment on attachment 73101 [details] [diff] [review] V 3.0 of patch that incorporates jband's feedback sr=jband. I'm happy.
Attachment #73101 - Flags: superreview+
Comment on attachment 73101 [details] [diff] [review] V 3.0 of patch that incorporates jband's feedback a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #73101 - Flags: approval+
Fix checked into the trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.