431260 - Crash [@ gfxSkipCharsIterator::SetOffsets] with ::first-letter, position: absolute and setting innerHTML on root

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Martijn Wargers (dead)]

Attachment: testcase 1 (crashes firefox)

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

Comment 1

[Martijn Wargers (dead)]

Still crashes, using current trunk build.

Comment 2

[Daniel Holbert [:dholbert]]

Crashes Linux as well.  OS / Hardware --> All.

(assignee = me; taking a crack at this)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: produces assertions

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.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2a: produces assertions

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.

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Daniel Veditz [:dveditz]]

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.

Comment 8

[Daniel Holbert [:dholbert]]

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.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> it looks like it was already fixed before that

er, s/before that/independently of that/.

Comment 10

[Daniel Holbert [:dholbert]]

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

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 12

[Daniel Holbert [:dholbert]]

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.

Comment 13

[Daniel Veditz [:dveditz]]

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?

Comment 14

[Ria Klaassen (not reading all bugmail)]

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.

Comment 15

[Samuel Sidler (old account; do not CC)]

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

Comment 16

[Daniel Veditz [:dveditz]]

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

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 18

[Daniel Holbert [:dholbert]]

(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.

Comment 19

[Daniel Holbert [:dholbert]]

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

Comment 20

[Daniel Holbert [:dholbert]]

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.

Comment 21

[Daniel Holbert [:dholbert]]

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.

Comment 22

[Daniel Holbert [:dholbert]]

(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?

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: patch for bug 431341, backported to CVS trunk

(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.

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: patch for bug 455826, backported to CVS trunk [checked in]

(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)

Comment 25

[Daniel Holbert [:dholbert]]

(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?

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 28

[Daniel Holbert [:dholbert]]

(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?

Comment 29

[Daniel Holbert [:dholbert]]

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.

Comment 30

[Daniel Holbert [:dholbert]]

(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.)

Comment 31

[Daniel Holbert [:dholbert]]

Attachment: patch for bug 431341, backported to CVS trunk [checked in]

(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.)

Comment 32

[Daniel Holbert [:dholbert]]

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.

Comment 33

[Samuel Sidler (old account; do not CC)]

That sounds good to me.

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

Comment 34

[Tony Chung [:tchung]]

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!

Comment 35

[Daniel Holbert [:dholbert]]

(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 36

[Daniel Holbert [:dholbert]]

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 37

[Daniel Veditz [:dveditz]]

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 38

[Daniel Veditz [:dveditz]]

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 39

[Daniel Holbert [:dholbert]]

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 40

[Daniel Holbert [:dholbert]]

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.

Comment 41

[Daniel Holbert [:dholbert]]

(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.

Comment 42

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 44

[Marc Bejarano]

dholbert: are you going to check in tests?

Comment 45

[Daniel Holbert [:dholbert]]

Yeah, I will -- thanks for the reminder.

Comment 46

[Daniel Holbert [:dholbert]]

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.

Comment 47

[Daniel Holbert [:dholbert]]

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

Comment 48

[Daniel Holbert [:dholbert]]

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)"