606642 - "ASSERTION: StealFrame: can't find aChild" with moz-column, rel & abs pos [@ nsFrameList::DestroyFrom][@ nsFrameList::RemoveFrame(nsIFrame*)]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: StealFrame: can't find aChild: 'removed', file layout/generic/nsContainerFrame.cpp, line 1047

###!!! ASSERTION: StealFrame failure: 'NS_SUCCEEDED(rv)', file layout/generic/nsContainerFrame.cpp, line 1140

Crash [@ nsFrameList::RemoveFrame]

Comment 1

[timeless]

Signature	nsFrameList::DestroyFrom
UUID	89ec3020-0b3a-49cf-800a-4993e2101208
Time 	2010-12-08 18:12:14.978150
Uptime	52
Last Crash	9118872 seconds (more than 3 months) before submission
Install Age	204745 seconds (2.4 days) since version was first installed.
Product	Firefox
Version	4.0b8pre
Build ID	20101206030318
Branch	2.0
OS	Mac OS X
OS Version	10.6.5 10H574
CPU	amd64
CPU Info	family 6 model 23 stepping 6
Crash Reason	EXC_BAD_ACCESS / 0x0000000d
Crash Address	0x0
User Comments	bug 606642
App Notes 	Renderers: 0x22600,0x22600,0x20400
Processor Notes 	
EMCheckCompatibility	False
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	XUL 	nsFrameList::DestroyFrom 	layout/generic/nsIFrame.h:1032
1 	XUL 	nsContainerFrame::DestroyFrom 	layout/generic/nsContainerFrame.cpp:283
2 	XUL 	nsBlockFrame::DestroyFrom 	layout/generic/nsBlockFrame.cpp:342
3 	XUL 	nsContainerFrame::DeleteNextInFlowChild 	layout/generic/nsIFrame.h:542
4 	XUL 	nsContainerFrame::DeleteNextInFlowChild 	layout/generic/nsContainerFrame.cpp:1129
5 	XUL 	nsAbsoluteContainingBlock::RemoveFrame 	layout/generic/nsAbsoluteContainingBlock.cpp:125
6 	XUL 	nsBlockFrame::RemoveFrame 	layout/generic/nsBlockFrame.cpp:4990
7 	XUL 	nsFrameManager::RemoveFrame 	layout/base/nsFrameManager.cpp:512
8 	XUL 	nsPlaceholderFrame::DestroyFrom 	layout/generic/nsPlaceholderFrame.cpp:169
9 	XUL 	nsBlockFrame::DoRemoveFrame 	layout/generic/nsIFrame.h:542
10 	XUL 	nsBlockFrame::RemoveFrame 	layout/generic/nsBlockFrame.cpp:4984
11 	XUL 	nsFrameManager::RemoveFrame 	layout/base/nsFrameManager.cpp:512
12 	XUL 	nsCSSFrameConstructor::ContentRemoved 	layout/base/nsCSSFrameConstructor.cpp:7590
13 	XUL 	PresShell::ContentRemoved 	layout/base/nsPresShell.cpp:5132
14 	XUL 	nsNodeUtils::ContentRemoved 	content/base/src/nsNodeUtils.cpp:196
15 	XUL 	nsINode::doRemoveChildAt 	content/base/src/nsGenericElement.cpp:3691
16 	XUL 	nsGenericElement::RemoveChildAt 	content/base/src/nsGenericElement.cpp:3636
17 	XUL 	nsIDOMNode_RemoveChild 	nsINode.h:485
18 	XUL 	CallCompiler::generateNativeStub 	js/src/jscntxtinlines.h:684
19 	XUL 	js::mjit::ic::NativeCall 	js/src/methodjit/MonoIC.cpp:898
20 		@0x11a84ec68 	
21 	XUL 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:745
22 	XUL 	js::Invoke 	js/src/jsinterp.cpp:654
23 	XUL 	js::ExternalInvoke 	js/src/jsinterp.cpp:858
24 	XUL 	JS_CallFunctionValue 	js/src/jsinterp.h:962
25 	XUL 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:2177

Comment 2

[Alex Miller]

On Windows this crashes at a non-random non-null address.

mov     eax,dword ptr [eax+24h] ds:002b:f0de8023=????????

Comment 3

[Alex Miller]

Attachment: Debug logs / stack

It looks like a frame is being used after the pointer to it has been freed and overwritten, maybe security-sensitive?

Comment 4

[Jesse Ruderman]

That's the poison-frame address.  Worse than a null deref, but shouldn't be exploitable.

Comment 5

[Alex Miller]

(In reply to comment #4)
> That's the poison-frame address.  Worse than a null deref, but shouldn't be
> exploitable.

Oh. Well, still worth fixing.

Comment 6

[Daniel Veditz [:dveditz]]

see also bug 642088 with the same signature

Comment 7

[Mats Palmgren (inactive)]

Attachment: frame dump

Here's a few frame dumps as we destroy the frame tree.  The first one is
the original frame tree when ContentRemoved is called.
There's an "outer" overflow container (OC) chain, colored red/magenta/pink.
Inside it is another OC chain, light-blue/blue/cyan.
We start at the first placeholder and its out-of-flow (red), calling
nsAbsoluteContainingBlock::RemoveFrame which calls DeleteNextInFlowChild
on its nif (magenta).  DeleteNextInFlowChild destroys in reverse order
so the first frame we actually call Destroy for is the last nif (pink).

We're currently at line 241 destroying the OverflowContainersList here:
http://hg.mozilla.org/mozilla-central/annotate/469333ea459c/layout/generic/nsContainerFrame.cpp#l220
looping at line 60 here:
http://hg.mozilla.org/mozilla-central/annotate/469333ea459c/layout/generic/nsFrameList.cpp#l38

We destroy the first child, 0x7fffd3b68e70, then find the (lime)
placeholder leading to the inner OC chain - where pretty much the same
happens, it destroys the chain in reverse order: cyan/blue/light-blue.

But, StealFrame fails because it can't find the cyan frame in its parent
(pink).  Looking at nsContainerFrame::DestroyFrom again, the reason is
that we removed the frame property before starting to destroy the list.
(line 239)

In DeleteNextInFlowChild, we Destroy it anyway:
http://hg.mozilla.org/mozilla-central/annotate/469333ea459c/layout/generic/nsContainerFrame.cpp#l1372

So after cyan/blue/light-blue are destroyed we unwind to DestroyFrom for
(pink), continuing the nsFrameList loop destroying the next child after
the placeholder ... oops ... (cyan) was already destroyed!

============

My first attempt at fixing this was to simply leave the property
in place, so that StealFrame can find (cyan) and remove it from the
list.  The nsFrameList loop deals with that - it'll just get null from
the RemoveFirstChild() and stop.  Something like this:
  frameList = GetProp(...)
  if (frameList) frameList->DestroyFrom()
  delete RemoveProp(...)

But that doesn't work because when StealFrame removes (cyan) the list
becomes empty and thus the property is removed and the list deleted!

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix

https://tbpl.mozilla.org/?tree=Try&rev=a90f8968601b

Comment 9

[Mats Palmgren (inactive)]

> +namespace mozilla{

I'll add a space there.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 712031 [details] [diff] [review]
fix

Review of attachment 712031 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsContainerFrame.cpp
@@ +232,5 @@
> +        // property was deleted -- that's fine but 'frameList' is now dangling.
> +        return;
> +      }
> +      MOZ_ASSERT(propVal == frameList, "frame list property was replaced");
> +    }

Wouldn't it be simpler to just get the framelist property at the start of each iteration?

Comment 11

[Mats Palmgren (inactive)]

Attachment: fix, v2

Yeah that's simpler, thanks.

https://tbpl.mozilla.org/?tree=Try&rev=0f9c68ae7a78

Comment 12

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/167195a95601

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/167195a95601