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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nisheeth Ranjan]

See summary.

Comment 1

[Nisheeth Ranjan]

-> 1.0

Comment 2

[Nisheeth Ranjan]

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!

Comment 3

[Nisheeth Ranjan]

In comment 2, task d) is owned by dougt.  :-)

Comment 4

[Phil Schwartau]

Setting default QA -

Comment 5

[Mike Shaver (:shaver emeritus)]

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.

Comment 6

[Nisheeth Ranjan]

Attachment: 1st attempt at fix

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!

Comment 7

[jag (Peter Annema)]

Nah, new/delete are fine.

Comment 8

[Doug Turner (:dougt)]

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.

Comment 9

[Nisheeth Ranjan]

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.

Comment 10

[Nisheeth Ranjan]

Attachment: V 2.0 of patch that incorporates dougt's feedback

JBand, would you please sr= this patch?  Thanks!

Comment 11

[John Bandhauer]

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.

Comment 12

[Nisheeth Ranjan]

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!

Comment 13

[Nisheeth Ranjan]

Attachment: V 3.0 of patch that incorporates jband's feedback

Here we go!  Jband, please sr.

Comment 14

[John Bandhauer]

Comment on attachment 73101 [details] [diff] [review]
V 3.0 of patch that incorporates jband's feedback

sr=jband. I'm happy.

Comment 15

[Asa Dotzler [:asa]]

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

Comment 16

[Nisheeth Ranjan]

Fix checked into the trunk.