Closed Bug 428113 Opened 12 years ago Closed 11 years ago

[Win & Mac only] Crash [@ nsHTMLReflowState::GetNearestContainingBlock] with xul:listbox, position:absolute

Categories

(Core :: Layout: Positioned, defect, P3, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: gkw, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [sg:critical?])

Crash Data

Attachments

(4 files)

Attached file testcase
The following testcase crashes Firefox with a null dereference @ nsHTMLReflowState::GetNearestContainingBlock.

Asserts first at 

###!!! ASSERTION: Placeholder relationship should have been torn down; see comments in nsPlaceholderFrame.h: '!shell->FrameManager()->GetPlaceholderFrameFor(mOutOfFlowFrame)', file /Users/skywalker/Desktop/Mozilla/cvs/mozilla/layout/generic/nsPlaceholderFrame.cpp, line 132

then at

###!!! ASSERTION: Dead placeholder in placeholder map: 'entry->placeholderFrame->GetOutOfFlowFrame() != (void*)0xdddddddd', file /Users/skywalker/Desktop/Mozilla/cvs/mozilla/layout/base/nsFrameManager.cpp, line 136

and finally at

###!!! ASSERTION: no placeholder frame: 'nsnull != placeholderFrame', file /Users/skywalker/Desktop/Mozilla/cvs/mozilla/layout/generic/nsHTMLReflowState.cpp, line 1082

Testcase is derived from crashtest in bug 354771.

Related to bug 398332?
Flags: blocking1.9?
Blocks: stirdom
Depends on: 398332
The second assertion indicates that there's a pointer to deleted memory floating around... is it just luck that Firefox crashes with a null deref before dereferencing a bogus address?
Whiteboard: [sg:critical?]
If this is a high top crash, please re-nom.  Otherwise, won't hold the release for this.
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9-
The testcase crashes Firefox 2.0.0.14 RC1 as well.
Summary: Crash [@ nsHTMLReflowState::GetNearestContainingBlock] → Crash [@ nsHTMLReflowState::GetNearestContainingBlock] with xul:listbox, position:absolute
See also bug 429881, which triggers some of the same assertions with a very different testcase.
Attached file stack
crash also for me after the landing of the patch from bug 398332 on Mac (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008052817 Firefox/3.0pre) and the testcase.
WFM, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008052907 Minefield/3.0pre
I still crash using nightly builds -  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008052804 Minefield/3.0pre

bp-44895dba-2e0b-11dd-a3db-001a4bd46e84
I can't reproduce this on 64bit linux trunk/1.9.1,
but the crash is still there on OSX.
For me it crashes on Mac and Windows but not on Linux.
Summary: Crash [@ nsHTMLReflowState::GetNearestContainingBlock] with xul:listbox, position:absolute → [Win & Mac only] Crash [@ nsHTMLReflowState::GetNearestContainingBlock] with xul:listbox, position:absolute
Attached patch patchSplinter Review
This is related to the only caller of RemoveMappingsForFrameSubtree, so I figured I'd handle it inside rather than outside, since it seems better to handle this inside the frame constructor.

The problem was that the list box body frame was calling RemoveMappingsForFrameSubtree on a placeholder, and we went on to call DeletingFrameSubtree on only the placeholder.  In the frame constructor itself, the callers of DeletingFrameSubtree tend to all manage placeholder issues themselves (although probably they shouldn't; they tend to all do it slightly differently, and DoDeletingFrameSubtree in theory already handles every case they deal with).

Anyway, here's a relatively straightforward patch that fixes the crash, although it probably has 10 things wrong with it or missing from it, given how complicated this code is.

Thoughts?
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #358643 - Flags: superreview?(roc)
Attachment #358643 - Flags: review?(mats.palmgren)
Attachment #358643 - Flags: superreview?(roc) → superreview+
Comment on attachment 358643 [details] [diff] [review]
patch

Is it useful to float/position a xul:listitem?  Why not add
  float: none !important;
  position: static !important;
like we did for html:option?

The layout doesn't look correct when using them anyway.

That said, the patch looks reasonable. This block:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1476&root=/cvsroot&mark=9604-9631#9603
is using UnregisterPlaceholderChain() instead which also unregisters
placeholder continuations -- but I think <listitem> can't have any,
given how nsListBoxBodyFrame is reflowed?
We should probably use that though in case we add new consumers of
RemoveMappingsForFrameSubtree in the future, r=mats with that.

(HG blame is insufferable so I used a CVS blame link instead -
that block of code is currently the same on trunk)
Attachment #358643 - Flags: review?(mats.palmgren) → review+
(In reply to comment #11)
> (From update of attachment 358643 [details] [diff] [review])
> Is it useful to float/position a xul:listitem?  Why not add
>   float: none !important;
>   position: static !important;
> like we did for html:option?

That doesn't seem like it would do any good unless we also ensured that nothing else could be the child of listbox.

> That said, the patch looks reasonable. This block:
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1476&root=/cvsroot&mark=9604-9631#9603
> is using UnregisterPlaceholderChain() instead which also unregisters
> placeholder continuations -- but I think <listitem> can't have any,
> given how nsListBoxBodyFrame is reflowed?
> We should probably use that though in case we add new consumers of
> RemoveMappingsForFrameSubtree in the future, r=mats with that.

I can't actually use UnregisterPlaceholderChain since I also want to delete the out-of-flows; other callers of UnregisterPlaceholderChain are using it when getting to the placeholder-oof relationship from the out-of-flow, rather than from the placeholder.

So, instead, I'll just write the loop into this function.
http://hg.mozilla.org/mozilla-central/rev/2a6b3ac2b49b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [sg:critical?] → [sg:critical?][needs 1.9.1 landing][needs 1.9.0.* landing]
Target Milestone: --- → mozilla1.9.2a1
Attachment #358643 - Flags: approval1.9.1?
Attachment #358643 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c2fda16fd7bc
Keywords: fixed1.9.1
Whiteboard: [sg:critical?][needs 1.9.1 landing][needs 1.9.0.* landing] → [sg:critical?][needs 1.9.0.* landing]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Attachment #358643 - Flags: approval1.9.0.8?
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090204 Minefield/3.2a1pre 
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre Ubiquity/0.1.5
Status: RESOLVED → VERIFIED
Comment on attachment 358643 [details] [diff] [review]
patch

Approved for 1.9.0.8, a=dveditz for release-drivers
Attachment #358643 - Flags: approval1.9.0.8? → approval1.9.0.8+
Checked in to CVS trunk for 1.9.0.8, 2009-02-22 19:21 -0800.
Keywords: fixed1.9.0.8
Whiteboard: [sg:critical?][needs 1.9.0.* landing] → [sg:critical?]
Verified for 1.9.0.8 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8pre) Gecko/2009031904 GranParadiso/3.0.8pre. Crashes 1.9.0.7 but renders fine without a crash with the 1.9.0.8 build.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next+
Attached patch maybe for 1.8Splinter Review
attaching unverified backport for 1.8.

Couldn't verify that it works as i don't have mac/win; anyone can check this patch?
On an up-to-date debug build of 1.8-branch Firefox I do not crash nor see the asserts with this testcase. I have not applied this patch.

Gary: do you still see Firefox 2.0.0.x crashes with this bug? Maybe your comment 3 was an unrelated crash that got fixed in some other bug.
Flags: blocking1.8.1.next+ → blocking1.8.1.next?
The build in the above comment was windows.
I do crash in a Mac 2.0.0.20 release build though. Guess I'll have to spin up an opt build to verify the patch.
I don't crash in a recent 1.8-branch opt build on windows. Either this is now mac-only, has been fixed by something else post 2.0.0.20 (but not in 1.9+ which needed this patch?), or is sensitive to some developer-opt vs. release build difference (dynamic vs static?).
Keywords: qawanted
Flags: blocking1.8.1.next? → blocking1.8.1.next+
so what can we do for 1.8? nobody seems to be able to reproduce. do we still want to consider the patch?
David might get time to look at this later, but not soon. The patch here (for 1.9.0) is already a bit scary and he'd want to spend time porting it over right. i.e., not for .22 but maybe .23.
Crash Signature: [@ nsHTMLReflowState::GetNearestContainingBlock]
Group: core-security
Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task
Keywords: qawanted
Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7bcc00d3e5c
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.