Last Comment Bug 578977 - Crash [@ IsPercentageAware] with first-letter, position:fixed, etc
: Crash [@ IsPercentageAware] with first-letter, position:fixed, etc
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla8
Assigned To: Mats Palmgren (:mats)
:
:
Mentors:
Depends on: 698335
Blocks: 672080
  Show dependency treegraph
 
Reported: 2010-07-15 06:24 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2012-01-25 07:25 PST (History)
8 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wanted


Attachments
testcase (578 bytes, application/vnd.mozilla.xul+xml)
2010-07-15 06:24 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
wallpaper (1.35 KB, patch)
2011-01-27 18:50 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
fix v1 (1.44 KB, patch)
2011-01-27 19:03 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
fix v2 (4.23 KB, patch)
2011-02-08 10:28 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
fix v2 (4.23 KB, patch)
2011-02-08 10:32 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
reduced testcase for the assertion in comment 11 (133 bytes, text/html)
2011-02-14 06:48 PST, Mats Palmgren (:mats)
no flags Details
frame dump + stack for the reduced testcase (3.10 KB, text/html)
2011-02-14 06:49 PST, Mats Palmgren (:mats)
no flags Details
frame dump #2 (8.20 KB, text/html)
2011-07-18 14:07 PDT, Mats Palmgren (:mats)
no flags Details
fix v3 (wdiff) (3.89 KB, patch)
2011-07-18 14:13 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2010-07-15 06:24:10 PDT
Created attachment 457556 [details]
testcase

See testcase, which crashes current trunk build, after you've resized the browser window.

http://crash-stats.mozilla.com/report/index/815b27c2-2d1a-477b-86bf-805662100715
0  	xul.dll  	IsPercentageAware  	 layout/generic/nsLineLayout.cpp:671
1 	mozcrt19.dll 	arena_malloc_small 	obj-firefox/memory/jemalloc/crtsrc/jemalloc.c:3734
2 	mozcrt19.dll 	malloc 	obj-firefox/memory/jemalloc/crtsrc/jemalloc.c:5811
3 	xul.dll 	nsTArray_base::EnsureCapacity 	obj-firefox/xpcom/build/nsTArray.cpp:76
4 	xul.dll 	xul.dll@0xbcb30f 	
5 	xul.dll 	nsContinuingTextFrame::Init 	
6 	xul.dll 	xul.dll@0x86e8f 	
7 	xul.dll 	PL_DHashTableOperate 	obj-firefox/xpcom/build/pldhash.c:625
8 	xul.dll 	mozilla::FramePropertyTable::Remove 	layout/base/FramePropertyTable.cpp:148
9 	xul.dll 	mozilla::FrameProperties::Remove 	obj-firefox/dist/include/FramePropertyTable.h:230
10 	xul.dll 	nsContainerFrame::StealOverflowFrames 	layout/generic/nsContainerFrame.h:623
11 	xul.dll 	nsLineLayout::BeginSpan 	layout/generic/nsLineLayout.cpp:435
12 	xul.dll 	nsFirstLetterFrame::Reflow 	layout/generic/nsFirstLetterFrame.cpp:234
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2010-07-22 08:40:10 PDT
Oh, this is exciting.  The immediate cause of the crash is that we pass a null frame pointer to IsPercentageAware.  And that happens because the nsFirstLetterFrame has no textframe kid but nsFirstLetterFrame::Reflow assumes |kid| is non-null.

Looking at the frame tree, it looks like we have a block frame for the <td> which has a Letter frame containing the first 'm' on the first line.  That Letter frame has a continuation (containing " m"), and that has another continuation (containing " m").  This last also has an overflow list containing the single textframe with "m\u0639" in it, and a next-continuation which is a Letter frame with no kid which has _another_ next-continuation which is a Letter frame with no kid.

We're reflowing this last letter frame.

So it looks like we somehow failed to reflow the Letter frame that should have pulled stuff off the overflow list and destroyed that last empty continuation, right?
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-08-09 17:41:41 PDT
Yes.
Comment 3 Mats Palmgren (:mats) 2011-01-27 15:58:09 PST
There were 122 reported crash incidents for IsPercentageAware in the past
two week period.  I think we should wallpaper (with an assert) the case
where kid == NULL until we think we have fixed this.
bp-1eef6369-ac50-496a-97c4-278962110126
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-01-27 17:07:33 PST
OK
Comment 5 Mats Palmgren (:mats) 2011-01-27 18:50:29 PST
Created attachment 507762 [details] [diff] [review]
wallpaper

I don't think this is needed though, because I have found the real bug....
Comment 6 Mats Palmgren (:mats) 2011-01-27 19:03:48 PST
Created attachment 507764 [details] [diff] [review]
fix v1

After reflowing a first-letter child frame we have
NS_FRAME_IS_COMPLETE(aReflowStatus) and thus we delete all the kid's
next-in-flows.
http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/layout/generic/nsFirstLetterFrame.cpp#l257
This makes a bunch of |this| next-in-flows empty and nsLineLayout::ReflowFrame
only deletes the next-in-flows in the !NS_INLINE_IS_BREAK_BEFORE case
http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/layout/generic/nsLineLayout.cpp#l977
the else-clause just pushes the frame and leaves the empty first-letter
next-in-flows intact.
http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/layout/generic/nsLineLayout.cpp#l1063

Let me know if you want the wallpaper as well.  I don't think it should
be necessary.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-01-27 19:22:11 PST
Isn't the nsLineLayout code a bogus optimization? I think it should always delete the next-in-flows of the child even if NS_INLINE_IS_BREAK_BEFORE.
Comment 8 Mats Palmgren (:mats) 2011-02-08 10:28:55 PST
Created attachment 510662 [details] [diff] [review]
fix v2

The crashtest triggers a couple of:
###!!! ASSERTION: Computed overflow area must contain frame bounds: 'aNewSize.width == 0 || aNewSize.height == 0 || aOverflowAreas.Overflow(otype).Contains(nsRect(nsPoint(0,0), aNewSize))', file layout/generic/nsFrame.cpp, line 6144

I'll file that separately.  (TryServer results pending...)

I collected some statistics (loading Alexa top 500 sites).
~7% of nsLineLayout::ReflowFrame calls ends up in the else-block,
of those ~1.7% has a next-in-flow.  In other words, only 0.01% of
nsLineLayout::ReflowFrame calls are affected by the "optimization".

Fwiw, I did a quick hack marking frames where we delete next-in-flows
(in the else-block) and then counting them in
nsCSSFrameConstructor::CreateContinuingFrame.
About 96% did call CreateContinuingFrame.
Comment 9 Mats Palmgren (:mats) 2011-02-08 10:32:35 PST
Created attachment 510663 [details] [diff] [review]
fix v2

Load the right test file...
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-02-08 14:29:56 PST
+      parent->DeleteNextInFlowChild(mPresContext, kidNextInFlow,
+                                    !NS_INLINE_IS_BREAK_BEFORE(aReflowStatus));

Shouldn't we just be passing "true" for aDeletingEmptyFrames?
Comment 11 Mats Palmgren (:mats) 2011-02-14 06:46:22 PST
If I pass PR_TRUE then nsLayoutUtils::AssertTreeOnlyEmptyNextInFlows says
###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'start == end || IsInLetterFrame(aSubtreeRoot)', file layout/base/nsLayoutUtils.cpp, line 3964
when loading layout/generic/crashtests/586806-1.html
Comment 12 Mats Palmgren (:mats) 2011-02-14 06:48:03 PST
Created attachment 512154 [details]
reduced testcase for the assertion in comment 11

This is a reduced testcase based on layout/generic/crashtests/586806-1.htm
Comment 13 Mats Palmgren (:mats) 2011-02-14 06:49:44 PST
Created attachment 512155 [details]
frame dump + stack for the reduced testcase
Comment 14 Mats Palmgren (:mats) 2011-07-18 14:07:44 PDT
Created attachment 546633 [details]
frame dump #2

Here's a little more detail on what happens leading up the assertion
in nsLayoutUtils::AssertTreeOnlyEmptyNextInFlows.

Where reflowing an inline, 0x7fffd7c353e0 (lime).  It calls
ReflowInlineFrame on its text frame child 0x7fffd7c35450 (yellow)
and the resulting status is INLINE_IS_BREAK_BEFORE, here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsInlineFrame.cpp#564
so we break out of the loop and don't reflow any more child frames.
Returning to nsLineLayout::ReflowFrame we now have a reflow status
this is INLINE_IS_BREAK_BEFORE and FRAME_IS_COMPLETE.

The previous patch removed the next-in-flow (blue) for the inline (lime)
which caused the assertion, since text frame (red) maps content.
I think removing the next-in-flow in this case is wrong -- when the
reflow status is INLINE_IS_BREAK_BEFORE whether it's also COMPLETE
or INCOMPLETE is irrelevant since we haven't even tried to reflow
all the child frames yet.

I think the real bug is in nsFirstLetterFrame::Reflow (see comment 6)
which caused these first-letter frame continuations to be empty in the
first place.
Comment 15 Mats Palmgren (:mats) 2011-07-18 14:13:43 PDT
Created attachment 546634 [details] [diff] [review]
fix v3 (wdiff)

Don't mess with next-in-flows when the status is INLINE_IS_BREAK_BEFORE.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-18 19:07:51 PDT
Comment on attachment 546634 [details] [diff] [review]
fix v3 (wdiff)

Review of attachment 546634 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 19 :Ehsan Akhgari 2011-07-19 12:12:47 PDT
Also: http://hg.mozilla.org/mozilla-central/rev/101cf2df62d5

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