Marking security sensitive, just in case. See testcase, which crashes current trunk build, within 100ms or a little later. This seems to have regressed between 2007-09-16 and 2007-09-18: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-09-16+22&maxdate=2007-09-18+05&cvsroot=%2Fcvsroot Regression from bug 385607 or from bug 393796? http://crash-stats.mozilla.com/report/index/20407c45-1586-11dd-8e99-001321b13766?p=1 0 xul.dll gfxSkipCharsIterator::SetOffsets mozilla/gfx/thebes/src/gfxSkipChars.cpp:129 1 xul.dll gfxSkipCharsIterator::SetOriginalOffset gfxSkipChars.h:265 2 xul.dll xul.dll@0x2aeae9 3 xul.dll nsLineLayout::ReflowFrame mozilla/layout/generic/nsLineLayout.cpp:859 4 xul.dll nsFirstLetterFrame::Reflow mozilla/layout/generic/nsFirstLetterFrame.cpp:240

Still crashes, using current trunk build.

Crashes Linux as well. OS / Hardware --> All. (assignee = me; taking a crack at this)

This reduced version of the original testcase produces these 2 assertions on my Linux box: ###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /scratch/work/builds/central/central.08-10-01.16-24/mozilla/gfx/thebes/src/gfxSkipChars.cpp, line 92 ###!!! ASSERTION: Text run does not map enough text for our reflow: 'gfxSkipCharsIterator(iter).ConvertOriginalToSkipped(offset + length) <= mTextRun->GetLength()', file /scratch/work/builds/central/central.08-10-01.16-24/mozilla/layout/generic/nsTextFrameThebes.cpp, line 5768 (Note: It only asserts when the content doesn't all fit on one line. If you can't reproduce the assertions, and you see all the contents on one line, try reducing your browser's width, to make some text wrap.

Here's a slightly clearer version of testcase 2. This produces the assertions mentioned in my previous comment, and it renders like this on my debug build: a b [arabic_char] c c c c c c c c c c c c c c c c c c c c c c c c c c c c c c c Also interesting: If I make the browser slightly wider, so that everything just barely fits on one line, I get this warning: WARNING: We shouldn't be backing up more than once! Someone must have set a break opportunity beyond the available width, even though there were better break opportunities before it: file /mozilla/layout/generic/nsBlockFrame.cpp, line 3451 So, it looks like the Arabic character could be messing with our determination of break opportunities.

FWIW, bug 455826 triggers the same assertions listed here in Comment 3 -- see bug 455826 comment 9.

Regression range for testcase 1's crash is between these nightlies: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007091704 Minefield/3.0a8pre Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007091804 Minefield/3.0a8pre Bonsai link: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Flayout+mozilla%2Fgfx&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-09-17+03%3A00%3A00&maxdate=2007-09-18+05%3A00%3A00&cvsroot=%2Fcvsroot Looks like a regression from bug 385607 or bug 393796.

The actual crash seems to be an out of bounds read (low-moderate) but "this" may be a freed object since some of its properties are 0xdddddddd in a Windows debug build.

