Last Comment Bug 256108 - [FIX]crash when switching off style sheet [@0x00000000 - DoDeletingFrameSubtree]
: [FIX]crash when switching off style sheet [@0x00000000 - DoDeletingFrameSubtree]
Status: VERIFIED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: Layout: R & A Pos (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.8alpha5
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
http://literarymoose.info/
: 284122 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-08-19 01:05 PDT by Sekundes
Modified: 2011-06-09 14:58 PDT (History)
8 users (show)
asa: blocking‑aviary1.0-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Minimized testcase (415 bytes, application/xhtml+xml)
2004-08-19 02:27 PDT, Bill Mason
no flags Details
Patch fixing issue #2 in comment 7 (7.16 KB, patch)
2004-09-25 20:25 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Patch as diff -w (4.10 KB, patch)
2004-09-25 20:28 PDT, Boris Zbarsky [:bz]
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
Patch for checkin (7.16 KB, patch)
2004-09-25 20:29 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review

Description Sekundes 2004-08-19 01:05:15 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.2) Gecko/20040816 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.2) Gecko/20040816 Firefox/0.9.1+

It could be reproduced on Firefox nightly with Gecko 1.7.2, and Mozilla 1.8alpha3.

Reproducible: Always
Steps to Reproduce:
1. Go to http://literarymoose.info/
2. View -> Use Style -> None.
3. Crashes.
Comment 1 Bill Mason 2004-08-19 01:15:09 PDT
Confirm, 1.8a3 PC/WinXP
Talkback TB598661X
Comment 2 Stephen Donner [:stephend] 2004-08-19 01:39:52 PDT
0x00000000
DoDeletingFrameSubtree 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 9125]
DoDeletingFrameSubtree 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 9155]
DeletingFrameSubtree 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 9201]
nsCSSFrameConstructor::RemoveFixedItems 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 13306]
nsCSSFrameConstructor::ReconstructDocElementHierarchy 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 7353]
nsCSSFrameConstructor::RecreateFramesForContent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 11353]
nsCSSFrameConstructor::ProcessRestyledFrames 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 9869]
PresShell::ReconstructStyleData 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 5283]
PresShell::SetAuthorStyleDisabled 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 2137]
DocumentViewerImpl::SetAuthorStyleDisabled 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsDocumentViewer.cpp,
line 2360]
XPTC_InvokeByIndex 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 102]
XPCWrappedNative::CallMethod 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,
line 2030]
XPC_WN_GetterSetter 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1312]
js_Invoke 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 1283]
js_InternalInvoke 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 1379]
js_InternalGetOrSet 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 1422]
js_Interpret 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 3219]
js_Invoke 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 1302]
js_InternalInvoke 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 1379]
JS_CallFunctionValue 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsapi.c, line
3686]
nsJSContext::CallEventHandler 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp,
line 1352]
nsJSEventListener::HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/events/nsJSEventListener.cpp,
line 180]
nsEventListenerManager::HandleEventSubType 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1513]
nsEventListenerManager::HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp,
line 1590]
nsXULElement::HandleDOMEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/content/src/nsXULElement.cpp,
line 2823]
PresShell::HandleDOMEventWithTarget 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6090]
nsMenuFrame::Execute 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsMenuFrame.cpp,
line 1637]
nsMenuFrame::HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsMenuFrame.cpp,
line 436]
PresShell::HandleEventInternal 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6060]
PresShell::HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 5925]
nsViewManager::HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2295]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2025]
HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp,
line 79]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1101]
nsWindow::DispatchWindowEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1118]
nsWindow::DispatchMouseEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 5404]
ChildWindow::DispatchMouseEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 5655]
nsWindow::ProcessMessage 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 4159]
nsWindow::WindowProc 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1380]
USER32.dll + 0x3a50 (0x77d43a50)
USER32.dll + 0x3b1f (0x77d43b1f)
USER32.dll + 0x3d79 (0x77d43d79)
USER32.dll + 0x3ddf (0x77d43ddf)
nsAppShellService::Run 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/appshell/src/nsAppShellService.cpp,
line 489]
main1 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1331]
main 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1801]
WinMain 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1827]
WinMainCRTStartup()
kernel32.dll + 0x214c7 (0x77e814c7)
Comment 3 Xanthor Sccas 2004-08-19 02:14:34 PDT
(It crashes also under Linux)
Comment 4 Bill Mason 2004-08-19 02:27:52 PDT
Created attachment 156494 [details]
Minimized testcase
Comment 5 Bill Mason 2004-08-19 02:28:58 PDT
I think from the Talkback that this is a Layout bug.
Comment 6 Sekundes 2004-09-10 21:11:07 PDT
Since "Page Style" feature is back in Firefox, I'd nominate for Firefox 1.0.
Comment 7 Boris Zbarsky [:bz] 2004-09-25 19:03:09 PDT
So I debugged this a bit.  The basic problem here is markup like this:

