85271 - [API] eliminate assignment into |nsXPIDLCString| when the intent is to adopt; eliminate |getter_Shares|

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Scott Collins]

Before |nsXPIDLC?String| can be brought into the string hierarchy, cases where
clients use assignment, but mean to re-bind must be fixed, as well as uses of
|getter_Shares|, which need to be replaced with |nsDependentC?String|s.

Comment 1

[Scott Collins]

There are fewer than 15 uses of |getter_Shares| in the mozilla tree (and none in
Netscape's commercial tree).

  http://lxr.mozilla.org/mozilla/search?string=getter_Shares

Comment 2

[Scott Collins]

Assignment turns out only to be implemented for the C-string variant, and it
doesn't just mean re-bind, it means re-bind to a clone of...  Yuck.

Comment 3

[Scott Collins]

I really want to make Patrick Beard fix every instance of the assignment form. 
I haven't seen one place where it is used correctly, and 90% of the uses are
actually leaks.  You introduced this misbegotten operator, Patrick!  Oy!  Common
use:

  mHTTPSProxyHost = nsCRT::strdup(tempString);

mmm, leak.  Other wonderful repercussions are pointless extra copies.  Patch anon.

Comment 4

[Patrick C. Beard]

Sorry about that... I've certainly been reporting those leaks. Folks just started 
using that form incorrectly, because they could. Of course, this may have come 
about because people changed the declaration of their member variables from char* 
to nsXPIDLCString w/o examinging all of the consequences. I'd be happy to help in 
any way I can.

Comment 5

[Scott Collins]

Attachment: eliminate assignment in favor of rebind; fix leaks

Comment 6

[Scott Collins]

Remarkably enough, a large fraction of these changes are in Necko.  If you want
to help, Patrick, you can review those changes and make sure I'm not out of my
head.  I'll send mail as well (as per mozilla.org policy).  You could also apply
the patch to nsXPIDLString.(h|cpp), and see what needs to be fixed in Netscape's
commercial tree :-)

Comment 7

[Chris Waterson]

Instead of |#if 0|-ing |nsXPIDLCString::operator=()|, shouldn't you declare it
private?

Comment 8

[Scott Collins]

(a) it has a |private| |operator=| already, and (b) this class will be replaced
entirely as soon as I get rid of the uses of |operator=| and |getter_Shares|. 
The new class with the same name will inherit from |nsSharableC?String| and be
in the public directory rather than obsolete.  It will be a full member of the
string hierarchy, which is why |operator=| has to go.  |operator=| for all other
strings means `copy characters from', not `take ownership of'.

Comment 9

[Chris Waterson]

Ah, you are right. In that case, just nuke the code: cvs can remember that we
once had an |operator=(const char*)|. That said...

What about doing something like the hack that dbaron ``invented'' (at least, I
think it was dbaron that I first saw doing this)? Specifically,

  nsXPIDLCString foo, bar;
  *getter_Copies(foo) = strdup("hello");
  *getter_Shares(bar) = "hello";

It is a bit ugly, but it's unambiguous. (Even Rebind() is ambiguous, because a
|const char*| is a |char*|.) What do you think?

Comment 10

[Scott Collins]

Chopping out is good, I'll attach a small patch that is just those two files if
desired.  The implementation of |Rebind| is exactly |*getter_Copies(*this) =
aNewValue;|  I originally was using the completely-spelled-out form (as you
suggest), but went back and repaired it all, because I didn't think it was
clear.  Even in the future, using the spelled-out form and assigning through
|getter_Copies| will continue to be legal, though I would probably continue to
recommend |Rebind| over it as more descriptive.  Plus, other reference-form
strings use |Rebind| in this way.  |getter_Shares| (used in only about a dozen
places) has to be replaced with something entirely different.  The replacement
class for |nsXPIDLC?String| _always_ owns.  |getter_Shares| is just going away
in favor of making dependent strings, e.g., |nsDependentC?String|.  I'll be
attaching an additional patch later today that does away with |getter_Shares|
entirely.

Comment 11

[Chris Waterson]

Okay, so should (can?) you make it _illegal_ to |Rebind()| a |const char*|,
then? I guess I can't see the whole picture from the gallery seats, but my
impression is that |Rebind()| is prone to the same usage errors that
|operator=(char*)| was. (Except now we'll crash at runtime, instead of leaking!)

For example,

  nsXPIDLCString foo;
  foo.Rebind(strdup("foo")); // right
  foo.Rebind("foo");         // wrong, but it compiles, and might
                             //   even run sometimes!

And even if you _can_ make the |Rebind(const char*)| verboten with some compiler
magic, we'll still get burned by:

  char* bar = /* some string I don't own */;
  nsXPIDLCString foo;
  foo.Rebind(bar);  // wrong!

I guess one of the things I really like about nsCOMPtr is that you _have_ to do
more typing to prove that you understand the ownership correctly. It seems like
that same principle should apply here, even if the
class-that-will-someday-replace-nsXPIDLCString always owns. Maybe my problem is
that the verbage of |Rebind()| is too weak. This would more clearly be an error:

  nsXPIDLCString foo;
  foo.AssumeOwnershipOfAndEventuallyDelete("bar");

Or maybe I just need to be conditioned to understand that |Rebind()| means
``assume ownership of and eventually delete''. Anyway, that's my $0.02 from the
cheap seats. I'll be happy to sr= the change as is, but guess I would prefer a
more committal method name! ;-)

