499027 - Copying an empty string to a new string causes it to allocate a buffer

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. nsCString a, b;
2. a = b;
Expected result: nothing happens

Actual results: a now has a sharable buffer

Comment 1

[[e-mail]]

Attachment: Possible patch

As well as failing, if you compile with string stats you would notice that the new test consumes an extra string buffer without the empty copy optimisation.

I'm not sure that the test correctly tests for a new string buffer creation.

I also fixed an inconsistent memory allocation since I build with them fatal.

Comment 2

[[e-mail]]

Attachment: Correct patch

Sorry, I forgot to check what I was diffing...

Comment 3

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

Comment on attachment 384139 [details] [diff] [review]
Correct patch

I hate the way we implement VOIDED.  (I wish it were orthogonal to what was in the buffer.)  But anyway...

>+    if (str.mFlags & F_VOIDED)
>+      {
>+        // inherit the F_VOIDED attribute
>+        SetIsVoid(PR_TRUE);
>+      }
>+    else if (!str.mLength)
>+      {
>+        Truncate();
>+      }

Isn't it true that F_VOIDED should only be set when mLength is zero?  Can't we assert that and then combine these two clauses:

if (!str.mLength)
  {
    if (str.mFlags & F_VOIDED)
      SetIsVoid(PR_TRUE);
    else
      Truncate();
  }

or perhaps even:

if (!str.mLength)
  {
    Truncate();
    mFlags |= (str.mFlags & F_VOIDED);
  }



r=dbaron, but I'd like jst to sr to make sure that my assumption that it's expected for F_VOIDED to be propagated on assignment is correct (since I suppose right now that only happens some of the time).

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 384139 [details] [diff] [review]
Correct patch

sr=jst. I do think we should propagate the voided flag on assignment consistently.

Comment 5

[[e-mail]]

We already do, since we never set both F_SHARED and F_VOIDED simultaneously.

Comment 6

[[e-mail]]

Attachment: Addressed review comment

Comment 7

[[e-mail]]

Pushed changeset f319ed9df9d4 to mozilla-central.

Comment 8

[Karl Tomlinson (:karlt)]

Very good Tp and sspider improvements here :)

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get a mozilla-1.9.2 patch nominated for approval, please?

Comment 10

[[e-mail]]

Pushed changeset dcc67c2cb0f1 to releases/mozilla-1.9.2