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

VERIFIED FIXED in mozilla1.9.1b3

Status

()

Core
Layout: R & A Pos
P3
critical
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: gkw, Assigned: dbaron)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla1.9.1b3
assertion, crash, testcase, verified1.9.0.9, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(4 attachments)

(Reporter)

Description

9 years ago
Created attachment 314716 [details]
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?

Updated

9 years ago
Blocks: 306663

Updated

9 years ago
Depends on: 398332

Comment 1

9 years ago
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-
(Reporter)

Comment 3

9 years ago
The testcase crashes Firefox 2.0.0.14 RC1 as well.

Updated

9 years ago
Summary: Crash [@ nsHTMLReflowState::GetNearestContainingBlock] → Crash [@ nsHTMLReflowState::GetNearestContainingBlock] with xul:listbox, position:absolute

Comment 4

9 years ago
See also bug 429881, which triggers some of the same assertions with a very different testcase.
Created attachment 322803 [details]
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
(Reporter)

Comment 7

9 years ago
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.
(Assignee)

Comment 9

9 years ago
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
(Assignee)

Comment 10

9 years ago
Created attachment 358643 [details] [diff] [review]
patch

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+
(Assignee)

Comment 12

9 years ago
(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.
(Assignee)

Comment 13

9 years ago
http://hg.mozilla.org/mozilla-central/rev/2a6b3ac2b49b
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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
(Assignee)

Updated

9 years ago
Attachment #358643 - Flags: approval1.9.1?
Attachment #358643 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Comment 14

9 years ago
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
(Assignee)

Updated

9 years ago
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
Keywords: fixed1.9.1 → verified1.9.1
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+
(Assignee)

Comment 17

9 years ago
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.
Keywords: fixed1.9.0.8 → verified1.9.0.8
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.next+

Comment 19

8 years ago
Created attachment 374810 [details] [diff] [review]
maybe for 1.8

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+

Comment 24

8 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/d7bcc00d3e5c
You need to log in before you can comment on or make changes to this bug.