Closed Bug 489477 Opened 15 years ago Closed 14 years ago

Crash [@ nsFrameList::InsertFrame] with -moz-column, contenteditable

Categories

(Core :: Layout: Block and Inline, defect, P2)

Product:
Core
Component:
Layout: Block and Inline
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: roc)

References

Details

(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical?][critsmash:resolved])

Crash Data

Attachments

(3 files)

testcase (crashes Firefox when loaded)
761 bytes, text/html
Details
Part 1: fix "Must only be called on reflowed lines" assertion
1022 bytes, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Part 2: Fix nsOverflowContinuationTracker::Finish to notice when the overflow container list is going away
3.62 KB, patch
fantasai.bugs
: review+
MatsPalmgren_bugz
: superreview+
Details | Diff | Splinter Review 
###!!! ASSERTION: Must only be called on reflowed lines: '!(GetStateBits() & NS_FRAME_IS_DIRTY)', file /Users/jruderman/central/layout/generic/nsFrame.cpp, line 4331

###!!! ASSERTION: prev sibling has different parent: 'aNewFrame->GetParent() == aPrevSibling->GetParent()', file /Users/jruderman/central/layout/generic/nsFrameList.cpp, line 186

Crash [@ nsFrameList::InsertFrame] touching 0xddddddf9.
Whiteboard: [sg:critical?]
Still crashes on mozilla-central.
Still crashes on my mozilla-central (debug build, rev 88f7fdddc4ac).
I'm on Linux, so I'm setting Platform to "All"

Also: I see the first assertion-failure that Jesse mentioned ("Must only be called on reflowed lines"), but not the second.

GDB says I'm crashing at a call to nsIFrame::GetParent(), within nsFrameList::InsertFrames.  At this point, the "this" nsFrameList's mFirstChild and mLastChild pointers are both 0x5a5a5a5a.  So we're calling FirstChild()->GetParent() with FirstChild() == 0x5a5a5a5a, which crashes.

FWIW, this crashes my mozilla-central nightly opt build, too:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090928 Minefield/3.7a1pre
http://crash-stats.mozilla.com/report/pending/97ea0ec0-394b-4cf9-acab-64a912090928
OS: Mac OS X → All
This actually only recently started crashing on optimized builds.

* NO CRASH: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090915 Minefield/3.7a1pre
* CRASH: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20090916 Minefield/3.7a1pre
* PUSHLOG: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cdcf1519121f&tochange=38754465ffde

I'd bet this change was due to the patch from Bug 497495 (frame poisoning).
I also hit this crash in a 1.9.0.x debug build (from a recent CVS checkout), so the underlying problem here is probably quite old.
This may be the only remaining -moz-column crash on trunk.
CCing people who might be able to help with this editor security bug.
> ###!!! ASSERTION: Must only be called on reflowed lines

This is happening because we call nsBlockFrame::ShouldApplyTopMargin on an IsBlock() line that claims to not be mDirty but whose first (and only) child is a column-set which in fact has the dirty bit set.

