Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 17 years ago

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?

	Jesse Ruderman
	Comment 1 • 17 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?

	Damon Sicore (:damons)
	Comment 2 • 16 years ago

If this is a high top crash, please re-nom.  Otherwise, won't hold the release for this.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 3 • 16 years ago

The testcase crashes Firefox 2.0.0.14 RC1 as well.

	Jesse Ruderman
	Comment 4 • 16 years ago

See also bug 429881, which triggers some of the same assertions with a very different testcase.

	Carsten Book [:Tomcat]
	Comment 5 • 16 years ago

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.

	Martijn Wargers (dead)
	Comment 6 • 16 years ago

WFM, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008052907 Minefield/3.0pre

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 7 • 16 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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 16 years ago

I can't reproduce this on 64bit linux trunk/1.9.1,
but the crash is still there on OSX.

	David Baron :dbaron: Assignee
	Comment 9 • 16 years ago

For me it crashes on Mac and Windows but not on Linux.

	David Baron :dbaron: Assignee
	Comment 10 • 16 years ago

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?

	Mats Palmgren (inactive)
	Comment 11 • 16 years ago

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)

	David Baron :dbaron: Assignee
	Comment 12 • 16 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.

	David Baron :dbaron: Assignee
	Comment 13 • 16 years ago

http://hg.mozilla.org/mozilla-central/rev/2a6b3ac2b49b

	David Baron :dbaron: Assignee
	Comment 14 • 16 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c2fda16fd7bc

	Tony Chung [:tchung]
	Comment 15 • 16 years ago

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

	Daniel Veditz [:dveditz]
	Comment 16 • 16 years ago

Comment on attachment 358643 [details] [diff] [review]
patch

Approved for 1.9.0.8, a=dveditz for release-drivers

	David Baron :dbaron: Assignee
	Comment 17 • 16 years ago

Checked in to CVS trunk for 1.9.0.8, 2009-02-22 19:21 -0800.

	Al Billings [:abillings - ex-MoCo]
	Comment 18 • 16 years ago

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.

	Alexander Sack
	Comment 19 • 15 years ago

attaching unverified backport for 1.8.

Couldn't verify that it works as i don't have mac/win; anyone can check this patch?

	Daniel Veditz [:dveditz]
	Comment 20 • 15 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 21 • 15 years ago

The build in the above comment was windows.

	Daniel Veditz [:dveditz]
	Comment 22 • 15 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 23 • 15 years ago

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?).

	Alexander Sack
	Comment 24 • 15 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?

	Samuel Sidler (old account; do not CC)
	Comment 25 • 15 years ago

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.

	Joshua Mitchell (Inactive)
	Comment 26 • 10 years ago

Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task

	Mats Palmgren (inactive)
	Comment 27 • 10 years ago

Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7bcc00d3e5c

	Carsten Book [:Tomcat]
	Comment 28 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/d7bcc00d3e5c