This is now WFM using my debug mozilla-central build -- no crashes & no asserts. I initially thought this was fixed by bug 455826 (based on comment 5 here), but it looks like it was already fixed before that. (I tried backing out that bug's checkin, changeset df41ce61d237, and I still get no crashes/asserts.) So, I'm marking this as WORKSFORME. The only remaining (minor) issue here is that we now spam a warning about "Scanning overflow inline frames is something we should avoid". But I've filed bug 471202 on that issue.

(In reply to comment #8) > it looks like it was already fixed before that er, s/before that/independently of that/.

This is still broken on the 1.9 branch -- I just tried loading the first testcase with this build... Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.6pre) Gecko/2008122604 GranParadiso/3.0.6pre ...and it crashed with crash ID 65fd6fef-1196-4e98-97fb-b0a422081226 Given the wanted1.9.0.x status on this bug, we should track down the fix range and try to get the fix backported to 1.9

Is it fixed on 1.9.1? If so, probably best to add the fixed1.9.1 keyword given that this is blocking1.9.1+ and we don't have a worksforme1.9.1 keyword.

Yeah -- no crash on latest-mozilla-1.9.1 build: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20081226 Shiretoko/3.1b3pre Adding fixed1.9.1 keyword.

We'd like whatever fixed this in 1.9.1 to land on 1.9.0.x. Ria: Sam tells me you're good at finding fix ranges, want to take a crack at this one?

It must have happened on 26 Nov 2008 in any case. http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-11-26+02%3A00%3A00&enddate=2008-11-26+23%3A00%3A00 Bug 463292 was checked in twice on this day.

roc: You owned bug 463292. What say you on getting it ported to 1.9.0?

Thanks Ria! I doubt it's bug 463292, but others in that same group from roc look promising. Looking for the files from the assertions, nsTextFrameThebes.cpp is touched by bug 459968, bug 455826, and bug 430332 Negative heights were prevented in bug 462968, and nsBlockFrame.cpp was mentioned in the warning in comment 4

The most efficient way to find out which change fixed it, even for me, would probably just be to pull and build changesets and test them.

(In reply to comment #16) > Thanks Ria! > > I doubt it's bug 463292, but others in that same group from roc look promising. (In reply to comment #17) > The most efficient way to find out which change fixed it, even for me, would > probably just be to pull and build changesets and test them. I'll do that to figure out what fixed this, so we can fix this crash on the branches.

This crash was fixed on mozilla-central by Bug 455826's checkin, http://hg.mozilla.org/mozilla-central/rev/df41ce61d237

Hrm, I guess comment 19 contradicts comment 8 -- maybe I didn't back out correctly in comment 8, or maybe another patch inadvertently worked around this crash later on. In any case, comment 19 is correct -- I get a crash when building with changeset db3ad9fd7b65, and no crash when building with changeset df41ce61d237, so Bug 455826 fixed this.

However, Bug 455826's patch doesn't apply on 1.9.0.x branch. :( It modifies a bunch of stuff related to the "FrameTextTraversal" struct, which is newly added since the 1.9.0.x branch (added in bug 431341) So, it looks like a simple direct backport isn't going to work.

(In reply to comment #21) > However, Bug 455826's patch doesn't apply on 1.9.0.x branch. :( It modifies a > bunch of stuff related to the "FrameTextTraversal" struct, which is newly added > since the 1.9.0.x branch (added in bug 431341) However, *bug 431341's* patch actually *does* apply on CVS Trunk fairly cleanly (it just requires hand-merging in one chunk). After that, bug 455826's patch applies cleanly (except for "reftest.list" bitrot) and fixes this crash. So, I guess the question is -- is it appropriate to land both bug 431341 and bug 455826 on 1.9.0.x, to fix this crash? Or is it better to hack up a more targeted workaround specific to this bug?

(In reply to comment #22) > However, *bug 431341's* patch actually *does* apply on CVS Trunk fairly cleanly > (it just requires hand-merging in one chunk). FWIW, here's the cleaned-up CVS-Trunk-Specific version of that patch.

(And here's the patch for bug 455826, with the reftest.list merge conflicts fixed for CVS trunk. Again, this patch plus the previous one fixes this bug's crash)

(In reply to comment #22) > So, I guess the question is -- is it appropriate to land both bug 431341 and > bug 455826 on 1.9.0.x, to fix this crash? Or is it better to hack up a more > targeted workaround specific to this bug? roc, you're the original author of both patches -- do you think they're appropriate to land on the 1.9.0.x branch?

If we do anything to fix this bug, we should just take 431341 and 455826 since that's less work and those patches have baked long on trunk so it's probably less risk too.

I'm not sure how important it is to fix this bug on branch, really, so it's a tough call. I wouldn't object to the patches landing.

(In reply to comment #26) > If we do anything to fix this bug, we should just take 431341 and 455826 since > that's less work and those patches have baked long on trunk Yeah -- and neither of them caused any regressions on mozilla-central, which is nice. (In reply to comment #27) > I'm not sure how important it is to fix this bug on branch, really Comment 7 shows that we may be calling methods with "this" == a freed object, which is why this was marked [sg:critical?]. I'm going to double-check that in my linux debug build, but if that's what we're doing, I think that falls into the category of things we really don't want to leave unresolved, right?

So at first glance, I don't see any 0xdddddddd pointers at crash-time, but I do get the out-of-bounds read, and some oddly large numbers. I crash on this line: http://mxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxSkipChars.cpp#129 129 PRInt32 currentRunLength = mSkipChars->mList[mListPrefixLength]; Here, mListPrefixLength is 1598378073, and "this" (a gfxSkipChars object) is: (gdb) p *this $23 = {mSkipChars = 0xac1a9030, mOriginalStringOffset = 0, mSkippedStringOffset = 0, mOriginalStringToSkipCharsOffset = 4294966973, mListPrefixLength = 1598378073, mListPrefixCharCount = 1280266068, mListPrefixKeepCharCount = 5261652} I'm guessing that those large numbers might originate from a negative-number-to-unsigned conversion in nsTextFrame::EnsureTextRun, here: 1821 gfxSkipCharsIterator iter(mTextRun->GetSkipChars(), 1822 flow->mDOMOffsetToBeforeTransformOffset, mContentOffset); At that point, "flow->mDOMOffsetToBeforeTransformOffset" is -2, which gets used to initialize the _unsigned_ value iter->mOriginalStringToSkipCharsOffset as 4294967294 == 2^32 - 2. ... though maybe that's just one of the places where we legitimately use unsigned values to represent signed values (including negatives)? I'm not sure. In any case -- I'm not seeing any methods called on deleted objects in Linux.

(In reply to comment #27) > I'm not sure how important it is to fix this bug on branch, really, so it's a > tough call. I wouldn't object to the patches landing. dveditz, what's your feel on this bug's importance for 1.9.0.x? (If you think it's still blocking, I'll request approval on & subsequently land the attached patches.)

(I just noticed that the patch I backported for bug 431341 didn't include the reftest files 431341-1.html and 431341-1-ref.html. They're in an earlier version of the patch on that bug, and they were landed on mozilla-central in an earlier changeset. I've now added them in this version of the patch.)

Can we postpone this to the next 1.9.0.x release? The code freeze for 1.9.0.7 is in a few hours, and I feel like it'd be a bit rushed to make a decision and then (possibly) land this right now. If we decide to take this on branch, I'd prefer to land it early on in the 1.9.0.8 cycle, giving it a little time to bake in the nightlies there.

That sounds good to me. Daniel: Thanks for doing all the hard work on this. We'll make a call a little bit later. :)

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090204 Minefield/3.2a1pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre Ubiquity/0.1.5 Please include the link to the changeset to 1.9.1 next time. Thanks!

(In reply to comment #34) > Please include the link to the changeset to 1.9.1 next time. Thanks! Comment 19 includes the bug # and changeset that fixed this, which incidentally was _before_ the 1.9.1 branch point. (So, it's the same changeset on both m-c and 1.9.1.) Here's the 1.9.1 version of the changeset, though, if it's useful: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/df41ce61d237

Comment on attachment 360419 [details] [diff] [review] patch for bug 431341, backported to CVS trunk [checked in] I'm requesting approval to land the backported crash-fixing patches on 1.9.0.8. (Given that this was marked as blocking1.9.0.8+, I'm guessing that the answer to comment 30 is "it's important to fix this on 1.9.0.x", in which case these are the patches that should land.)

Comment on attachment 360419 [details] [diff] [review] patch for bug 431341, backported to CVS trunk [checked in] Approved for 1.9.0.8, a=dveditz for release-drivers

Comment on attachment 360386 [details] [diff] [review] patch for bug 455826, backported to CVS trunk [checked in] Approved for 1.9.0.8, a=dveditz for release-drivers

Comment on attachment 360419 [details] [diff] [review] patch for bug 431341, backported to CVS trunk [checked in] patch for bug 431341 is now checked into CVS, as noted in bug 431341 comment 21

Comment on attachment 360386 [details] [diff] [review] patch for bug 455826, backported to CVS trunk [checked in] patch for bug 455826 is now checked into CVS, as noted in bug 431341 comment 21. This bug should now be fixed on the 1.9.0.x branch.

(In reply to comment #40) > (From update of attachment 360386 [details] [diff] [review]) > patch for bug 455826 is now checked into CVS, as noted in bug 431341 comment Sorry -- I meant "as noted in bug 455826 comment 19". I'm ticking "in-testsuite?" flag, as a reminder to check in a crashtest for this once it's safe to open this bug up.

Verified fix with first testcase in 1.9.0.8 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8pre) Gecko/2009031904 GranParadiso/3.0.8pre. 1.9.0.7 crashes cleanly.

We have patches, so it's fixed and not worksforme.

dholbert: are you going to check in tests?

Yeah, I will -- thanks for the reminder.

Checked in testcase 1 as a crashtest and testcase 2a as an assertion test (both modified to use reftest-wait) to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/27ebc214a570 I verified that my modified-testcase-1 still crashes on an old mozilla-central build. I don't have an old debug build handy, so I can't immediately verify that the modified testcase 2a still produces assertions in pre-patched code, but I expect that it should. I'll add the tests to the 1.9.1 and 1.9.0 branches after the 1.9.1 tree reopens.

Crashtests checked into 1.9.1 and 1.9.0.x: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f63ef3d31bdb RCS file: /cvsroot/mozilla/layout/generic/crashtests/431260-1.html,v done Checking in 431260-1.html; /cvsroot/mozilla/layout/generic/crashtests/431260-1.html,v <-- 431260-1.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/generic/crashtests/431260-2.html,v done Checking in 431260-2.html; /cvsroot/mozilla/layout/generic/crashtests/431260-2.html,v <-- 431260-2.html initial revision: 1.1 done Checking in crashtests.list; /cvsroot/mozilla/layout/generic/crashtests/crashtests.list,v <-- crashtests.list new revision: 1.123; previous revision: 1.122 done

It looks like this bug's crashtest 431260-1.html timed out on its second 1.9.0.x linux tinderbox run, though there was another known-sporadic timeout before it (bug 483508 for 421671.html) Linux fx-linux-1.9-slave09 dep unit test on 2009/04/30 11:07:43 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.0/1241114863.1241118385.32620.gz REFTEST UNEXPECTED FAIL (LOADING): file:///builds/slave/trunk_centos5_9/mozilla/layout/generic/crashtests/421671.html REFTEST UNEXPECTED FAIL (LOADING): file:///builds/slave/trunk_centos5_9/mozilla/layout/generic/crashtests/431260-1.html If this happens frequently, we can probably remedy it by reducing the number of iterations used -- i.e. change "setTimeout(innerhtml,0,30)" to "setTimeout(innerhtml,0,10)"