Closed Bug 398982 Opened 17 years ago Closed 17 years ago

Crash [@ DoDeletingFrameSubtree] with menuitem, tooltip, treecols and removing position: absolute

Categories

(Core :: Layout, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [dbaron-1.9:Rs])

Crash Data

Attachments

(7 files, 1 obsolete file)

Attached file testcase (deleted) —
See testcase, which crashes current trunk build after 200ms. This regressed between 2007-10-05 and 2007-10-07: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-05+04&maxdate=2007-10-07+09&cvsroot=%2Fcvsroot Somehow a regression from bug 372685? http://crash-stats.mozilla.com/report/index/4c3f3d97-7591-11dc-94e2-001a4bd43e5c 0 DoDeletingFrameSubtree mozilla/layout/base/nsCSSFrameConstructor.cpp:9285 1 DoDeletingFrameSubtree mozilla/layout/base/nsCSSFrameConstructor.cpp:9291 2 DoDeletingFrameSubtree mozilla/layout/base/nsCSSFrameConstructor.cpp:9291 3 DeletingFrameSubtree mozilla/layout/base/nsCSSFrameConstructor.cpp:9339 4 nsCSSFrameConstructor::RemoveMappingsForFrameSubtree(nsIFrame*) mozilla/layout/base/nsCSSFrameConstructor.cpp:9378 5 nsPopupSetFrame::DestroyPopupFrame(nsCSSFrameConstructor*, nsIFrame*) mozilla/layout/xul/base/src/nsPopupSetFrame.cpp:303 6 nsPopupSetFrame::Destroy() mozilla/layout/xul/base/src/nsPopupSetFrame.cpp:147 7 nsFrameList::DestroyFrames() mozilla/layout/generic/nsFrameList.cpp:67 8 nsContainerFrame::Destroy() mozilla/layout/generic/nsContainerFrame.cpp:259 9 nsBoxFrame::RemoveFrame(nsIAtom*, nsIFrame*) mozilla/layout/xul/base/src/nsBoxFrame.cpp:1012 10 nsRootBoxFrame::RemoveFrame(nsIAtom*, nsIFrame*) mozilla/layout/xul/base/src/nsRootBoxFrame.cpp:212 11 nsFrameManager::RemoveFrame(nsIFrame*, nsIAtom*, nsIFrame*) mozilla/layout/base/nsFrameManager.cpp:690 12 nsCSSFrameConstructor::ReconstructDocElementHierarchyInternal() mozilla/layout/base/nsCSSFrameConstructor.cpp:7740 13 nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*) mozilla/layout/base/nsCSSFrameConstructor.cpp:11120 14 nsCSSFrameConstructor::RestyleElement(nsIContent*, nsIFrame*, nsChangeHint) mozilla/layout/base/nsCSSFrameConstructor.cpp:9973 15 nsCSSFrameConstructor::ProcessOneRestyle(nsIContent*, nsReStyleHint, nsChangeHint) mozilla/layout/base/nsCSSFrameConstructor.cpp:13001 16 nsCSSFrameConstructor::ProcessPendingRestyles() mozilla/layout/base/nsCSSFrameConstructor.cpp:13054 17 PresShell::DoFlushPendingNotifications(mozFlushType, int) mozilla/layout/base/nsPresShell.cpp:4443 18 PresShell::FlushPendingNotifications(mozFlushType) mozilla/layout/base/nsPresShell.cpp:4407 19 nsCSSFrameConstructor::RestyleEvent::Run() mozilla/layout/base/nsCSSFrameConstructor.cpp:13110 20 nsThread::ProcessNextEvent(int, int*) mozilla/xpcom/threads/nsThread.cpp:490 21 NS_ProcessNextEvent_P(nsIThread*, int) nsThreadUtils.cpp:227 22 nsBaseAppShell::Run() mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:154 23 nsAppStartup::Run() mozilla/toolkit/components/startup/src/nsAppStartup.cpp:170 24 XRE_main mozilla/toolkit/xre/nsAppRunner.cpp:3142 25 main mozilla/browser/app/nsBrowserApp.cpp:153 26 WinMain mozilla/browser/app/nsBrowserApp.cpp:166 27 __tmainCRTStartup crtexe.c:589 28 BaseProcessStart
Assignee: nobody → mats.palmgren
Blocks: 372685
Flags: blocking1.9?
OS: Windows XP → All
Hardware: PC → All
Attached file trace + frame dump (deleted) —
ReconstructDocElementHierarchyInternal() clears the frame manager maps before destroying the frame tree: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1408&root=/cvsroot&mark=7724-7728,7739#7691 ClearPlaceholderFrameMap() removes all the placeholder/out-of-flow mappings so DoDeletingFrameSubtree() will eventually crash on a null out-of-flow: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1408&root=/cvsroot&mark=9284#9247 RemoveMappingsForFrameSubtree() has a check to prevent this: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1408&root=/cvsroot&mark=9367#9364 but |mIsDestroyingFrameTree| is false in this case...
Attached patch wip (obsolete) (deleted) — Splinter Review
This patch fixes the crash, but maybe we should call WillDestroyFrameTree() instead, to clear counters and pending restyles - there's no point in keeping those if we recreate the entire frame tree is there? http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1408&root=/cvsroot&mark=10190#10183 more later...
Yes, clearing quotes, counters, and pending restyles here would be a really good idea.
Attached file Testcase #2 (deleted) —
Attached file trace + frame dump (for Testcase #2) (deleted) —
There is actually a problem with the unconditional RemoveMappingsForFrameSubtree() that we added in bug 372685. This is the frame dump + trace for Testcase #2. This time ReconstructDocElementHierarchyInternal() is not involved at all, it's a restyle causing RecreateFramesForContent(). See the original frame tree before it begins at the top, and the stack at the bottom. Note that all popup-list children are out-of-flows (red,blue,pink). We should not call RemoveMappingsForFrameSubtree() for out-of-flows when the placeholder is on a reachable frame list since we would traverse the same frames twice (same as DoDeletingFrameSubtree() avoids those lists). - aFc->RemoveMappingsForFrameSubtree(aPopup); + if (!(aPopup->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) + aFc->RemoveMappingsForFrameSubtree(aPopup); would do the right thing, but... After reading the frame construction code, it looks like a PopupSet's ::popupList can only contain out-of-flows which means we don't need to call RemoveMappingsForFrameSubtree() at all here since all these frames will be reachable (directly or indirectly) through a placeholder from outside the ::popupList itself. So, I'm leaning towards removing it again and adding assertions instead... I'll investigate some more...
Attached patch Patch rev. 1 (deleted) — Splinter Review
* call WillDestroyFrameTree() before destroying the frame tree * revert the patch from bug 372685 and add assertions that the ::popupList frames must be out-of-flow MenuPopupFrames * add nsPopupSetFrame::List() that also prints the ::popupList frames
Attachment #284041 - Attachment is obsolete: true
Attachment #284978 - Flags: superreview?(bzbarsky)
Attachment #284978 - Flags: review?(bzbarsky)
Attached patch Mochitests (deleted) — Splinter Review
The attached testcases in Mochitest form.
Comment on attachment 284978 [details] [diff] [review] Patch rev. 1 DEBUG, not NS_DEBUG, please. r+sr=bzbarsky with that.
Attachment #284978 - Flags: superreview?(bzbarsky)
Attachment #284978 - Flags: superreview+
Attachment #284978 - Flags: review?(bzbarsky)
Attachment #284978 - Flags: review+
Comment on attachment 284978 [details] [diff] [review] Patch rev. 1 ok, I'll fix that before checkin
Attachment #284978 - Flags: approval1.9?
Flags: blocking1.9? → blocking1.9+
Whiteboard: [dbaron-1.9:Rs]
Fyi, the patch doesn't need approval, because this bug is blockg1.9+, so you should be ale to check it in.
That's no longer the case, at least till after beta. It needs approval now to land for beta1.
I think I also get this crash while playing around with the search input in the Places Organizer: http://crash-stats.mozilla.com/report/index/fd7d6f30-8171-11dc-a531-001a4bd46e84
Looks like it. Maybe we should take this for beta1 then?
Target Milestone: --- → mozilla1.9 M9
Should this definitely block beta? Criteria to use to judge: * Most sites should display properly and regression free (from previous major release) * No known common data loss bugs * No common hangs or crashes * No problems with major features in common use cases Also, what's the risk of regression w/ this patch? Thanks for any clarification here.
Comment on attachment 284978 [details] [diff] [review] Patch rev. 1 Approved to land for beta. Please check in ASAP.
Attachment #284978 - Flags: approval1.9? → approval1.9+
mozilla/layout/xul/test/Makefile.in 1.4 mozilla/layout/xul/test/test_bug398982-1.xul 1.1 mozilla/layout/xul/test/test_bug398982-2.xul 1.1 mozilla/layout/base/nsCSSFrameConstructor.cpp 1.1417 mozilla/layout/xul/base/src/nsPopupSetFrame.cpp 1.172 mozilla/layout/xul/base/src/nsPopupSetFrame.h 1.62 -> FIXED
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102407 Minefield/3.0a9pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102404 Minefield/3.0a9pre. I ran both testcases in the bug and no crash was observed.
Status: RESOLVED → VERIFIED
Crash Signature: [@ DoDeletingFrameSubtree]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: