Closed
Bug 489477
Opened 16 years ago
Closed 15 years ago
Crash [@ nsFrameList::InsertFrame] with -moz-column, contenteditable
Categories
(Core :: Layout: Block and Inline, defect, P2)
Tracking
()
RESOLVED
FIXED
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)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Reporter | ||
Updated•16 years ago
|
Whiteboard: [sg:critical?]
Comment 2•15 years ago
|
||
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
Comment 3•15 years ago
|
||
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).
Comment 4•15 years ago
|
||
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.
Reporter | ||
Comment 6•15 years ago
|
||
CCing people who might be able to help with this editor security bug.
Comment 7•15 years ago
|
||
> ###!!! 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
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → roc
blocking2.0: --- → ?
Assignee | ||
Comment 9•15 years ago
|
||
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)
Assignee | ||
Comment 10•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
Comment 11•15 years ago
|
||
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+
Assignee | ||
Comment 12•15 years ago
|
||
Good points.
Whiteboard: [sg:critical?][needs review] → [sg:critical?][needs landing]
Assignee | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?][needs landing] → [sg:critical?]
Assignee | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
Comment 14•15 years ago
|
||
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+
Assignee | ||
Comment 15•15 years ago
|
||
Sure
Whiteboard: [sg:critical?][needs review] → [sg:critical?][needs landing]
Comment 16•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Attachment #431021 -
Flags: superreview?(bzbarsky)
Comment 18•15 years ago
|
||
Ah, right. /me tweaks whiteboard to reflect that.
Whiteboard: [sg:critical?][needs landing] → [sg:critical?][needs landing after getting sr]
Assignee | ||
Updated•15 years ago
|
blocking2.0: ? → final+
Priority: -- → P2
Updated•15 years ago
|
Whiteboard: [sg:critical?][needs landing after getting sr] → [sg:critical?][needs landing after getting sr][critsmash:patch]
Assignee | ||
Updated•15 years ago
|
Attachment #431021 -
Flags: superreview?(bzbarsky) → superreview?(matspal)
Comment 19•15 years ago
|
||
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+
Updated•15 years ago
|
Whiteboard: [sg:critical?][needs landing after getting sr][critsmash:patch] → [sg:critical?][needs landing][critsmash:patch]
Assignee | ||
Comment 20•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/90bb0545957a http://hg.mozilla.org/mozilla-central/rev/75d39842fbae
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs landing][critsmash:patch] → [sg:critical?][critsmash:patch]
Updated•15 years ago
|
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?][critsmash:resolved]
Comment 21•14 years ago
|
||
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);
Updated•13 years ago
|
Crash Signature: [@ nsFrameList::InsertFrame]
Comment 22•11 years ago
|
||
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.
Description
•