Last Comment Bug 491547 - Crash [@ IsPercentageAware] with first-letter float and direction: rtl
: Crash [@ IsPercentageAware] with first-letter float and direction: rtl
[sg:dos] null deref; blocks further t...
: crash, regression, testcase, verified1.9.0.16, verified1.9.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Windows XP
-- critical (vote)
: mozilla1.9.2
Assigned To: Timothy Nikkel (:tnikkel)
: Jet Villegas (:jet)
Depends on:
Blocks: 332655 429865 436982 460389 477731 502560
  Show dependency treegraph
Reported: 2009-05-05 12:53 PDT by Martijn Wargers [:mwargers]
Modified: 2011-06-13 10:01 PDT (History)
13 users (show)
dveditz: wanted1.9.0.x+
tnikkel: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (465 bytes, application/vnd.mozilla.xul+xml)
2009-05-05 12:53 PDT, Martijn Wargers [:mwargers]
no flags Details
Wallpaper patch (903 bytes, patch)
2009-10-18 01:56 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
patch (2.28 KB, patch)
2009-10-26 23:15 PDT, Timothy Nikkel (:tnikkel)
roc: review+
smontagu: review+
roc: approval1.9.2+
dveditz: approval1.9.1.6+
dveditz: approval1.9.0.16+
Details | Diff | Splinter Review
testcase (500 bytes, text/html)
2009-10-28 18:03 PDT, Timothy Nikkel (:tnikkel)
no flags Details

Description User image Martijn Wargers [:mwargers] 2009-05-05 12:53:38 PDT
Created attachment 375849 [details]

See testcase, which crashes current trunk build after 100ms.
It doesn't crash in a 2009-05-01 build, it does crash in a 2009-05-02 build:
No idea what could have caused it.
0  	xul.dll  	IsPercentageAware  	 layout/generic/nsLineLayout.cpp:672
1 	xul.dll 	xul.dll@0x8fbb0b
Comment 1 User image Simon Montagu :smontagu 2009-05-05 18:44:55 PDT
My, which is the last changeset before that range, looks like a likely candidate. Can you check in about:buildconfig whether the build that doesn't crash includes that changeset?
Comment 2 User image Simon Montagu :smontagu 2009-05-06 03:01:14 PDT
Reverting did not remove the crash.

I get a series of assertions:

###!!! ASSERTION: prev sibling not in line list: 'Not Reached', file ...layout/generic/nsBlockFrame.cpp, line 4913

###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', file ...layout/generic/nsHTMLReflowState.cpp, line 518

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file ...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 ...layout/generic/nsTextFrameThebes.cpp, line 6107

###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', file ...layout/generic/nsHTMLReflowState.cpp, line 518

###!!! ASSERTION: shouldn't use unconstrained widths anymore: '(mFrameType == NS_CSS_FRAME_TYPE_INLINE && !frame->IsFrameOfType(nsIFrame::eReplaced)) || frame->GetType() == nsGkAtoms::textFrame || mComputedWidth != NS_UNCONSTRAINEDSIZE', file ...layout/generic/nsHTMLReflowState.cpp, line 287

###!!! ASSERTION: null frame is not allowed: 'aFrame', file ...layout/generic/nsLineLayout.cpp, line 670
Comment 3 User image Simon Montagu :smontagu 2009-05-06 04:40:37 PDT
I am seeing a different regression range: 2009-04-22-03 - 2009-04-23-03. Reverting the bidi-related checkin in that range, from bug 489517, prevents the crash with this testcase, but it still happens when changing direction manually, so this looks like an older bug that bug 489517 is exposing.
Comment 4 User image Martijn Wargers [:mwargers] 2009-05-06 04:49:02 PDT
Ok, I guess my regression range didn't make sense then.
Comment 5 User image Simon Montagu :smontagu 2009-05-06 05:12:49 PDT
I found the regression range for the older bug, and it turns out to be regression from bug 332655 (which is actually what I suspected in the first place).
Comment 6 User image timeless 2009-07-06 07:34:11 PDT
*** Bug 502560 has been marked as a duplicate of this bug. ***
Comment 7 User image Jesse Ruderman 2009-07-09 11:12:26 PDT
This bug interferes with fuzzing by triggering a bunch of assertions before crashing, sometimes including the dreaded "StealFrame failure" assertion.  As a result, I can't look for other bugs that trigger these assertions.
Comment 8 User image Carsten Book [:Tomcat] 2009-08-20 14:49:04 PDT
!exploitable output with the current 3.5.2 debug nightly 

(aec.9bc): Access violation - code c0000005 (!!! second chance !!!)
eax=00000000 ebx=7ffdb000 ecx=00000000 edx=00000026 esi=0012ee2c edi=0012ee34
eip=01ac6952 esp=0012e034 ebp=0012e058 iopl=0         nv up ei pl zr na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000246
01ac6952 8b11            mov     edx,dword ptr [ecx]  ds:0023:00000000=????????
0:000> cdb: Reading initial command '!load winext\msec.dll;.logappend;!exploitable;k;q'

Exploitability Classification: PROBABLY_EXPLOITABLE
Recommended Bug Title: Probably Exploitable - Data from Faulting Address controls Code Flow starting at gklayout!IsPercentageAware+0x0000000000000032 (Hash=0x1d0e112e.0x416e3159)

The data from the faulting address is later used as the target for a branch.
ChildEBP RetAddr
0012e058 01ac59c1 gklayout!IsPercentageAware+0x32
0012e1e0 01aba585 gklayout!nsLineLayout::ReflowFrame+0x81
0012e3c0 01ac5ceb gklayout!nsFirstLetterFrame::Reflow+0x235
0012e558 01a854fe gklayout!nsLineLayout::ReflowFrame+0x3ab
0012e600 01a84f10 gklayout!nsBlockFrame::ReflowInlineFrame+0x5e
0012e6a0 01a84ad2 gklayout!nsBlockFrame::DoReflowInlineFrames+0x210
0012e79c 01a82842 gklayout!nsBlockFrame::ReflowInlineFrames+0xf2
0012e89c 01a81491 gklayout!nsBlockFrame::ReflowLine+0x2c2
0012ea5c 01a7ed71 gklayout!nsBlockFrame::ReflowDirtyLines+0x561
0012ee2c 01a56ec2 gklayout!nsBlockFrame::Reflow+0x251
0012f02c 01a5627e gklayout!nsFrame::BoxReflow+0x522
0012f110 01a56860 gklayout!nsFrame::RefreshSizeCache+0x11e
0012f128 01bb3824 gklayout!nsFrame::GetBoxAscent+0x60
0012f158 01bc6972 gklayout!nsSprocketLayout::GetAscent+0x44
0012f170 01bb134b gklayout!nsBoxFrame::GetBoxAscent+0x72
0012f308 01bc6d55 gklayout!nsSprocketLayout::Layout+0xeb
0012f328 01bb499b gklayout!nsBoxFrame::DoLayout+0x55
0012f34c 01bb612d gklayout!nsIFrame::Layout+0x6b
0012f41c 01bc6d55 gklayout!nsStackLayout::Layout+0x17d
0012f43c 01bb499b gklayout!nsBoxFrame::DoLayout+0x55
Comment 9 User image chris hofmann 2009-08-20 17:29:17 PDT
I actually see this signature come in from users sometime.  yesterday was a big day.

IsPercentageAware Firefox	3.0.13	2009073022	1.9	Windows NT	5.1.2600 Service Pack 3	x86	

IsPercentageAware  Firefox	3.5.2	20090729225027	1.9.1	Windows NT	5.1.2600 Service Pack 2	x86	

IsPercentageAware Firefox	3.5.2	 Windows NT	5.1.2600 Szervizcsomag 

IsPercentageAware Firefox	3.5.2	

IsPercentageAware	about:blank

IsPercentageAware	about:blank



