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

RESOLVED FIXED in mozilla0.9.2

Status

()

RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: scc, Assigned: scc)

Tracking

Trunk
mozilla0.9.2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(16 attachments)

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
(Assignee)

Description

18 years ago
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.
(Assignee)

Updated

18 years ago
Blocks: 74726
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2
(Assignee)

Comment 1

18 years ago
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
(Assignee)

Updated

18 years ago
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|
(Assignee)

Updated

18 years ago
Blocks: 73009
(Assignee)

Comment 2

18 years ago
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|
(Assignee)

Comment 3

18 years ago
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

18 years ago
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.
(Assignee)

Comment 5

18 years ago
Created attachment 38129 [details] [diff] [review]
eliminate assignment in favor of rebind; fix leaks
(Assignee)

Comment 6

18 years ago
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

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

Comment 8

18 years ago
(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

18 years ago
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?
(Assignee)

Comment 10

18 years ago
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

18 years ago
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.
(Assignee)

Comment 12

18 years ago
|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

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

Comment 14

18 years ago
Created attachment 38418 [details] [diff] [review]
(replaces patch above) addresses waterson's comments, cutting instead of |#if|-ing
(Assignee)

Comment 15

18 years ago
Created attachment 38456 [details] [diff] [review]
(replaces patches above) ...and removes implementation and uses of |nsXPIDLC?String::Copy|
(Assignee)

Comment 16

18 years ago
Created attachment 38478 [details] [diff] [review]
(for review, but I will combine with patch above) eliminates |getter_Shares|
(Assignee)

Comment 17

18 years ago
Created attachment 38491 [details] [diff] [review]
(replaces all patches above) previous two patches combined
(Assignee)

Comment 18

18 years ago
...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!
(Assignee)

Comment 19

18 years ago
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

18 years ago
r=beard
(Assignee)

Comment 21

18 years ago
Created attachment 38515 [details] [diff] [review]
(replaces all patches above) all fixes above plus removal of wide |nsXPIDLString::Copy|
(Assignee)

Comment 22

18 years ago
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?
(Assignee)

Comment 23

18 years ago
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
(Assignee)

Updated

18 years ago
Keywords: patch
(Assignee)

Comment 24

18 years ago
Created attachment 38535 [details] [diff] [review]
(replaces all patches above) as above, but (after a long discussion with waterson and debaron) s/Rebind/Adopt/g
(Assignee)

Updated

18 years ago
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
|StartAssignmentByReference|?
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|
(Assignee)

Comment 26

18 years ago
...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.
(Assignee)

Comment 27

18 years ago
Created attachment 38543 [details] [diff] [review]
(replaces all patches above) as above ... but addresses dbaron's comments and removes |mBufOwner| and |StartAssignmentByReference|

Updated

18 years ago
Blocks: 83989

Comment 28

18 years ago
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).
(Assignee)

Comment 30

18 years ago
Created attachment 38556 [details] [diff] [review]
(replaces all patches above) as above, but brought up-to-date with the trunk
(Assignee)

Comment 31

18 years ago
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

18 years ago
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?
(Assignee)

Comment 33

18 years ago
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?
(Assignee)

Comment 34

18 years ago
Created attachment 38608 [details] [diff] [review]
(replaces all patches above) addresses jag's concerns in "nsNSSCallbacks.cpp" and "nsMsgAccount.cpp"
(Assignee)

Comment 35

18 years ago
hmm, |nsEscape| returns a new string, but |nsUnescape| returns a pointer to the
supplied string.  Therefore, the two uses of |nsUnescape| require copying.
(Assignee)

Comment 36

18 years ago
Created attachment 38623 [details] [diff] [review]
(replaces all patches above) ... and copies the string in the (two) case(s) of |nsUnescape|
(Assignee)

Comment 37

18 years ago
Created attachment 38631 [details] [diff] [review]
(replaces all patches above) ...adding a newline makes the "nsMsgAccount.cpp" patch not be malformed, and thus applicable on Unix
(Assignee)

Comment 38

18 years ago
Created attachment 38632 [details] [diff] [review]
some unix specific patches

Comment 39

18 years ago
a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)

Comment 40

18 years ago
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
(Assignee)

Comment 41

18 years ago
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.
(Assignee)

Comment 42

18 years ago
Created attachment 38670 [details] [diff] [review]
(replaces all patches above) ...combined patch containing complete fixes for Linux
*** Bug 77960 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 44

18 years ago
Created attachment 38797 [details] [diff] [review]
(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
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)
(Assignee)

Comment 46

18 years ago
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 :-)
(Assignee)

Comment 47

18 years ago
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|.
(Assignee)

Comment 48

18 years ago
Created attachment 38887 [details] [diff] [review]
fix other places where I needed to test for |NULL| before calling |nsCRT::strdup|
(Assignee)

Comment 49

18 years ago
final fixes checked in.  time, at last, to close this bug
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.