78824 - Impl |AdoptData|, |AdoptDataWithLength| on nsISupportsWString

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Dan Rosen]

nsISupportsWString needs AdoptData and AdoptDataWithLength methods to provide a
non-copying alternative to SetData and SetDataWithLength. This will support the
pattern:

  nsAString string;
  PRUnichar* unicharBuf(ToNewUnicode(string));
  nsISupportsWString* wstring( ... );
  wstring->AdoptData(unicharBuf);

(whereas SetData would have leaked the charbuf).

Comment 1

[Dan Rosen]

Attachment: Patch adding methods to nsISupportsString and nsISupportsWString, and cleaning up minor sloppiness in nsSupportsPrimitives.cpp

Comment 2

[Dan Rosen]

Attached patch implements AdoptData and AdoptDataWithLength on both
nsISupportsString and nsISupportsWString. Note that these are [noscript], as we
are explicitly subsuming a char buffer, which wouldn't fly in XPConnect.

I also cleaned up some tabbing inconsistencies and removed a couple
else-after-returns in nsSupportsPrimitives.cpp.

scc: I'm pretty sure I have the calls to nsMemory correct, and I have tested
this code, but I still need a sharp review to be sure.

(marking blocker of 78010, since my patch for that bug requires this)

Comment 3

[Dan Rosen]

Attachment: [untested] revised patch, doing the right xpcom/xpidl thing with arguments, and reworking internals

Comment 4

[Dan Rosen]

Attachment: D'oh! Previous patch was wrong. This is the right patch... [still untested]

Comment 5

[Dan Rosen]

jband: Can you please review this code? The intent is to add a set of AdoptData
methods to the nsISupports[W]String interfaces, to subsume a char buffer into
the wrapper interface rather than to copy it.

Relevant info: In scc's initial review, he asked me to rewrite the SetData
routines in terms of AdoptData, and to add a |length| member var to cache the
length of the buffer. The addition of the native |unicharPtr| type to rootidl
was brendan's suggestion (using the native ptr types was necessary, since we're
subsuming the buffer -- using the xpidl |string| and |wstring| types wouldn't
work since they're const as in-params). Other changes to the
nsSupportsPrimitives code are just routine cleanup.

The one thing I'd still like to do, potentially, after this lands, is to
templatize the two string wrappers on their char type -- all the code for
nsSupportsWStringImpl is duplicated from nsSupportsStringImpl....

Comment 6

[Dan Rosen]

I should also mention that the reason for not integrating this more tightly with
scc's string classes (which jag was quite rightfully concerned about) is that
the semantics of the ISupports string classes is solely that of a wrapper for
char buffers.

Comment 7

[Edward Kandrot]

My main comment would be in SetDataWithLength to not alloc a new buffer unless
the size of the old buffer is different then that of the new string length.

Comment 8

[John Bandhauer]

Dan: Undo all the whitespace changes. You can fix the emacs Mode line to 
reflect the real 4 space indent in the file. There is no need to 
inconsistently reformat whitespace here.

Why are you rewriting code that seems to work? e.g. GetData. Why not just 
substitute nsCRT::strlen(mData) with mLength and get on with your life?

Comment 9

[Dan Rosen]

John: I'll go back and fix the whitespace changes, and (reluctantly) change the 
emacs modeline. The changes to working code, namely Get and SetData, are per 
scc's request (I'm wishing more and more he'd posted his comments to the 
bug...).

Comment 10

[Scott Collins]

The old code always knew the length, John, or could have, since it was always
provided at |SetData| time; and always needed it at |GetData| time to do the
allocation---but none the less, always recalculated it.  The annoying side
effects of this were that the (old) implementation always did extra work and was
needlessly incompatible with data-strings containing NULLs.

Comment 11

[John Bandhauer]

scc: this is a completely trivial point... I didn't have a problem with storing 
the length. I was just unsure about why the functions were rewritten rather than 
making a small change.

I'll look closer at the thing that this bug is trying to actually fix when the 
next patch comes along.

Comment 12

[Scott Collins]

:-)  Hope I didn't sound snippy or anything.  I was just proffering my
explanation of why he wasn't just calling |nsCRT::strlen|.  In general, the
advice I first offered him was that, since he was adding these two new routines,
he could simplify the rest of the implementation by leveraging them; and that
there was another small win that could go along with that, by storing the
length.  I still stand by that ... with the caveat that the |Set...| routines
actually wouldn't call through to the |Adopt...| routines in the case where
sufficient storage space was already allocated, as you point out above.

Comment 13

[Dan Rosen]

Attachment: The hopefully less controversial next revision. Reverts tabs per jband, doesn't realloc when unnecessary per kandrot.

Comment 14

[Dan Rosen]

So along comes the next patch. The attachment comment and code should be
self-explanatory. Note also that I put assertions back in a couple places where
they originally were, except for in GetData (which sets |*aData = nsnull| to
start with).

I should say that John is entirely correct in pointing out that the semantics of
GetData are unchanged. It's rewritten more or less for clarity. If you *really*
want me to, I can change GetData back to just replace the call to |strlen| with
mLength, and "get on with my life" :)

Comment 15

[John Bandhauer]

It would be nice if you said you'd actually tested this stuff.
r/sr=jband

Comment 16

[Dan Rosen]

Yes, it's tested. Thanks, John. Scott or Edward, can you also give a quick
second review?

Comment 17

[Edward Kandrot]

AdoptDataWithLength is an entry point where someone could upset the underlying 
assumption of null termination.  If you always write a '\0' to |length| in this 
function, then r=kandrot.

Comment 18

[Dan Rosen]

Attachment: final patch, with null-termination safety cleanup per kandrot

Comment 19

[Dan Rosen]

fixed

Comment 20

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old