280 bytes, application/xhtml+xml
46.42 KB, text/plain
2.09 KB, patch
Mats Palmgren (vacation - back in August): review+
|Details | Diff | Splinter Review|
2.39 KB, patch
|Details | Diff | Splinter Review|
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?
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?
If this is a high top crash, please re-nom. Otherwise, won't hold the release for this.
The testcase crashes Firefox 126.96.36.199 RC1 as well.
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
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.
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?
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)
(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.
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
Comment on attachment 358643 [details] [diff] [review] patch Approved for 188.8.131.52, a=dveditz for release-drivers
Checked in to CVS trunk for 184.108.40.206, 2009-02-22 19:21 -0800.
Verified for 220.127.116.11 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:18.104.22.168pre) Gecko/2009031904 GranParadiso/3.0.8pre. Crashes 22.214.171.124 but renders fine without a crash with the 126.96.36.199 build.
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.
The build in the above comment was windows.
I do crash in a Mac 188.8.131.52 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 184.108.40.206 (but not in 1.9+ which needed this patch?), or is sensitive to some developer-opt vs. release build difference (dynamic vs static?).
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.
Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task
Landed the crashtest: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7bcc00d3e5c