Last Comment Bug 729955 - Debug Crash [@ nsIFrame::GetSize() ]
: Debug Crash [@ nsIFrame::GetSize() ]
Status: RESOLVED FIXED
: crash, regression, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: mozilla14
Assigned To: Matt Woodrow (:mattwoodrow)
:
Mentors:
http://acko.net/blog/this-is-your-bra...
: 767727 (view as bug list)
Depends on:
Blocks: 532972 721082
  Show dependency treegraph
 
Reported: 2012-02-23 08:04 PST by Bob Clary [:bc:]
Modified: 2012-06-23 11:52 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (4.92 KB, text/html)
2012-03-06 10:28 PST, Bob Clary [:bc:]
no flags Details
Invalidate before destroying the placeholder (1.20 KB, patch)
2012-03-21 20:02 PDT, Matt Woodrow (:mattwoodrow)
bzbarsky: review-
Details | Diff | Review
Invalidate before destroying the placeholder v2 (4.00 KB, patch)
2012-03-21 20:38 PDT, Matt Woodrow (:mattwoodrow)
bzbarsky: review+
Details | Diff | Review

Description Bob Clary [:bc:] 2012-02-23 08:04:11 PST
1. http://acko.net/blog/this-is-your-brain-on-css/
2. Crash Debug Nightly on Mac, Linux, Windows

I didn't crash with an opt build in my limited testing.

I could reproduce with a saved version but it was very finicky to reduce. I'll try again later.

Operating system: Windows NT
                  6.1.7601 Service Pack 1
CPU: x86
     GenuineIntel family 6 model 37 stepping 1
     2 CPUs

Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
Crash address: 0x10

Thread 0 (crashed)
 0  xul.dll!nsIFrame::GetSize() [nsIFrame.h : 830 + 0xa]
    eip = 0x6ed0e4aa   esp = 0x0035b6ac   ebp = 0x0035b6b0   ebx = 0x00000001
    esi = 0x03f10b68   edi = 0x00000000   eax = 0x00000000   ecx = 0x00000000
    edx = 0x00000034   efl = 0x00210246
    Found by: given as instruction pointer in context
 1  xul.dll!nsDisplayTransform::GetFrameBoundsForTransform(nsIFrame const *) [nsDisplayList.cpp : 2354 + 0xb]
    eip = 0x6ed399aa   esp = 0x0035b6b8   ebp = 0x0035b6cc
    Found by: call frame info
 2  xul.dll!GetDeltaToMozPerspectiveOrigin [nsDisplayList.cpp : 2468 + 0x1d]
    eip = 0x6ed3a1ec   esp = 0x0035b6d4   ebp = 0x0035b774
    Found by: call frame info
 3  xul.dll!nsDisplayTransform::GetResultingTransformMatrix(nsIFrame const *,nsPoint const &,float,nsRect const *,nsIFrame * *) [nsDisplayList.cpp : 2571 + 0x1a]
    eip = 0x6ed39c46   esp = 0x0035b77c   ebp = 0x0035ba3c
    Found by: call frame info
 4  xul.dll!nsDisplayTransform::TransformRectOut(nsRect const &,nsIFrame const *,nsPoint const &,nsRect const *) [nsDisplayList.cpp : 2943 + 0x24]
    eip = 0x6ed3b1e1   esp = 0x0035ba44   ebp = 0x0035baa8
    Found by: call frame info

related assertions:

###!!! ASSERTION: no placeholder frame for out-of-flow frame: 'Not Reached', file /work/mozilla/builds/nightly/mozilla/layout/generic/nsFrame.cpp, line 7139
###!!! ASSERTION: Can't get delta without a style parent!: 'aFrame->GetParentStyleContextFrame()', file /work/mozilla/builds/nightly/mozilla/layout/base/nsDisplayList.cpp, line 2456
###!!! ASSERTION: no placeholder frame for out-of-flow frame: 'Not Reached', file /work/mozilla/builds/nightly/mozilla/layout/generic/nsFrame.cpp, line 7139
###!!! ASSERTION: Can't get the bounds of a nonexistent frame!: 'aFrame', file /work/mozilla/builds/nightly/mozilla/layout/base/nsDisplayList.cpp, line 2353
Comment 1 Bob Clary [:bc:] 2012-03-06 10:28:44 PST
Created attachment 603338 [details]
testcase

###!!! ASSERTION: no placeholder frame for out-of-flow frame: 'Not Reached', file /work/mozilla/builds/nightly/mozilla/layout/generic/nsFrame.cpp, line 7141
###!!! ASSERTION: Can't get delta without a style parent!: 'aFrame->GetParentStyleContextFrame()', file /work/mozilla/builds/nightly/mozilla/layout/base/nsDisplayList.cpp, line 2461
###!!! ASSERTION: no placeholder frame for out-of-flow frame: 'Not Reached', file /work/mozilla/builds/nightly/mozilla/layout/generic/nsFrame.cpp, line 7141
###!!! ASSERTION: Can't get the bounds of a nonexistent frame!: 'aFrame', file /work/mozilla/builds/nightly/mozilla/layout/base/nsDisplayList.cpp, line 2358

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000010
0x05ba9769 in nsIFrame::GetSize (this=0x0) at nsIFrame.h:830
830	  nsSize GetSize() const { return nsSize(mRect.width, mRect.height); }
Comment 2 Alice0775 White 2012-03-09 07:07:18 PST
Confirmed:
bp-6888d516-f76a-4af6-9b27-7c0d02120309 .
http://hg.mozilla.org/mozilla-central/rev/2f6368ca605e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120308 Firefox/13.0a1 ID:20120308031058

Regression window(m-c),
Not crash:
http://hg.mozilla.org/mozilla-central/rev/81f6b9cbb2a9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120214 Firefox/13.0a1 ID:20120215021649
Crashes:
http://hg.mozilla.org/mozilla-central/rev/46e22ce549b0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120215 Firefox/13.0a1 ID:20120215083849
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=81f6b9cbb2a9&tochange=46e22ce549b0


Regression window(m-i),
Not crash:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1db98753a46f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120214 Firefox/13.0a1 ID:20120215011955
Crashes:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3884d70d2f7d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120214 Firefox/13.0a1 ID:20120215012949
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1db98753a46f&tochange=3884d70d2f7d

Last Good : 1158b09686b7	Matt Woodrow — Bug 721082 - Constify nsIFrame::GetParentStyleContextFrame. r=roc
First bad : b736157e56f6	Matt Woodrow — Bug 721082 - Make perspective-origin relative to the parent elements border box. r=roc
Comment 3 Bob Clary [:bc:] 2012-03-09 07:12:48 PST
Hey Matt, can you look at this?
Comment 4 Matt Woodrow (:mattwoodrow) 2012-03-11 16:18:22 PDT
GetParentStyleContextFrame() is returning NULL here, I'm not sure why.

bz: Is this expected? for this to return null, when aFrame->GetStyleContext()->GetParent() is returning a valid style context.

I'm not sure what we should be doing if this is expected, since perspective needs the parents bounding box size.
Comment 5 Boris Zbarsky [:bz] 2012-03-11 21:29:18 PDT
What does the frame tree look like here?  Which of those frames are we calling GetParentStyleContextFrame() on?
Comment 6 Matt Woodrow (:mattwoodrow) 2012-03-19 18:19:09 PDT
Frame tree: http://pastebin.mozilla.org/1526954

frame: 0x11c128b50
sc: 0x11c127f10
sc->Parent: 0x11aea79b0
Comment 7 Boris Zbarsky [:bz] 2012-03-19 20:39:11 PDT
Well, there's nothing in that tree with sc=0x11aea79b0 offhand.

Worse yet, if you look at the frame dump, there is no placeholder corresponding to the Block(section) that I can see.  I would have expected to see it as a child of the Block(div) which has as its parent the Block(body), but I don't see any of that stuff in there at all.  The Block(html) has a single empty line and that's it.  Where did the frames for the <body> go?  And when they went away, why didn't the Block(section) go away?

Given the busted frame tree, the null return value from GetParentStyleContextFrame() is "expected"...

Where was that data gathered?  Is it possibly in the middle of frame construction?
Comment 8 Matt Woodrow (:mattwoodrow) 2012-03-19 20:48:06 PDT
Looks like frame destruction:

#0  0x00000001016e0bc0 in nsIFrame::GetSize (this=0x0) at nsIFrame.h:830
#1  0x000000010170fbe1 in nsDisplayTransform::GetFrameBoundsForTransform (aFrame=0x0) at nsDisplayList.cpp:2359
#2  0x00000001017161c3 in GetDeltaToMozPerspectiveOrigin (aFrame=0x10ce2ef70, aAppUnitsPerPixel=60) at nsDisplayList.cpp:2471
#3  0x00000001017171b8 in nsDisplayTransform::GetResultingTransformMatrix (aFrame=0x10ce2ef70, aOrigin=@0x7fff5fbf9ea8, aAppUnitsPerPixel=60, aBoundsOverride=0x0, aOutAncestor=0x0) at nsDisplayList.cpp:2575
#4  0x00000001017176ae in nsDisplayTransform::TransformRectOut (aUntransformedBounds=@0x7fff5fbfa0d8, aFrame=0x10ce2ef70, aOrigin=@0x7fff5fbf9ea8, aBoundsOverride=0x0) at nsDisplayList.cpp:2955
#5  0x0000000101845b38 in nsIFrame::InvalidateInternalAfterResize (this=0x10ce2ef70, aDamageRect=@0x7fff5fbfa0d8, aX=0, aY=0, aFlags=0) at nsFrame.cpp:4593
#6  0x0000000101845db5 in nsIFrame::InvalidateInternal (this=0x10ce2ef70, aDamageRect=@0x7fff5fbfa0d8, aX=0, aY=0, aForChild=0x10ce2ef70, aFlags=0) at nsFrame.cpp:4627
#7  0x00000001018116af in nsBlockFrame::InvalidateInternal (this=0x10ce2ef70, aDamageRect=@0x7fff5fbfa0d8, aX=0, aY=0, aForChild=0x0, aFlags=0) at nsBlockFrame.cpp:540
#8  0x000000010183149d in nsIFrame::InvalidateWithFlags (this=0x10ce2ef70, aDamageRect=@0x7fff5fbfa0d8, aFlags=0) at nsFrame.cpp:4522
#9  0x00000001017c892a in nsIFrame::Invalidate (this=0x10ce2ef70, aDamageRect=@0x7fff5fbfa0d8) at nsIFrame.h:2103
#10 0x0000000101842137 in nsIFrame::InvalidateFrameSubtree (this=0x10ce2ef70) at nsFrame.cpp:4708
#11 0x0000000101746e15 in nsFrameManager::RemoveFrame (this=0x11b154800, aListID=mozilla::layout::kAbsoluteList, aOldFrame=0x10ce2ef70) at nsFrameManager.cpp:515
#12 0x00000001018b9de7 in nsPlaceholderFrame::DestroyFrom (this=0x10ce2f008, aDestructRoot=0x119f915b0) at nsPlaceholderFrame.cpp:169
#13 0x00000001018a4884 in nsLineBox::DeleteLineList (aPresContext=0x117c77000, aLines=@0x114239fb8, aDestructRoot=0x119f915b0) at nsLineBox.cpp:406
#14 0x0000000101812651 in nsBlockFrame::DestroyFrom (this=0x114239f40, aDestructRoot=0x119f915b0) at nsBlockFrame.cpp:321
#15 0x000000010185c811 in nsFrameList::DestroyFramesFrom (this=0x1142388d8, aDestructRoot=0x119f915b0) at nsFrameList.cpp:94
#16 0x000000010182bd52 in nsContainerFrame::DestroyFrom (this=0x114238878, aDestructRoot=0x119f915b0) at nsContainerFrame.cpp:250
#17 0x0000000101870f03 in nsHTMLScrollFrame::DestroyFrom (this=0x114238878, aDestructRoot=0x119f915b0) at nsGfxScrollFrame.cpp:125
#18 0x00000001018a4884 in nsLineBox::DeleteLineList (aPresContext=0x117c77000, aLines=@0x119f91628, aDestructRoot=0x119f915b0) at nsLineBox.cpp:406
#19 0x0000000101812651 in nsBlockFrame::DestroyFrom (this=0x119f915b0, aDestructRoot=0x119f915b0) at nsBlockFrame.cpp:321
#20 0x0000000101ad9bf3 in nsIFrame::Destroy (this=0x119f915b0) at nsIFrame.h:582
#21 0x00000001018018cf in nsBlockFrame::DoRemoveFrame (this=0x119f8f870, aDeletedFrame=0x119f915b0, aFlags=2) at nsBlockFrame.cpp:5525
#22 0x0000000101802142 in nsBlockFrame::RemoveFrame (this=0x119f8f870, aListID=mozilla::layout::kPrincipalList, aOldFrame=0x119f915b0) at nsBlockFrame.cpp:5079
#23 0x0000000101746ff4 in nsFrameManager::RemoveFrame (this=0x11b154800, aListID=mozilla::layout::kPrincipalList, aOldFrame=0x119f915b0) at nsFrameManager.cpp:531
#24 0x00000001016dc8a9 in nsCSSFrameConstructor::ContentRemoved (this=0x11b154800, aContainer=0x11a0c99d0, aChild=0x10d81e820, aOldNextSibling=0x0, aFlags=nsCSSFrameConstructor::REMOVE_FOR_RECONSTRUCTION, aDidReconstruct=0x7fff5fbfaa37) at nsCSSFrameConstructor.cpp:7450
#25 0x00000001016db35a in nsCSSFrameConstructor::RecreateFramesForContent (this=0x11b154800, aContent=0x10d81e820, aAsyncInsert=false) at nsCSSFrameConstructor.cpp:9049
#26 0x00000001016dd9bc in nsCSSFrameConstructor::ProcessRestyledFrames (this=0x11b154800, aChangeList=@0x7fff5fbfac70) at nsCSSFrameConstructor.cpp:7893
#27 0x00000001016de591 in nsCSSFrameConstructor::RestyleElement (this=0x11b154800, aElement=0x10d81e820, aPrimaryFrame=0x119f915b0, aMinHint=0, aRestyleTracker=@0x11b1548e8, aRestyleDescendants=true) at nsCSSFrameConstructor.cpp:8006
#28 0x00000001016ba58d in mozilla::css::RestyleTracker::ProcessOneRestyle (this=0x11b1548e8, aElement=0x10d81e820, aRestyleHint=3, aChangeHint=0) at RestyleTracker.cpp:157
#29 0x00000001016b897c in mozilla::css::RestyleTracker::DoProcessRestyles (this=0x11b1548e8) at RestyleTracker.cpp:242
#30 0x00000001016e46b7 in mozilla::css::RestyleTracker::ProcessRestyles (this=0x11b1548e8) at RestyleTracker.h:101
#31 0x00000001016de2a7 in nsCSSFrameConstructor::ProcessPendingRestyles (this=0x11b154800) at nsCSSFrameConstructor.cpp:11569
#32 0x000000010179ba94 in PresShell::FlushPendingNotifications (this=0x11a970800, aType=Flush_InterruptibleLayout) at nsPresShell.cpp:3961
#33 0x00000001017842f8 in PresShell::WillPaint (this=0x11a970800, aWillSendDidPaint=false) at nsPresShell.cpp:7117
#34 0x0000000102238639 in nsViewManager::CallWillPaintOnObservers (this=0x10cd4c880, aWillSendDidPaint=false) at nsViewManager.cpp:1383
#35 0x000000010223b732 in nsViewManager::DispatchEvent (this=0x10cd4c880, aEvent=0x7fff5fbfc348, aView=0x10cd18660, aStatus=0x7fff5fbfc154) at nsViewManager.cpp:777
#36 0x0000000102234ee9 in HandleEvent (aEvent=0x7fff5fbfc348) at nsView.cpp:158
#37 0x000000010323fc74 in nsChildView::DispatchEvent (this=0x10d0b37d0, event=0x7fff5fbfc348, aStatus=@0x7fff5fbfc2e4) at nsChildView.mm:1512
#38 0x00000001032386bc in nsChildView::DispatchWindowEvent (this=0x10d0b37d0, event=@0x7fff5fbfc348) at nsChildView.mm:1522
#39 0x0000000103233d68 in -[ChildView viewWillDraw] (self=0x10d0be9d0, _cmd=0x7fff8412ae1e) at nsChildView.mm:2707
#40 0x00007fff83871c11 in -[NSView viewWillDraw] ()
#41 0x00007fff83871c11 in -[NSView viewWillDraw] ()
#42 0x00007fff83870952 in -[NSView _sendViewWillDrawInRect:clipRootView:suppressRecursion:] ()
#43 0x00007fff8386f6c1 in -[NSView displayIfNeeded] ()
#44 0x00007fff8386f07d in _handleWindowNeedsDisplayOrLayoutOrUpdateConstraints ()
#45 0x00007fff8d379f37 in __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ ()
#46 0x00007fff8d379e96 in __CFRunLoopDoObservers ()
#47 0x00007fff8d34f159 in __CFRunLoopRun ()
#48 0x00007fff8d34eae6 in CFRunLoopRunSpecific ()
#49 0x00007fff8dd813d3 in RunCurrentEventLoopInMode ()
#50 0x00007fff8dd8858f in ReceiveNextEventCommon ()
#51 0x00007fff8dd884ca in BlockUntilNextEventMatchingListInMode ()
#52 0x00007fff838333f1 in _DPSNextEvent ()
#53 0x00007fff83832cf5 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#54 0x000000010320b3ba in -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (self=0x100310230, _cmd=0x7fff840cda78, mask=18446744073709551615, expiration=0x422d63c37f00000d, mode=0x7fff7303bae0, flag=1 '\001') at nsAppShell.mm:208
#55 0x00007fff8382f62d in -[NSApplication run] ()
#56 0x000000010320c237 in nsAppShell::Run (this=0x10910eac0) at nsAppShell.mm:795
#57 0x0000000102dee2b2 in nsAppStartup::Run (this=0x10910ca10) at nsAppStartup.cpp:295
#58 0x0000000101285261 in XRE_main (argc=3, argv=0x7fff5fbffab0, aAppData=0x1000081c0) at nsAppRunner.cpp:3703
#59 0x0000000100001362 in do_main (argc=3, argv=0x7fff5fbffab0) at nsBrowserApp.cpp:190
#60 0x0000000100001b7c in main (argc=3, argv=0x7fff5fbffab0) at nsBrowserApp.cpp:277
Comment 9 Boris Zbarsky [:bz] 2012-03-20 09:45:20 PDT
Ah, yes.  Walking the frame tree during frame destruction seems like a bad idea.

In particular, the sequence of events here is as follows:

1)  We end up in nsPlaceholderFrame::DestroyFrom for the placeholder of that out-of-flow
    <section>.
2)  The link between the out-of-flow and the placeholder is severed.
3)  Since the destruct root doesn't contain the out-of-flow, an explicit RemoveFrame call
    is made on the out-of-flow.
4)  nsFrameManager::RemoveFrame invalidates the frame before removing it (can we nuke that
    in bug 539356 or a followup to that bug, perhaps?)
5)  Invalidation as of bug 721082 involves looking for style context parents and we lose.

Just adding a null-check seems like it would lead to incorrect invalidation, right?

The simplest fix is probably adding a boolean argument to RemoveFrame to skip the invalidation and invalidating manually in the placeholder teardown code before unhooking the out-of-flow and the placeholder. But even that seems somewhat dangerous, because the frame tree is not exactly in a consistent state here and fundamentally getting the style context parent (as well as invalidation in general!) involve walking around the frame tree....  Maybe it's good enough for now pending item 4 above getting fixed?
5)
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-20 11:48:11 PDT
(In reply to Boris Zbarsky (:bz) from comment #9)
> 4)  nsFrameManager::RemoveFrame invalidates the frame before removing it
> (can we nuke that
>     in bug 539356 or a followup to that bug, perhaps?)

Yes, bug 539356 should kill that.

> The simplest fix is probably adding a boolean argument to RemoveFrame to
> skip the invalidation and invalidating manually in the placeholder teardown
> code before unhooking the out-of-flow and the placeholder. But even that
> seems somewhat dangerous, because the frame tree is not exactly in a
> consistent state here and fundamentally getting the style context parent (as
> well as invalidation in general!) involve walking around the frame tree.... 
> Maybe it's good enough for now pending item 4 above getting fixed?
> 5)

Probably.
Comment 11 Matt Woodrow (:mattwoodrow) 2012-03-21 18:54:53 PDT
(In reply to Boris Zbarsky (:bz) from comment #9)
> Just adding a null-check seems like it would lead to incorrect invalidation,
> right?

Right.

> 
> The simplest fix is probably adding a boolean argument to RemoveFrame to
> skip the invalidation and invalidating manually in the placeholder teardown
> code before unhooking the out-of-flow and the placeholder.

Where is the code for this? RemoveFrame already calls InvalidateFrameSubtree() before doing any other work, isn't this enough to invalidate everything?

It seems like the other invalidations called from below this aren't required at all.
Comment 12 Boris Zbarsky [:bz] 2012-03-21 19:12:29 PDT
The point is that by the time RemoveFrame is called here (from nsPlaceholderFrame::DestroyFrom) the frame tree is already in an inconsistent state.  In particular, trying to find the style parent of the frame the RemoveFrame is called on will crash as in this bug.

So my proposal (again, modulo whatever happens in bug 539356) is that nsPlaceholderFrame::DestroyFrom should do the invalidate _before_ putting the frame tree in an inconsistent state, and tell RemoveFrame to not do the invalidation itself.
Comment 13 Matt Woodrow (:mattwoodrow) 2012-03-21 19:36:01 PDT
Right, got it. I was looking at the earlier RemoveFrame call (#23 in the stack trace).
Comment 14 Matt Woodrow (:mattwoodrow) 2012-03-21 20:02:50 PDT
Created attachment 608206 [details] [diff] [review]
Invalidate before destroying the placeholder

Fixes the crash for me
Comment 15 Boris Zbarsky [:bz] 2012-03-21 20:15:35 PDT
Comment on attachment 608206 [details] [diff] [review]
Invalidate before destroying the placeholder

I think you forgot to qref...
Comment 16 Matt Woodrow (:mattwoodrow) 2012-03-21 20:38:05 PDT
Created attachment 608211 [details] [diff] [review]
Invalidate before destroying the placeholder v2

Sure did!
Comment 17 Matt Woodrow (:mattwoodrow) 2012-03-26 11:28:11 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dec7f7bb0302
Comment 18 Marco Bonardo [::mak] 2012-03-27 05:29:41 PDT
https://hg.mozilla.org/mozilla-central/rev/dec7f7bb0302
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-23 11:52:12 PDT
*** Bug 767727 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.