90981 - Crash in GetReadableFragment [@ nsAFlatCString::GetReadableFragment ]

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Scott MacGregor]

scc and I fixed one of our top crashers on the trunk today. If you had an
nsXPIDLString which hadn't been initialized with a value and you then assigned
it into a string which inherits from nsFlatString then we crash.

i.e.
nsXPIDLCString foo;
nsCAutoString bar;

bar = foo;

boom.

We have a patch which I'll post next.

Comment 1

[Scott MacGregor]

keyword mumbo jumbo

Comment 2

[Scott MacGregor]

Attachment: the patch from scc.

Comment 3

[Scott MacGregor]

the fix comes from scc. 

sr=mscott
we just need an r= now.

Comment 4

[Scott Collins]

Attachment: (replaces patch above) ...with indentation to match the rest of the file [curse you, VC++ editor]

Comment 5

[Scott Collins]

as I discussed with mscott, this patch is appropriate for the trunk, but is not
needed on the branch

Comment 6

[Scott MacGregor]

sr=mscott

Comment 7

[Scott Collins]

rs=waterson, and now it is checked in; revision 1.6 of "string/src/nsAString.cpp"

Comment 8

[Scott Collins]

...and marking fixed

Comment 9

[Ed Stetz]

Oh come on!

--------------------------------------------------------------------------------
@@ -245,11 +245,14 @@
   {
     SetLength(0);
-    SetLength(aReadable.Length());
-      // first setting the length to |0| avoids copying characters only to be 
overwritten later
-      //  in the case where the implementation decides to re-allocate
-
-    const_iterator fromBegin, fromEnd;
-    iterator toBegin;
-    copy_string(aReadable.BeginReading(fromBegin), 
aReadable.EndReading(fromEnd), BeginWriting(toBegin));
+    if ( !aReadable.IsEmpty() )
+      {
+        SetLength(aReadable.Length());
+          // first setting the length to |0| avoids copying characters only to 
be overwritten later
+          //  in the case where the implementation decides to re-allocate
+
+        const_iterator fromBegin, fromEnd;
+        iterator toBegin;
+        copy_string(aReadable.BeginReading(fromBegin), 
aReadable.EndReading(fromEnd), BeginWriting(toBegin));
+      }
   }
--------------------------------------------------------------------------------

At least put the comments someplace where they make sense:

--------------------------------------------------------------------------------
+        SetLength(aReadable.Length());
+          // first setting the length to |0| avoids copying characters only to 
be overwritten later
+          //  in the case where the implementation decides to re-allocate
--------------------------------------------------------------------------------

... like with the SetLength(0) call at the top.
Ditto for the mirror image changes in the second half of the patch.

Who puts comments for a block of code *after* the block instead of before it?

Is the call to SetLength(0) even needed?  It seems a little odd in the original
code as to why it was necessary to do two SetLength's in a row.

-- Ed