Closed Bug 125466 Opened 22 years ago Closed 22 years ago

Add CString and UTF8String support to XPCOM's proxy event mechanism


(Core :: XPCOM, defect)

Windows 2000
Not set





(Reporter: nisheeth_mozilla, Assigned: nisheeth_mozilla)




(1 file, 2 obsolete files)

See summary.
Target Milestone: --- → mozilla0.9.9
-> 1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
Keywords: mozilla1.0
Met with jband and dougt on this today.  We've decided to break up the work as

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!
Component: XPConnect → XPCOM
Blocks: 128560
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.
Keywords: mozilla1.0mozilla1.0+
Attached patch 1st attempt at fix (obsolete) — Splinter Review
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

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+
Blocks: 129613
No longer blocks: 129613
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.
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.