100751 - foo.Adopt(ToNewXXX(bar)) defeats sharing

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect, P3)

Description

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

The patterns foo.Adopt(ToNewXXX(bar)) where |ToNewXXX| is one of
|ToNewUnicode(nsAString&)| or |ToNewCString(nsACString&)| (which don't do
conversion) are harmful because they defeat sharing (for strings on which
sharing is implemented).  There aren't too many of these in the tree yet
(although there are a few), but jag is planning to add a bunch more for bug
100476 (since apparently some of the assignment operations on nsXPIDL[C]String
don't work completely yet, or are at least rumored not to).

This pattern should be eliminated in favor of a simple assignment (operator= or
Assign) that allows sharing.

Comment 1

[Bradley Baetz (:bbaetz)]

nsXPIDL[C]String::Assign does crash, as do the related += operators, and so on.

Comment 2

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

Attachment: the patch we would want for the instances without encoding conversion

Comment 3

[jag (Peter Annema)]

-> 0.9.7

Comment 4

[jag (Peter Annema)]

dbaron's doing this as part of another bug, so giving it to him.

Comment 5

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

mozilla/mailnews/imap/src/nsImapProtocol.cpp also needs to be fixed

Comment 6

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

I still need to fix
http://lxr.mozilla.org/seamonkey/source/mailnews/imap/src/nsImapProtocol.cpp#6549
but other than that these need to wait for our converters (and generators?)
story to be fixed.

A converter-based assign is better than this pattern since it could allow fused
allocation of buffer handle and buffer.

Comment 7

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

Index: nsImapProtocol.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/imap/src/nsImapProtocol.cpp,v
retrieving revision 1.380
diff -u -d -r1.380 nsImapProtocol.cpp
--- nsImapProtocol.cpp	4 Dec 2001 00:25:58 -0000	1.380
+++ nsImapProtocol.cpp	8 Dec 2001 01:23:19 -0000
@@ -6551,7 +6551,7 @@
     // we are in the imap thread so *NEVER* try to extract the password with UI
    // if logon redirection has changed the password, use the cookie as the password
     if (m_overRideUrlConnectionInfo)
-      password.Adopt(ToNewCString(m_logonCookie));
+      password.Assign(m_logonCookie);
     else
       rv = server->GetPassword(getter_Copies(password));
     rv = server->GetRealUsername(&userName);

Comment 8

[jag (Peter Annema)]

r=jag

Comment 9

[Johnny Stenback (:jst)]

sr=jst

Comment 10

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

Fix checked in 2001-12-08 14:42 PDT.  That fixes all the cases that don't do
conversion.

We still need to fix the cases that do do conversion -- we should be allocating
a contiguous buffer and handle, probably using some generator mechanism.  Moving
to Future.

Comment 11

[Cathleen]

dbaron landed the fix on 12/08/2001 14:42

Comment 12

[jag (Peter Annema)]

Reopening. This was only partially fixed (see his last comment).

Comment 13

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

yep