cc'ing some other people who like to name things.

Comment 12

[Scott Collins]

|Rebind| takes a |char*| (or else |PRUnichar*|).  You've got it backwards :-)  A
|T*| can be used where a |const T*| is asked for, but not the reverse.

  str.Rebind("hello");

...is illegal because you would have to cast away |const|.  Does that make it
any better?  I could rename |Rebind| to |Adopt|?  That might make it clear.

Comment 13

[Chris Waterson]

Like I said, sr=waterson, but I'm concerned the name isn't strong enough. But I
don't have any better suggestions, either.

Comment 14

[Scott Collins]

Attachment: (replaces patch above) addresses waterson's comments, cutting instead of |#if|-ing

Comment 15

[Scott Collins]

Attachment: (replaces patches above) ...and removes implementation and uses of |nsXPIDLC?String::Copy|

Comment 16

[Scott Collins]

Attachment: (for review, but I will combine with patch above) eliminates |getter_Shares|

Comment 17

[Scott Collins]

Attachment: (replaces all patches above) previous two patches combined

Comment 18

[Scott Collins]

...all that remains before I can install the new implementation (to be provided
in bug #74726) is to eliminated implicit conversion into a pointer.  Attacking
that now.

Patrick ... you _did_ promise a review!

Comment 19

[Scott Collins]

Hmmm, I managed to miss the wide version: |nsXPIDLString::Copy|.  I'll attach a
patch that kills that first, then I'll fix the implicit pointer conversion.

Comment 20

[Patrick C. Beard]

r=beard

Comment 21

[Scott Collins]

Attachment: (replaces all patches above) all fixes above plus removal of wide |nsXPIDLString::Copy|

Comment 22

[Scott Collins]

I'd like to get one more pair of eyes on this before it gets checked in.  I'm
hoping for a review from jag (and I'll send mail).  It is ready to be checked in
as is, and that would allow for the new implementation of |nsXPIDLC?String|, as
long as that implementation also provides implicit conversion to a pointer. 
Jag: should I fix that as part of this patch?  Or should I wait because you've
already fixed this in your tree?

Comment 23

[Scott Collins]

Awaiting approval from drivers (and extra review from jag), this latest patch
(06/14/01 16:46) is what I want to check in to clear the way for the new
|nsXPIDLC?String| (see bug #74726).  This patch fixes leaks and simplifies
things.  It's broad, but mechanical and has both review and super-review (and
hopefully, soon, will have even a little extra review).

Comment 24

[Scott Collins]

Attachment: (replaces all patches above) as above, but (after a long discussion with waterson and debaron) s/Rebind/Adopt/g

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In the changes to nsXPIDL[C]String itself, can't you also remove |mBufOwner| and
|StartAssignmentByReference|?

Comment 26

[Scott Collins]

...I could, and would be happy to do so, but of course not long after I check
this in, I'll be checking in a whole new implementation of |nsXPIDLC?String| in
string/public (no longer obsolete).  I don't mind removing those things now;
just explaining why patches up this point didn't bother.  Patch to follow.

Comment 27

[Scott Collins]

Attachment: (replaces all patches above) as above ... but addresses dbaron's comments and removes |mBufOwner| and |StartAssignmentByReference|

Comment 28

[Chris Waterson]

Mmm, good. sr=waterson.

Comment 29

[Bradley Baetz (:bbaetz)]

From the nsDirectoryViewer.cpp patch:

-     r->GetValueConst(getter_Shares(dest));
+     const char* temp;
+     r->GetValueConst(&temp);
+     dest.Adopt(nsCRT::strdup(temp));

Won't this result in an additional string copy, compared with before? That code
went to a bit of trouble to reduce unneeded string copies (just on a matter of
principle, so its not really important performance-wise).

Comment 30

[Scott Collins]

Attachment: (replaces all patches above) as above, but brought up-to-date with the trunk

Comment 31

[Scott Collins]

BTW, I used the build that includes these changes to attach this latest patch. 
In case you were wondering if the patch could actually be expected to work :-)

Comment 32

[jag (Peter Annema)]

Index: mailnews/base/src/nsMsgAccount.cpp

I think alecf meant to remove the |return rv;| there, sent him an e-mail about it.


Index: mailnews/compose/src/nsURLFetcher.cpp

Why are you setting |contentType| and |charset| to |0| there? The old code
didn't do that, and it doesn't look right since the next line is a |if
(contentType)|.


Index: netwerk/base/src/nsProtocolProxyService.cpp

+        if (!NS_SUCCEEDED(rv))
+            mHTTPProxyHost.Adopt(nsCRT::strdup(""));

Why not use NS_FAILED? (x 5)


Index: security/manager/ssl/src/nsCrypto.cpp

-  args->m_jsCallback = jsCallback;
+  args->m_jsCallback.Adopt(jsCallback);

I think you need to strdup this.


Index: security/manager/ssl/src/nsNSSCallbacks.cpp

-      status->mCipherName = cipherName;
+      status->mCipherName.Adopt(cipherName);

And this...



Index: security/manager/ssl/src/nsNSSCertificate.cpp

-  serialNumber = CERT_Hexify(serialItem, 1);
+  serialNumber.Adopt(CERT_Hexify(serialItem, 1));

This one is fine though. Were we leaking that before?

Comment 33

[Scott Collins]

in nsURLFetcher.cpp: it might be clearer if you saw what followed the test :-)

  if (contentType)
    nsCRT::free(contentType);

I set it to |0| to prevent it being double-freed in the case where it was
|Adopt|ed by a |nsXPIDLC?String| which, from that point on, will manage that
data's lifespan.


in nsDirectoryViewer.cpp: in this case I had no choice, since |nsXPIDLC?String|s
no longer support sharing, this case (and I think one other) had to copy.  I
could have avoided this perhaps, as I did in many other cases, except that the
|nsXPIDLC?String| in question was a formal parameter.


in nsCrypto.cpp: |jsCallback| is produced on line 1424 with

  char *jsCallback = JS_GetStringBytes(jsString);

|JS_GetStringBytes| returns non-|const|, which implied to me that the result
carries with it ownership, and the receiver must free it.  Previously this would
have been a leak.  Is this not correct?  Should I have looked into the
implementation of |JS_GetStringBytes|?


in nsNSSCallbacks.cpp: hmmm, I did screw up here, but I think the right fix is
not to duplicate, but to chop out the

  PR_Free(cipherName);

that happens several lines later.


in nsNSSCertificate.cpp: "were we leaking that one before?"  Yes.


in nsMsgAccount.cpp: Yeah, that's probably right.  I'll chop out the return in
my patch.


in nsProtocolProxyService.cpp: I just left what was there.  I can change it to
|NS_FAILED| if you think that's important enough.  Do you?

Comment 34

[Scott Collins]

Attachment: (replaces all patches above) addresses jag's concerns in "nsNSSCallbacks.cpp" and "nsMsgAccount.cpp"

Comment 35

[Scott Collins]

hmm, |nsEscape| returns a new string, but |nsUnescape| returns a pointer to the
supplied string.  Therefore, the two uses of |nsUnescape| require copying.

Comment 36

[Scott Collins]

Attachment: (replaces all patches above) ... and copies the string in the (two) case(s) of |nsUnescape|

Comment 37

[Scott Collins]

Attachment: (replaces all patches above) ...adding a newline makes the "nsMsgAccount.cpp" patch not be malformed, and thus applicable on Unix

Comment 38

[Scott Collins]

Attachment: some unix specific patches

Comment 39

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 40

[jag (Peter Annema)]

in nsURLFetcher.cpp:

yeah, you're right, I should've checked that.

in nsCrypto.cpp:

and again you're right. Indeed we were leaking that.

r=jag

Comment 41

[Scott Collins]

mmm, building, running, and passing tests on Mac and Linux.  I'll run through
Windows and look for a commercial buddy before I try to check in, though.

Comment 42

[Scott Collins]

Attachment: (replaces all patches above) ...combined patch containing complete fixes for Linux

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 77960 has been marked as a duplicate of this bug. ***

Comment 44

[Scott Collins]

Attachment: (replaces all patches above) ...includes Windows fixes as well, and fixes an inconsistency in one of the hunks that made it not apply on Windows

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Two more comments, better late than never:

mailnews/mime/emitters/src/nsMimeHtmlEmitter.cpp :
  you changed the return value of the function

security/manager/ssl/src/nsCrypto.cpp :
  need to nsCRT::strdup (as bryner already changed in his checkin of the parts
in security since I pointed this out a few minutes ago)

Comment 46

[Scott Collins]

OK, this patch built (both mozilla and commercial) on Mac, Windows, and modern
Linux before checkin (there was trouble with GCC 2.7.2.3, and some of the
extensions not built everywhere, but we got that all repaired) and passed
pre-checkin tests on both Mac and Linux (and markh verified that it could read
news and browse on Windows).  Using my post-checkin builds, all seems well ...
except I have problems with two sites that I was not experiencing before.

  http://www.go2mac.com/
  http://www.macintouch.com/

It looks I broke something in the UR(I|L) or HTTP connection protocol.  I
verified this against a build made before my checkin (though it may be missing
other peoples checkins for about a day before), and that build does not have any
problem loading the two sites above.  It seems very likely my checkin either
caused or exposed this problem.  I'm going over my netwerk checkins with a fine
tooth comb now, but I am actively soliciting clues.  If you have one, please
help :-)

Comment 47

[Scott Collins]

trouble found and fixed, see bug #86316.  I'm now looking for any other cases
where I missed a |NULL| test before calling |nsCRT::strdup|.

Comment 48

[Scott Collins]

Attachment: fix other places where I needed to test for |NULL| before calling |nsCRT::strdup|

Comment 49

[Scott Collins]

final fixes checked in.  time, at last, to close this bug