Last Comment Bug 601699 - Crash [@ nsSVGPathElement::GetMarkPoints]
: Crash [@ nsSVGPathElement::GetMarkPoints]
Status: RESOLVED FIXED
[sg:critical?] [qa-needs-STR]
: crash, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Jonathan Watt [:jwatt]
:
Mentors:
Depends on:
Blocks: 326633
  Show dependency treegraph
 
Reported: 2010-10-04 12:32 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.13+
.13-fixed
.16+
.16-fixed


Attachments
testcase (crashes Firefox when loaded) (498 bytes, image/svg+xml)
2010-10-04 12:32 PDT, Jesse Ruderman
no flags Details
stack trace (3.90 KB, text/plain)
2010-10-04 12:33 PDT, Jesse Ruderman
no flags Details
patch (2.23 KB, patch)
2010-10-06 04:48 PDT, Jonathan Watt [:jwatt]
longsonr: review+
dveditz: approval1.9.2.13+
dveditz: approval1.9.1.16+
Details | Diff | Review

Description Jesse Ruderman 2010-10-04 12:32:08 PDT
Created attachment 480696 [details]
testcase (crashes Firefox when loaded)
Comment 1 Jesse Ruderman 2010-10-04 12:33:48 PDT
Created attachment 480697 [details]
stack trace

Scary: "Crash address: 0x29bdbf0"
Comment 2 Damon Sicore (:damons) 2010-10-05 13:53:30 PDT
Assigning to jwatt.  Jonathan, can you take a look?
Comment 3 Jonathan Watt [:jwatt] 2010-10-05 14:10:06 PDT
As it happens, when I was discussing another bug with jst earlier today, he noticed that we have two mRefCnt members for path segments. It may not be related, but it's highly suspicious.

Anyway, I'm completely rewriting this code in bug 522306, so it would make most sense to check if we still have this problem after I'm done with that. Does that sound okay?
Comment 4 Jesse Ruderman 2010-10-05 17:06:25 PDT
I think this also affects branches such as 1.9.2, so it would be nice to have a more direct fix.  I can't reproduce the crash at the moment but I do see an invalid array index assertion on both trunk and 1.9.2.

(Two mRefCnt members? How the hell?)
Comment 5 Jesse Ruderman 2010-10-05 19:16:13 PDT
> (Two mRefCnt members? How the hell?)

Harmless, see bug 602122.
Comment 6 Jonathan Watt [:jwatt] 2010-10-06 04:48:45 PDT
Created attachment 481187 [details] [diff] [review]
patch
Comment 7 Robert Longson 2010-10-06 05:17:00 PDT
Looks like this fixes bug 366130 too.
Comment 8 Daniel Veditz [:dveditz] 2010-10-08 10:23:28 PDT
Comment on attachment 481187 [details] [diff] [review]
patch

>+      // first non-moveto segment after a moveto
>       pathIndex = aMarks->Length() - 1;
>       pathAngle = startAngle;
>       aMarks->ElementAt(pathIndex).angle = pathAngle;

So the comment is saying that after a moveto you're guaranteed aMarks->Length() isn't 0? Otherwise I don't see why this doesn't have the same problem you're fixing a few lines down.

Approved for 1.9.2.12 and 1.9.1.15, a=dveditz for release-drivers
Comment 9 Ben Hearsum (:bhearsum) 2010-11-08 10:42:04 PST
This landed in the wrong place on 1.9.2; on a really old relbranch (GECKO1924_20100413_RELBRANCH) instead of default.
Comment 10 Daniel Holbert [:dholbert] 2010-11-08 10:49:10 PST
The broken 1.9.2 landing noted in comment 9 was:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6b4d4bdf28fe

Looks like jwatt landed it on 1.9.1 as well (correctly there -- not on a relbranch):  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b55da5097c00
Comment 11 Jonathan Watt [:jwatt] 2010-11-08 12:11:14 PST
(In reply to comment #8)
> So the comment is saying that after a moveto you're guaranteed aMarks->Length()
> isn't 0? Otherwise I don't see why this doesn't have the same problem you're
> fixing a few lines down.

The problem is that we were running the code |aMarks->ElementAt(aMarks->Length() - 1)| when the length of aMarks was zero, causing us to access memory at a bad location. That's what the new check for |aMarks->Length()| prevents (makes sure the length of the nsTArray is at least 1).

The comment, to answer your question, says that we know that we already have an item in aMarks, so there's no need for a |aMarks->Length()| check there.
Comment 12 Jonathan Watt [:jwatt] 2010-11-08 12:11:35 PST
1.9.1 commit:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b55da5097c00

1.9.2 commit:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1fbb00c824a9

On mozilla-central the code in question was substantially rewritten by the patch for bug 522306. That rewrite also fixed this bug, so on mozilla-central I just committed a test to make sure we don't regress:

http://hg.mozilla.org/mozilla-central/rev/981ba35eb1c3
Comment 13 Al Billings [:abillings] 2010-11-17 16:10:57 PST
(In reply to comment #0)
> Created attachment 480696 [details]
> testcase (crashes Firefox when loaded)

This might crash trunk but it doesn't crash 1.9.2.12 when loaded on OS X 10.6.5 in the release build. Was this crash seen with normal 1.9.2 builds?
Comment 14 Jonathan Watt [:jwatt] 2010-11-17 16:13:49 PST
It wasn't a very reliable crash on trunk, and apparently not on 1.9.2 either. It did reliably hit the assertion saying the index was bad though. See comment 4.
Comment 15 Al Billings [:abillings] 2010-11-17 16:38:22 PST
That points to the necessity of a debug build then (as opposed to an actual release build which is optimized).
Comment 16 Jonathan Watt [:jwatt] 2010-11-18 00:24:50 PST
To verify the fix? Yes, I guess so. But this bug is very much present in optimized builds too - the buggy logic and inevitability of going down the bad code path is the same in both.

Note You need to log in before you can comment on or make changes to this bug.