Crash in GetReadableFragment [@ nsAFlatCString::GetReadableFragment ]

RESOLVED FIXED in mozilla0.9.3

Status

()

P2
critical
RESOLVED FIXED
18 years ago
9 years ago

People

(Reporter: mscott, Assigned: mscott)

Tracking

({crash, topcrash})

Trunk
mozilla0.9.3
x86
Windows 2000
crash, topcrash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

18 years ago
keyword mumbo jumbo
Severity: normal → major
Status: NEW → ASSIGNED
Keywords: topcrash
Priority: -- → P2
Target Milestone: --- → mozilla0.9.3
(Assignee)

Comment 2

18 years ago
Created attachment 42441 [details] [diff] [review]
the patch from scc.
(Assignee)

Comment 3

18 years ago
the fix comes from scc. 

sr=mscott
we just need an r= now.

Comment 4

18 years ago
Created attachment 42444 [details] [diff] [review]
(replaces patch above) ...with indentation to match the rest of the file [curse you, VC++ editor]

Comment 5

18 years ago
as I discussed with mscott, this patch is appropriate for the trunk, but is not
needed on the branch
(Assignee)

Comment 6

18 years ago
sr=mscott

Comment 7

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

Comment 8

18 years ago
...and marking fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 9

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

Updated

9 years ago
Severity: major → critical
Keywords: crash
Summary: Crash in GetReadableFragment [ @ nsAFlatCString::GetReadableFragment ] → Crash in GetReadableFragment [@ nsAFlatCString::GetReadableFragment ]
Crash Signature: [@ nsAFlatCString::GetReadableFragment ]
You need to log in before you can comment on or make changes to this bug.