Then we crash because we're in nsOverflowContinuationTracker::Insert and its mOverflowContList has first and last child pointers into lala-land.  Furthermore, its mPrevOverflowCont is pointing to a frame that's poisoned (so the pointer itself is ok, but the frame it's pointing to is not).

The trackers mParent is a blockframe for the <body>.

Seems like a block reflow bug to me, pure and simple.
Component: Editor → Layout: Block and Inline
QA Contact: editor → layout.block-and-inline
This has been added to the Top Security Bugs list.  We need an owner here.
Assignee: nobody → roc
blocking2.0: --- → ?
This part is pretty easy. In ReflowDirtyLines we already check if the block will be reflowed again for clearance, and if it will be, we mark the line dirty. We just need to also do it when we're pulling lines from the next-in-flow and reflowing them. Also, if the block will be reflowed again for clearance, we should stop pulling lines from the next-in-flow by breaking out of the loop. Unfortunately this patch alone doesn't fix the crash...
Attachment #431020 - Flags: review?(matspal)
The cause of the crash here is that a frame X is reflowed with status COMPLETE, we delete its next-in-flows, but the last next-in-flow is an overflow container, the only overflow container in its parent's list. The causes the mOverflowContList in the nsOverflowContinuationTracker for X's parent block to become a dangling pointer. While still reflowing X's parent, we add a new overflow container to the tracker, which uses the dangling pointer and disaster quickly follows.

Now, we did call nsOverflowContinuationTracker::Finish on X. However, mSentry is null because we skipped all the overflow container children --- aSkipOverflowContainerChildren is true when nsBlockFrame::Reflow constructs its tracker. So, nsOverflowContinuationTracker::Finish was effectively a no-op.

I wasn't quite sure what to do here. However, I think that the code "// Make sure we drop all references if the only frame in the overflow containers list is about to be destroyed" should be executed even if mSentry != f. Basically, how can it make sense to hold onto a dangling pointer in that situation? So this patch makes it not depend on mSentry == f. That fixes the bug.

I'm running reftests on the try-server now.
Attachment #431021 - Flags: review?(fantasai.bugs)
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
Comment on attachment 431020 [details] [diff] [review]
Part 1: fix "Must only be called on reflowed lines" assertion

(In reply to comment #9)
> Also, if the block will be reflowed again for
> clearance, we should stop pulling lines from the next-in-flow by breaking out
> of the loop.

Maybe I'm missing something but that doesn't seem to match what the code
does.  Your 'break' only ends the inner loop, not the outer loop that pulls
lines from the next-in-flow.  You need a 'keepGoing = PR_FALSE' to do that.

Assigning 'willReflowAgain' here is pointless, all code that uses it
is unreachable from this point.  Please remove it from the CheckForInterrupt
block as well (which should also have a 'keepGoing = PR_FALSE' ?).

r=mats with those issues addressed.
Attachment #431020 - Flags: review?(matspal) → review+
Good points.
Whiteboard: [sg:critical?][needs review] → [sg:critical?][needs landing]
Whiteboard: [sg:critical?][needs landing] → [sg:critical?]
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
fantasai says she'll review this tomorrow.
Comment on attachment 431021 [details] [diff] [review]
Part 2: Fix nsOverflowContinuationTracker::Finish to notice when the overflow container list is going away

Seems reasonable to me.

While you're at it, maybe add the missing "is" to
+     is an excessOverflowContainersProperty, or null, then this **is** our frame
?
Attachment #431021 - Flags: review?(fantasai.bugs) → review+
Sure
Whiteboard: [sg:critical?][needs review] → [sg:critical?][needs landing]
At jst's suggestion, I'll push this patch (with the tweak from comment 14) to m-c first thing tomorrow, since roc's mostly away for this week.
Actually you can't, because it doesn't have sr.
Attachment #431021 - Flags: superreview?(bzbarsky)
Ah, right. /me tweaks whiteboard to reflect that.
Whiteboard: [sg:critical?][needs landing] → [sg:critical?][needs landing after getting sr]
blocking2.0: ? → final+
Priority: -- → P2
Whiteboard: [sg:critical?][needs landing after getting sr] → [sg:critical?][needs landing after getting sr][critsmash:patch]
Attachment #431021 - Flags: superreview?(bzbarsky) → superreview?(matspal)
Comment on attachment 431021 [details] [diff] [review]
Part 2: Fix nsOverflowContinuationTracker::Finish to notice when the overflow container list is going away

sr=mats
Attachment #431021 - Flags: superreview?(matspal) → superreview+
Whiteboard: [sg:critical?][needs landing after getting sr][critsmash:patch] → [sg:critical?][needs landing][critsmash:patch]
http://hg.mozilla.org/mozilla-central/rev/90bb0545957a
http://hg.mozilla.org/mozilla-central/rev/75d39842fbae
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs landing][critsmash:patch] → [sg:critical?][critsmash:patch]
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?][critsmash:resolved]
I hit a bad result of the code in part 1 while working on bug 563584; it's fixed in my patch series there (in patch "set-incomplete-for-clearance-case").

In particular, this code can leave a frame with continuations that ought to stick around with an NS_FRAME_IS_COMPLETE reflow status, which causes us to delete those continuations (which we actually want to keep).

I'm fixing it by making the added block also do:
+            NS_FRAME_SET_INCOMPLETE(aState.mReflowStatus);
Crash Signature: [@ nsFrameList::InsertFrame]
Landed the crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb7950fb547
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: