Closed Bug 256108 Opened 20 years ago Closed 20 years ago

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

Categories

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

Product:
Core
Component:
Layout: Positioned
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.8alpha5

People

(Reporter: sekundes, Assigned: bzbarsky)

References

(
URL
)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(3 files, 1 obsolete file)

Minimized testcase
415 bytes, application/xhtml+xml
Details
Patch as diff -w
4.10 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
Patch for checkin
7.16 KB, patch
Details | Diff | Splinter Review 
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.
Confirm, 1.8a3 PC/WinXP
Talkback TB598661X
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash
Summary: crash when switching off style sheet on specific site → crash when switching off style sheet [@0x00000000 - DoDeletingFrameSubtree]
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)
(It crashes also under Linux)
OS: Windows 2000 → All
Hardware: PC → All
Attached file Minimized testcase — 

    
    

    
  
I think from the Talkback that this is a Layout bug.
Assignee: general → nobody
Component: Browser-General → Layout
Keywords: testcase
QA Contact: general → core.layout

    
    

    
  
Since "Page Style" feature is back in Firefox, I'd nominate for Firefox 1.0.
Flags: blocking-aviary1.0?

    
    

    
  
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...
Component: Layout → Layout: R & A Pos

    
    

    
  
Hmmm... it looks like mFixedItems should already be handling the delay thing I
mentioned.  I wonder why that's not working.

    
    

    
  
Don't we already maintain some sort of list of out-of-flow frames that have been
destroyed?  Why aren't we using that?

    
    

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

    
    

    
  


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch as diff -w
        — 
          — Splinter Review
      

    


  
Previous patch was not quite right.

        Attachment #160106 -
        Attachment is obsolete: true

    
    

    
  


  

    
    

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

David, would you review?

        Attachment #160107 -
        Flags: superreview?(dbaron)

        Attachment #160107 -
        Flags: review?(dbaron)

    
    

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

    
    

    
  
Taking.
Assignee: nobody → bzbarsky
Priority: -- → P1
Summary: crash when switching off style sheet [@0x00000000 - DoDeletingFrameSubtree] → [FIX]crash when switching off style sheet [@0x00000000 - DoDeletingFrameSubtree]
Target Milestone: --- → mozilla1.8beta

    
    

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

    
    

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

    
  
Flags: blocking-aviary1.0? → blocking-aviary1.0-

    
    

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

r+sr=dbaron, although a cleaner approach would be nice in the future

        Attachment #160107 -
        Flags: superreview?(dbaron)

        Attachment #160107 -
        Flags: superreview+

        Attachment #160107 -
        Flags: review?(dbaron)

        Attachment #160107 -
        Flags: review+

    
    

    
  
Fixed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8beta → mozilla1.8alpha5

    
    

    
  
V 20041006 PC/WinXP
Status: RESOLVED → VERIFIED

    
    

    
  
*** Bug 284122 has been marked as a duplicate of this bug. ***
Crash Signature: [@0x00000000 - DoDeletingFrameSubtree]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: