Closed Bug 85271 Opened 23 years ago Closed 23 years ago

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


(Core :: XPCOM, defect)

Not set





(Reporter: scc, Assigned: scc)




(16 files)

35.48 KB, patch
Details | Diff | Splinter Review
36.46 KB, patch
Details | Diff | Splinter Review
51.43 KB, patch
Details | Diff | Splinter Review
13.74 KB, patch
Details | Diff | Splinter Review
62.74 KB, patch
Details | Diff | Splinter Review
69.58 KB, patch
Details | Diff | Splinter Review
69.49 KB, patch
Details | Diff | Splinter Review
71.21 KB, patch
Details | Diff | Splinter Review
71.53 KB, patch
Details | Diff | Splinter Review
71.50 KB, patch
Details | Diff | Splinter Review
71.38 KB, patch
Details | Diff | Splinter Review
71.39 KB, patch
Details | Diff | Splinter Review
1.07 KB, patch
Details | Diff | Splinter Review
75.26 KB, patch
Details | Diff | Splinter Review
77.53 KB, patch
Details | Diff | Splinter Review
9.48 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 74726
Target Milestone: --- → mozilla0.9.2
There are fewer than 15 uses of |getter_Shares| in the mozilla tree (and none in
Netscape's commercial tree).
Summary: eliminate assignment into |nsXPIDLC?String| when the intent is to re-bind; eliminate |getter_Shares| → [API] eliminate assignment into |nsXPIDLC?String| when the intent is to re-bind; eliminate |getter_Shares|
Blocks: 73009
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.
Summary: [API] eliminate assignment into |nsXPIDLC?String| when the intent is to re-bind; eliminate |getter_Shares| → [API] eliminate assignment into |nsXPIDLCString| when the intent is to re-bind; eliminate |getter_Shares|
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

  mHTTPSProxyHost = nsCRT::strdup(tempString);

mmm, leak.  Other wonderful repercussions are pointless extra copies.  Patch anon.
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.
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 policy).  You could also apply
the patch to nsXPIDLString.(h|cpp), and see what needs to be fixed in Netscape's
commercial tree :-)
Instead of |#if 0|-ing |nsXPIDLCString::operator=()|, shouldn't you declare it
(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'.
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?
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|
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;

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.
|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"); 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.
Like I said, sr=waterson, but I'm concerned the name isn't strong enough. But I
don't have any better suggestions, either.
...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!
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.
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?
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).
Keywords: approval, patch, review
Keywords: patch
Summary: [API] eliminate assignment into |nsXPIDLCString| when the intent is to re-bind; eliminate |getter_Shares| → [API] eliminate assignment into |nsXPIDLCString| when the intent is to adopt; eliminate |getter_Shares|
In the changes to nsXPIDL[C]String itself, can't you also remove |mBufOwner| and
Keywords: approval, review
Summary: [API] eliminate assignment into |nsXPIDLCString| when the intent is to adopt; eliminate |getter_Shares| → [API] eliminate assignment into |nsXPIDLCString| when the intent is to re-bind; eliminate |getter_Shares|
Keywords: approval, review
Summary: [API] eliminate assignment into |nsXPIDLCString| when the intent is to re-bind; eliminate |getter_Shares| → [API] eliminate assignment into |nsXPIDLCString| when the intent is to adopt; eliminate |getter_Shares|
...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.
Blocks: 83989
Mmm, good. sr=waterson.
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).
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 :-)
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

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?
in nsURLFetcher.cpp: it might be clearer if you saw what followed the test :-)

  if (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


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?
hmm, |nsEscape| returns a new string, but |nsUnescape| returns a pointer to the
supplied string.  Therefore, the two uses of |nsUnescape| require copying.
a= for checkin to the trunk.
(on behalf of drivers)
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.

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.
*** Bug 77960 has been marked as a duplicate of this bug. ***
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)
OK, this patch built (both mozilla and commercial) on Mac, Windows, and modern
Linux before checkin (there was trouble with GCC, 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.

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 :-)
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|.
final fixes checked in.  time, at last, to close this bug
Closed: 23 years ago
Resolution: --- → FIXED
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.