256108 - [FIX]crash when switching off style sheet [@0x00000000 - DoDeletingFrameSubtree]

Status: VERIFIED FIXED

(Core :: Layout: Positioned, defect, P1)

Description

[Sekundes]

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]

Confirm, 1.8a3 PC/WinXP
Talkback TB598661X

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

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]

(It crashes also under Linux)

Comment 4

[Bill Mason]

Attachment: Minimized testcase

Comment 5

[Bill Mason]

I think from the Talkback that this is a Layout bug.

Comment 6

[Sekundes]

Since "Page Style" feature is back in Firefox, I'd nominate for Firefox 1.0.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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-5, no longer working on Mozilla)]

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 [:bzbarsky]]

> 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 [:bzbarsky]]

Attachment: Patch fixing issue #2 in comment 7

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch as diff -w

Previous patch was not quite right.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for checkin

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 160107 [details] [diff] [review]
Patch as diff -w

David, would you review?

Comment 15

[Boris Zbarsky [:bzbarsky]]

I filed bug 261605 on the first issue I mention in comment 7.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Taking.

Comment 17

[chris hofmann]

bz/dbaron,  is this avaiary 1.7 branch kind of changes?

Comment 18

[Boris Zbarsky [:bzbarsky]]

I'd be very wary of landing it on a branch....

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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 [:bzbarsky]]

Fixed.

Comment 21

[Bill Mason]

V 20041006 PC/WinXP

Comment 22

[Boris Zbarsky [:bzbarsky]]

*** Bug 284122 has been marked as a duplicate of this bug. ***