Closed
Bug 85271
Opened 24 years ago
Closed 24 years ago
[API] eliminate assignment into |nsXPIDLCString| when the intent is to adopt; eliminate |getter_Shares|
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla0.9.2
People
(Reporter: scc, Assigned: scc)
References
Details
Attachments
(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 | |
(replaces all patches above) addresses jag's concerns in "nsNSSCallbacks.cpp" and "nsMsgAccount.cpp"
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.
Assignee | ||
Updated•24 years ago
|
Assignee | ||
Comment 1•24 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•24 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 | ||
Comment 2•24 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•24 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•24 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•24 years ago
|
||
Assignee | ||
Comment 6•24 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•24 years ago
|
||
Instead of |#if 0|-ing |nsXPIDLCString::operator=()|, shouldn't you declare it private?
Assignee | ||
Comment 8•24 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•24 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•24 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•24 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•24 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•24 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•24 years ago
|
||
Assignee | ||
Comment 15•24 years ago
|
||
Assignee | ||
Comment 16•24 years ago
|
||
Assignee | ||
Comment 17•24 years ago
|
||
Assignee | ||
Comment 18•24 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•24 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.
Assignee | ||
Comment 21•24 years ago
|
||
Assignee | ||
Comment 22•24 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•24 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).
Assignee | ||
Comment 24•24 years ago
|
||
Assignee | ||
Updated•24 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|?
Assignee | ||
Comment 26•24 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•24 years ago
|
||
Comment 29•24 years ago
|
||
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•24 years ago
|
||
Assignee | ||
Comment 31•24 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•24 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•24 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•24 years ago
|
||
Assignee | ||
Comment 35•24 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•24 years ago
|
||
Assignee | ||
Comment 37•24 years ago
|
||
Assignee | ||
Comment 38•24 years ago
|
||
Comment 40•24 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•24 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•24 years ago
|
||
*** Bug 77960 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 44•24 years ago
|
||
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•24 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•24 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•24 years ago
|
||
Assignee | ||
Comment 49•24 years ago
|
||
final fixes checked in. time, at last, to close this bug
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Updated•4 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•