Closed Bug 578977 Opened 11 years ago Closed 10 years ago
Crash [@ Is
Percentage Aware] with first-letter, position:fixed, etc
578 bytes, application/vnd.mozilla.xul+xml
1.35 KB, patch
|Details | Diff | Splinter Review|
133 bytes, text/html
8.20 KB, text/html
3.89 KB, patch
|Details | Diff | Splinter Review|
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
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?
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
I don't think this is needed though, because I have found the real bug....
Assignee: nobody → matspal
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.
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.
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.
Load the right test file...
+ parent->DeleteNextInFlowChild(mPresContext, kidNextInFlow, + !NS_INLINE_IS_BREAK_BEFORE(aReflowStatus)); Shouldn't we just be passing "true" for aDeletingEmptyFrames?
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
This is a reduced testcase based on layout/generic/crashtests/586806-1.htm
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.
Attachment #512155 - Attachment is obsolete: true
Don't mess with next-in-flows when the status is INLINE_IS_BREAK_BEFORE.
Attachment #546634 - Flags: review?(roc)
Comment on attachment 546634 [details] [diff] [review] fix v3 (wdiff) Review of attachment 546634 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #546634 - Flags: review?(roc) → review+
OS: Windows 7 → All
Hardware: x86 → All
Target Milestone: --- → mozilla8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.