Created attachment 480696 [details]
testcase (crashes Firefox when loaded)
Created attachment 480697 [details]
Scary: "Crash address: 0x29bdbf0"
Assigning to jwatt. Jonathan, can you take a look?
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?
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?)
> (Two mRefCnt members? How the hell?)
Harmless, see bug 602122.
Created attachment 481187 [details] [diff] [review]
Looks like this fixes bug 366130 too.
Comment on attachment 481187 [details] [diff] [review]
>+ // 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 220.127.116.11 and 18.104.22.168, a=dveditz for release-drivers
This landed in the wrong place on 1.9.2; on a really old relbranch (GECKO1924_20100413_RELBRANCH) instead of default.
The broken 1.9.2 landing noted in comment 9 was:
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
(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.
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:
(In reply to comment #0)
> Created attachment 480696 [details]
> testcase (crashes Firefox when loaded)
This might crash trunk but it doesn't crash 22.214.171.124 when loaded on OS X 10.6.5 in the release build. Was this crash seen with normal 1.9.2 builds?
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.
That points to the necessity of a debug build then (as opposed to an actual release build which is optimized).
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.