28.69 KB, text/plain
349 bytes, text/html
1.64 KB, patch
|Details | Diff | Splinter Review|
Created attachment 420240 [details] stack trace In debug Firefox trunk, loading any YouTube video page triggers: ###!!! ASSERTION: invalid array index: 'i < Length()', file ../../../dist/include/nsTArray.h, line 317 #5 0x041418ae in NS_DebugBreak_P (aSeverity=1, aStr=0x42f9140 "invalid array index", aExpr=0x42e7e60 "i < Length()", aFile=0x42f84dc "../../../dist/include/nsTArray.h", aLine=317) at /Users/jruderman/central/xpcom/base/nsDebugImpl.cpp:360 #6 0x0419b602 in nsTArray<gfxTextRun::GlyphRun>::ElementAt (this=0x213333d4, i=1) at nsTArray.h:317 #7 0x0419b636 in nsTArray<gfxTextRun::GlyphRun>::operator (this=0x213333d4, i=1) at nsTArray.h:350 #8 0x04197fb7 in gfxTextRun::SanitizeGlyphRuns (this=0x213333c0) at /Users/jruderman/central/gfx/thebes/src/gfxFont.cpp:2725 ...
Maybe Jonathan can look at this...
I'm not seeing this assertion with the youtube page. I suspect it was triggered by some particular (unusual) text string, perhaps in an ad or some transient listing of videos or comments. It'd be good to have a cut-down test case that reproduces it....
Ah - looks like I can reliably trigger this by loading the Unicode sample page at http://alanwood.net/unicode/unicode_samples.html. Should be able to narrow it down from there. (That page triggers several other assertions too....)
Created attachment 420323 [details] testcase, generates 4 assertions with current trunk build The assertion can be triggered by the presence of particular Unicode characters. This testcase has 4 characters (U+FEFF, FFF9, FFFA, FFFB) that each trigger it. I'm guessing the occurrence on youtube was probably temporary, caused by a character present in some dynamic content at that time.
Created attachment 420347 [details] [diff] [review] patch v1: update lastRunIndex variable when deleting a run The problem arises when SanitizeGlyphRuns deletes the last glyph run from the array, but fails to update the "last" variable; then on the next iteration it would try to look at the no-longer-valid "last" element of the array. Simple fix is to decrement "last" when deleting an array element. This patch does that, and the assertions no longer occur. I've also renamed "last" to "lastRunIndex" just because I think it's clearer. Incidentally, the problem only occurred with the Core Text backend, not ATSUI, because they generate different glyph arrays in the case of invisible characters such as those in the testcase here: Core Text deletes them (resulting in empty glyph runs, which SanitizeGlyphRuns is designed to eliminate), whereas ATSUI leaves a "deleted" placeholder glyph in the glyph array.
I think we should take this on 1.9.2 as well, as soon as it's landed on trunk. The chance of the bug actually being exploitable seems pretty slim, IMO, but I see no reason NOT to fix it; it's clear what the error is, and the patch is simple and safe.
Hmm, does my patch in bug 533393 fix this? I forgot to land it :-(
Oh, it's the same patch actually. Sorry about wasting your time.
Let's take your patch.
If this isn't a hard release blocker, which I don't think it is as per comment 6, then I don't think it would cause a RC respin on its own. If we have to do a new RC we'll take it as a ridealong, otherwise, 3.6.1.
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/8d23bc013df6
(In reply to comment #11) > If this isn't a hard release blocker, which I don't think it is as per comment > 6, then I don't think it would cause a RC respin on its own. If we have to do a > new RC we'll take it as a ridealong, otherwise, 3.6.1. Right - sorry, I didn't mean to suggest this needs to affect RC now, it's clearly not critical. Just that we should take it on the branch when we have the opportunity. Blocking was the wrong flag to request; how about approval188.8.131.52 on the patch?
Does this affect the 1.9.1 branch? It doesn't have the regression keyword so it might, and we don't want to land in a 3.6.x security update and not the corresponding 3.5.x security update if it affects both.
(In reply to comment #14) > Does this affect the 1.9.1 branch? It doesn't have the regression keyword so it > might, and we don't want to land in a 3.6.x security update and not the > corresponding 3.5.x security update if it affects both. No, it doesn't affect 1.9.1 because this bug is in code that was added specifically as part of the CoreText support, and we don't have CoreText support in 1.9.1 / 3.5.x.
Pushed to 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e6a4e7433b27
Verified for 1.9.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:184.108.40.206pre) Gecko/20100312 Namoroka/3.6.2pre using the attached testcase.
Crashtest added: http://hg.mozilla.org/mozilla-central/rev/d3e7c18f58fc