IsPercentageAware	about:blank
Comment 10 User image Daniel Veditz [:dveditz] 2009-09-12 09:47:56 PDT
Comment 8 shows a null deref DoS, consistent with the last assertion in comment 2. But this is blocking further fuzz testing of this area so that adds more importance to getting this fixed.
Comment 11 User image Simon Montagu :smontagu 2009-09-15 11:13:34 PDT
See bug 460389 comment 19. Note also that not all the crashes with this signature are this bug. IsPercentageAware is just the first place where nsLineLayout::ReflowFrame will crash when called with null aFrame.
Comment 12 User image Simon Montagu :smontagu 2009-10-18 01:56:18 PDT
Created attachment 406904 [details] [diff] [review]
Wallpaper patch

This prevents the crash, but leaves a bundle of assertions, so I'm not sure if it adds enough value to be worth pursuing.
Comment 13 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-10-18 15:58:06 PDT
First-letter frames definitely should have children. Is this just the general "first-letter and bidi reordering don't work together"? What are we going to do about this problem? I wonder if bidi-reordering could just ignore first-letter frames?
Comment 14 User image Timothy Nikkel (:tnikkel) 2009-10-18 16:21:28 PDT
When ltr the first letter frame should contain "&m&", but when rtl it should contain just "&" (correct me if I'm wrong, I don't know how bidi works, just basing this on how a static rendering works). When we dynamically change the direction that triggers a reflow, and when we ResolveBidi we create a non-fluid continuation of the first letter frame breaking between "&" and "m&". Leaving us with two first letter frames (NS_NewFirstLetterFrame is a good place to set a breakpoint to follow this). When reflowing that continuation the block frame creates a continuation (not totally sure why) of that first letter frame, and it stays empty and that is where we crash.

I don't think we should be creating continuations of first letter frames at all (?). So either we can teach Bidi resolution about first letter frames, or let the frame constructor deal with it by causing a reconstruct on containing blocks that have first letter style and "bidi stuff" (direction on the block changes, but I also imagine maybe changing some character data could do the same by mixing ltr and rtl characters?) changes on the block or its contents.
Comment 15 User image Timothy Nikkel (:tnikkel) 2009-10-26 23:15:17 PDT
Created attachment 408559 [details] [diff] [review]

Create an ugly special case in bidi resolution for floating first letter frames.

This also fixes bug 429865, bug 436982, bug 460389, and bug 477731.

I discovered two other bugs while fixing this. The overflow rect of the the child of a floating first letter frame doesn't get updated properly, likely due to the half-init'ed nsLineLayout that is used to reflow it. You can see this by converting the testcase to html. Then changing the direction will create a horizontal scrollbar to handle the overflow.

The other is that if you replace the "&m&m" in the testcase with a mixture of RTL and LTR characters, they won't get interleaved correctly when switching the direction from rtl to ltr and back.
Comment 16 User image Timothy Nikkel (:tnikkel) 2009-10-26 23:16:38 PDT
Comment on attachment 408559 [details] [diff] [review]

Not sure who needs/wants to look at this. Please adjust as necessary.
Comment 17 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-10-26 23:58:26 PDT
Comment on attachment 408559 [details] [diff] [review]

Looks good to me! Simon should definitely look at it too. Thanks!!!
Comment 18 User image Simon Montagu :smontagu 2009-10-28 14:21:48 PDT
(In reply to comment #15)

> The other is that if you replace the "&m&m" in the testcase with a mixture of
> RTL and LTR characters, they won't get interleaved correctly when switching the
> direction from rtl to ltr and back.

That sounds bad. What does the frame tree look like in this case?
Comment 19 User image Timothy Nikkel (:tnikkel) 2009-10-28 18:03:43 PDT
Created attachment 409000 [details]

A quick look at the frame tree looks okay. But here is a testcase so you can have a look. After some investigation changing the direction dynamically wasn't needed. I think the "bc" get incorrectly placed with the "ef".
Comment 20 User image Simon Montagu :smontagu 2009-10-28 22:08:03 PDT
That looks as if the embedding levels of the frames aren't getting set properly.
Comment 21 User image Timothy Nikkel (:tnikkel) 2009-10-31 19:49:20 PDT
Comment 22 User image Timothy Nikkel (:tnikkel) 2009-10-31 20:00:21 PDT
All branches are affected.
Comment 23 User image Simon Montagu :smontagu 2009-11-01 05:08:43 PST
I have a fix for attachment 409000 [details]: the embedding levels are set properly, but nsBidiPresUtils::GetFrameEmbeddingLevel is checking the embedding level of the placeholder instead of the placeholdee.
Comment 24 User image Simon Montagu :smontagu 2009-11-01 12:21:53 PST
Opened bug 525740 for the reordering bug.
Comment 25 User image Daniel Veditz [:dveditz] 2009-11-02 14:23:57 PST
Not blocking, but we'll look at the patch when we get to approvals.
Comment 27 User image Daniel Veditz [:dveditz] 2009-11-04 15:25:54 PST
Comment on attachment 408559 [details] [diff] [review]

Approved for and, a=dveditz for release-drivers
Comment 28 User image Timothy Nikkel (:tnikkel) 2009-11-04 15:40:50 PST
I might be more conservative about making this bug public, although on 1.9.1+ we crash with a null deref, we are just getting lucky, the frame tree is messed up. On 1.9.0 there's even more room for things to go wrong as you have to flip the direction back and forth a few times before it crashes, so there is plenty of time to mess with a broken frame tree.
Comment 29 User image Timothy Nikkel (:tnikkel) 2009-11-05 01:17:33 PST
I forgot to paste this above: a push for a bustage fix
Comment 31 User image Boris Zbarsky [:bz] (still a bit busy) 2009-11-09 09:04:52 PST
Checking in nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.117; previous revision: 1.116

Checking in layout/base/crashtests/503936-1.html;
/cvsroot/mozilla/layout/base/crashtests/503936-1.html,v  <--  503936-1.html
initial revision: 1.1
Checking in layout/base/crashtests/crashtests.list;
/cvsroot/mozilla/layout/base/crashtests/crashtests.list,v  <--  crashtests.list
new revision: 1.103; previous revision: 1.102
Comment 32 User image Boris Zbarsky [:bz] (still a bit busy) 2009-11-09 09:14:18 PST
Er, that second part should have been:

Checking in layout/base/crashtests/429865-1.html;
/cvsroot/mozilla/layout/base/crashtests/429865-1.html,v  <--  429865-1.html
initial revision: 1.1
Checking in layout/base/crashtests/436982-1.html;
/cvsroot/mozilla/layout/base/crashtests/436982-1.html,v  <--  436982-1.html
initial revision: 1.1
Checking in layout/base/crashtests/477731-1.html;
/cvsroot/mozilla/layout/base/crashtests/477731-1.html,v  <--  477731-1.html
initial revision: 1.1
Checking in layout/base/crashtests/crashtests.list;
/cvsroot/mozilla/layout/base/crashtests/crashtests.list,v  <--  crashtests.list
new revision: 1.104; previous revision: 1.103
Comment 33 User image Al Billings [:abillings] 2009-11-23 10:59:32 PST
Verified crash with attached testcase in and lack of crash with (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729)) so this is fixed for 1.9.1.

On 1.9.0, the testcase does not cause a crash in and I see no difference in behavior between and .16. Does this really affect 1.9.0 directly or is this just defense in depth across all of the code lines?
Comment 34 User image Timothy Nikkel (:tnikkel) 2009-11-23 14:07:23 PST
I have a testcase that crashes 1.9.0 but I would prefer not to post it in a public bug. I checked that 1.9.0 nightlies no longer crash on that testcase.
Comment 35 User image Al Billings [:abillings] 2009-11-23 14:38:04 PST
That's good enough for me, Timothy. I'm marking this verified for
Comment 36 User image Samuel Sidler (old account; do not CC) 2009-11-23 20:16:59 PST
Timothy, please be sure to attach it to this bug after we ship a release with this fix in it. (Alternatively, send it to someone in the security-group who can attach it as a private attachment.)
Comment 37 User image Timothy Nikkel (:tnikkel) 2010-02-03 22:18:32 PST
Landed the testcase of this bug and the testcase mentioned in comment 34 on mozilla-central

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