<div style="position: fixed">
  <div style="position: fixed"></div>
</div>

combined with something that causes us to reframe the root (the change in
::after content, in this case, but other thing could be used too).  When we
reframe the root (in ReconstructDocElementHierarchy), we call RemoveFixedItems,
which goes through the list of frames in the nsLayoutAtoms::fixedList of the
viewport and calls DeletingFrameSubtree on each one, then destroys it.

The problem is that the frame for the inner div comes _before_ the frame for the
outer div in the fixedList.  So we destroy it, then go on to the outer div's
frame, try to traverse through placeholders in DoDeletingFrameSubtree, and
crash, since the placeholder points to a deleted frame.

The reason that fixed frames end up out of order is that we put them in the
fixed list as we recurse up out of frame construction, and the inner one
naturally finishes constructing first...

Possible solutions here:

1)  Make the fixed frames be in the right order in the fixed list (by delaying
    adding them to the viewport until after we've recursed out of frame
    construction or something).
2)  Set the placeholder frames for the fixed frames to point to null before
    calling RemoveFixedItems() in ReconstructDocElementHierarchy (this means
    doing it before ClearPlaceholderFrameMap()).

I think we should do both, really...
Comment 8 Boris Zbarsky [:bz] 2004-09-25 19:36:06 PDT
Hmmm... it looks like mFixedItems should already be handling the delay thing I
mentioned.  I wonder why that's not working.
Comment 9 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2004-09-25 19:49:57 PDT
Don't we already maintain some sort of list of out-of-flow frames that have been
destroyed?  Why aren't we using that?
Comment 10 Boris Zbarsky [:bz] 2004-09-25 20:09:09 PDT
> Don't we already maintain some sort of list of out-of-flow frames that have
> been destroyed?

We have something like that in ProcessRestyledFrames().  I suppose we could
reuse the same idea here.  Other than that, I don't think we have anything, no.
 DeletingFrameSubtree() keeps track of out-of-flows it intends to destroy, but
we're dealing with two separate calls into DeletingFrameSubtree() here...

I found why the mFixedItems thing isn't working -- people are processing
children _before_ adding frames to aState.mFixedItems.  That seems wrong to me
(and is painful-ish to fix because of the multiple codepaths that happen in some
places...).
Comment 11 Boris Zbarsky [:bz] 2004-09-25 20:25:48 PDT
Created attachment 160106 [details] [diff] [review]
Patch fixing issue #2 in comment 7
Comment 12 Boris Zbarsky [:bz] 2004-09-25 20:28:16 PDT
Created attachment 160107 [details] [diff] [review]
Patch as diff -w

Previous patch was not quite right.
Comment 13 Boris Zbarsky [:bz] 2004-09-25 20:29:10 PDT
Created attachment 160108 [details] [diff] [review]
Patch for checkin
Comment 14 Boris Zbarsky [:bz] 2004-09-25 20:30:29 PDT
Comment on attachment 160107 [details] [diff] [review]
Patch as diff -w

David, would you review?
Comment 15 Boris Zbarsky [:bz] 2004-09-25 20:34:59 PDT
I filed bug 261605 on the first issue I mention in comment 7.
Comment 16 Boris Zbarsky [:bz] 2004-09-26 20:32:26 PDT
Taking.
Comment 17 chris hofmann 2004-09-28 18:59:40 PDT
bz/dbaron,  is this avaiary 1.7 branch kind of changes?
Comment 18 Boris Zbarsky [:bz] 2004-09-28 19:10:13 PDT
I'd be very wary of landing it on a branch....
Comment 19 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2004-10-05 16:46:05 PDT
Comment on attachment 160107 [details] [diff] [review]
Patch as diff -w

r+sr=dbaron, although a cleaner approach would be nice in the future
Comment 20 Boris Zbarsky [:bz] 2004-10-05 18:53:50 PDT
Fixed.
Comment 21 Bill Mason 2004-10-06 19:22:48 PDT
V 20041006 PC/WinXP
Comment 22 Boris Zbarsky [:bz] 2005-03-01 12:08:38 PST
*** Bug 284122 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.