592477 - SVG crash with DEP failure, using xbl, <use>, and <set>

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Reed Loden [:reed]]

Attachment: PoC

wushi of team509 reported the following potential vulnerability to security@:

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

Hi, I think I found a ff4 beta4 bug, the stack like this:

(a64.15c): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=00000000 ebx=01216980 ecx=05d538c0 edx=00620040 esi=05d539f0 
edi=064cba90
eip=01216980 esp=0012d7d0 ebp=0012d830 iopl=0         nv up ei pl zr na 
pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             
efl=00010246
01216980 b827c81002      mov     eax,210C827h
*** ERROR: Symbol file could not be found.  Defaulted to export symbols 
for C:\Program Files\Mozilla Firefox 4.0 Beta 4\xul.dll -
ChildEBP RetAddr
WARNING: Frame IP not in any known module. Following frames may be wrong.
0012d830 106d74e5 0x1216980
*** ERROR: Symbol file could not be found.  Defaulted to export symbols 
for C:\Program Files\Mozilla Firefox 4.0 Beta 4\MOZCRT19.dll -
0012d850 78138499 xul!mozilla::layers::LayerManagerD3D9::Initialize+0xbb9
0012d928 1058d2b8 MOZCRT19!expand+0x799
00000000 00000000 
xul!mozilla::layers::LayerManagerOGL::BindAndDrawQuad+0x58d0


Maybe you need refresh some times.

wushi

Comment 1

[Reed Loden [:reed]]

bp-33341931-f881-4ee1-a860-042f82100831

Mozilla/5.0 (X11; Linux i686; rv:2.0b5pre) Gecko/20100831 Firefox/4.0b5pre

Comment 2

[Martijn Wargers (dead)]

Xbl bindings don't work anymore in current trunk builds, so I'm wondering how you were able to crash.

Comment 3

[Daniel Holbert [:dholbert]]

Confirming, in latest nightly build - just crashed twice there, on Ubuntu Linux 10.04.
Mozilla/5.0 (X11; Linux i686; rv:2.0b5pre) Gecko/20100831 Firefox/4.0b5pre

Crash report IDs:
bp-9f9cec8c-18a3-430a-8480-6feec2100831
bp-954e1064-c640-45e5-a2dd-1d5e42100831

These reports have the same stack, both crashing in SMIL code.

Both crashes were accompanied by this output:
> pure virtual method called
> terminate called without an active exception

Comment 4

[Daniel Holbert [:dholbert]]

Unfortunately, I can't reproduce in my debug build, after many reloads.  :(  I just get a blank page -- no odd debug output or shutdown-crashes or anything.

Comment 5

[Martijn Wargers (dead)]

I doesn't crash for me, and I wouldn't expect it to crash, either, since xbl doesn't work in current trunk builds.

Comment 6

[Jonathan Watt [:jwatt]]

This does not crash beta 4 for me on Mac. Is this only happening on Linux?

Comment 7

[Brandon Sterne (:bsterne)]

(In reply to comment #6)

Apparently not.  wushi's report came from Windows on which I was unable to 
reproduce using XP SP3 and the latest nightly.

Comment 8

[wushi]

 Maybe you need to disable all cache selection in "about:config", I change some settings in "about:config" like IPC, "crash resume",cache.

Comment 9

[Daniel Holbert [:dholbert]]

I'm working on reducing the testcase.  So far, I've determined that the crash depends on the fact that the <set> animations are targeted with xlink:href. If I move them to be child elements of their animation-target (and remove their xlink:href attr), then the crash goes away.

Given that, I'm guessing this has something to do with us losing track of xlink:href animation targets, or getting confused about what element corresponds to our target-ID.

Comment 10

[Daniel Holbert [:dholbert]]

In particular, xlink:href-targeted elements whose xlink:href targets are in content that's cloned with <use>... I could see that causing issues.

Comment 11

[Brian Birtles (:birtles)]

Confirming that I can see this on a debug trunk build on Windows XP Pro. After reloading this about 20 times I got a message "Data Execution Prevention" and windows closed the program.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (somewhat reduced)

Here's a somewhat-simplified & cleaned up version of the testcase.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to comment #11)
> Confirming that I can see this on a debug trunk build on Windows XP Pro.

Ah, excellent! I still haven't been able to hit this in a debug build (after reloading hundreds of times).  Since you have, would you mind taking a crack at this?  (assuming that windows lets you examine the stack if you have a debugger connected)

Comment 14

[Brian Birtles (:birtles)]

(In reply to comment #13)
> Ah, excellent! I still haven't been able to hit this in a debug build (after
> reloading hundreds of times).  Since you have, would you mind taking a crack at
> this?  (assuming that windows lets you examine the stack if you have a debugger
> connected)

Unfortunately I've only been able to reproduce it once and not with a debugger attached. Without the debugger I have no stack trace or anything--Windows just forcibly terminated the process. :(

Reloaded about 100 times or more since but can't reproduce. I'll keep digging.

Comment 15

[Daniel Holbert [:dholbert]]

Ah, darn -- I guess it's not as easy as I thought it'd be, then. :)

FWIW, the fact that we get blocked by Data Execution Prevention (per comment 11) pretty much confirms that this is sg:critical.  /me removes question mark from [sg:critical] in whiteboard.

Comment 16

[Daniel Holbert [:dholbert]]

So based on the warning & crash reports from Comment 3, here's what we can establish:
 - We crash at  nsSVGAnimationElement.cpp:137, which is:

> 135 nsSVGAnimationElement::HasAnimAttr(nsIAtom* aAttName) const
> 136 {
> 137   return HasAttr(kNameSpaceID_None, aAttName);

 - That call to HasAttr is a virtual method call, defined in nsIContent.
 - The fact that we get a "pure virtual method called" warning on linux makes me worry that the vtable for our nsSVGAnimationElement been partly overwritten, since we apparently can't look up the inherited implementation for that method. (we should be using the version on nsGenericElement, I think -- though I haven't looked too thoroughly.)

Comment 17

[Daniel Holbert [:dholbert]]

(this is probably a job for Valgrind)

Comment 18

[Brian Birtles (:birtles)]

(In reply to comment #16)
>  - The fact that we get a "pure virtual method called" warning on linux makes
> me worry that the vtable for our nsSVGAnimationElement been partly overwritten,
> since we apparently can't look up the inherited implementation for that method.
> (we should be using the version on nsGenericElement, I think -- though I
> haven't looked too thoroughly.)

Yeah, I'm a bit stumped. I was wondering if somehow the sequence of calls in nsSVGAnimationElement::BindToTree/UnbindFromTree leaves us with dangling pointers in the compositor table when we go to compose. e.g. is it possible that for some combination of XBL and <use> the call to GetOwnerDoc in UnbindFromTree might return nsnull and leave us with a dangling pointer in the animation controller? Which we only discover once we traverse that pointer during the next composite?

I think I need to look into how <use> works to understand this.

(In reply to comment #17)
> (this is probably a job for Valgrind)

Yeah, I still can't reproduce this since the first occasion. I now have it running in the background reloading itself every 2 seconds but that doesn't seem to help. It's probably arrived in some sort of stable state.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (much more reduced)

Comment 20

[Brian Birtles (:birtles)]

Ok, just documenting what I've got so far since I have to go soon and probably won't get to look at this again for two days.

I managed to reproduce this with the debugger attached. I get a similar stack trace but the debugger breaks a little earlier at the call from nsSMILAnimationFunction::ComposeResult to GetValues which is a virtual function on the same class. So again, it seems we run in to trouble with the vtable.

Specifically the error I get is: 

Unhandled exception at 0x02b41f04 (gklayout.dll) in firefox.exe: 0xC0000005: Access violation reading location 0x0000001c.

So it's occuring in nsSMILAnimationFunction which has a null __vfptr. Also, it is the second animation function targetting the fill attribute.

After adding some debugging statements this is what I can see:

AF(0x62504f0): Created
AF(0x62504f0): Setting mAnimationElement to 0x6250400
AC(0x77d1bc0): Registering elem 0x6250400 with function 0x62504f0
AF(0x76502b8): Created
AF(0x76502b8): Setting mAnimationElement to 0x76501c8
AC(0x77d1bc0): Registering elem 0x76501c8 with function 0x76502b8
AF(0x625b290): Created
AF(0x625b290): Setting mAnimationElement to 0x625b1a0
AC(0x77d1bc0): Registering elem 0x625b1a0 with function 0x625b290
AC(0x77d1bc0): Unregistering elem 0x76501c8
    which has function 0x76502b8
AC(0x77d1bc0): Unregistering elem 0x76501c8
    which has function 0x76502b8

All of this takes place before the crash.
    
In this case the problematic animation function is at 0x76502b8 which appears to be a dangling pointer.

The curious thing is that the element that owns the nsSMILAnimationFunction has been unregistered (twice actually, but that's because UnbindFromTree seems to get called multiple times).

I wonder if it's possible that those calls to unregister the animation element are occuring somewhere in between creating the current compositor table and iterating over it to compose the result. For example, perhaps in clearing animation effects we drop the last reference to some parent element (perhaps via some nsSMILValue that goes out of scope? Not sure how CSS nsSMILValues work) which triggers the child animation to be unbound?

For my reference, I seem to be able to reproduce this only just after starting the browser.
So basically:
1) Start browser
2) Attach debugger
3) Load minimised test case as quickly as possible and refresh

If it doesn't reproduce the problem after about 20 reloads it probably never will (my record for manual reloads so far is 400) so it's better to restart the browser and try again.

Comment 21

[Brian Birtles (:birtles)]

My previous theory proves wrong. Adding some more debugging I get this:

Composing result...
AF(0x78d5ad8): Created
AF(0x78d5ad8): Setting mAnimationElement to 0x78d59e8
AC(0x6915c58): Registering elem 0x78d59e8 with function 0x78d5ad8
AC(0x6915c58): Unregistering elem 0x7631bc0
    which has function 0x7631cb0
AC(0x6915c58): Unregistering elem 0x7631bc0
    which has function 0x7631cb0
Creating compositor table...
Compositor table created...
Transferring cached values...
Removing matching compositors...
Clearing animation effects...
Composing result...

The problem animation function is question is that at 0x7631cb0 so the question is why it ended up in the compositor table since it's owning element was unregistered. Unless the functions are somehow being shared in this case? Or the unregistering isn't working as expected? I have no idea. I'll have a look on Friday unless you get to it before then Daniel.

Comment 22

[Brian Birtles (:birtles)]

Since adding some further debugging output I can't reproduce the problem :( So I propose we ship with my debugging output spewing to the console :)

What I have been getting though are the following two issues.

1) The following assertions firing
###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file c:/moz/src/cont
ent/events/src/nsEventDispatcher.cpp, line 514
###!!! ASSERTION: Want to fire mutation events, but it's not safe: '(aNode->IsNo
deOfType(nsINode::eCONTENT) && static_cast<nsIContent*>(aNode)-> IsInNativeAnony
mousSubtree()) || sScriptBlockerCount == sRemovableScriptBlockerCount', file c:/
moz/src/content/base/src/nsContentUtils.cpp, line 3657

2) Memory corruption when cleaning up CSS data structures. I've got stack traces similar to the following a couple of times:

>	mozalloc.dll!mozalloc_abort(const char * const msg=0x0013ca80)  Line 77	C++
 	xpcom_core.dll!Abort(const char * aMsg=0x0013ca80)  Line 379 + 0xa bytes	C++
 	xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=3, const char * aStr=0x02e81d4c, const char * aExpr=0x02e81d34, const char * aFile=0x02e81d08, int aLine=314)  Line 337 + 0xc bytes	C++
 	gklayout.dll!nsCSSCompressedDataBlock::~nsCSSCompressedDataBlock()  Line 314 + 0x24 bytes	C++
 	gklayout.dll!nsCSSCompressedDataBlock::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsAutoPtr<nsCSSCompressedDataBlock>::~nsAutoPtr<nsCSSCompressedDataBlock>()  Line 104 + 0x1e bytes	C++
 	gklayout.dll!mozilla::css::Declaration::~Declaration()  Line 76 + 0xb bytes	C++
 	gklayout.dll!mozilla::css::Declaration::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!CSSStyleRuleImpl::~CSSStyleRuleImpl()  Line 1398 + 0x1f bytes	C++
 	gklayout.dll!CSSStyleRuleImpl::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsCSSRule::Release()  Line 62 + 0xd2 bytes	C++
 	gklayout.dll!CSSStyleRuleImpl::Release()  Line 1415 + 0xd bytes	C++
 	gklayout.dll!nsRuleNode::~nsRuleNode()  Line 1251 + 0x1d bytes	C++
 	gklayout.dll!nsRuleNode::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsRuleNode::DestroyInternal(nsRuleNode * * * aDestroyQueueTail=0x0013cfa0)  Line 1209	C++
 	gklayout.dll!nsRuleNode::DestroyInternal(nsRuleNode * * * aDestroyQueueTail=0x00000000)  Line 1201	C++
 	gklayout.dll!nsRuleNode::Destroy()  Line 437 + 0x11 bytes	C++
 	gklayout.dll!nsStyleSet::Shutdown(nsPresContext * aPresContext=0x07574e38)  Line 1068	C++
 	gklayout.dll!PresShell::Destroy()  Line 1981	C++
 	gklayout.dll!DocumentViewerImpl::DestroyPresShell()  Line 4299	C++
 	gklayout.dll!DocumentViewerImpl::Destroy()  Line 1624	C++
 	gklayout.dll!DocumentViewerImpl::Show()  Line 1933	C++
 	gklayout.dll!nsPresContext::EnsureVisible()  Line 1657	C++
 	gklayout.dll!PresShell::UnsuppressAndInvalidate()  Line 4531 + 0xb bytes	C++
 	gklayout.dll!PresShell::UnsuppressPainting()  Line 4581	C++
 	gklayout.dll!DocumentViewerImpl::LoadComplete(unsigned int aStatus=0)  Line 1077	C++
 	docshell.dll!nsDocShell::EndPageLoad(nsIWebProgress * aProgress=0x068248ec, nsIChannel * aChannel=0x0bb58168, unsigned int aStatus=0)  Line 5965	C++
 	docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x068248ec, nsIRequest * aRequest=0x0bb58168, unsigned int aStateFlags=131088, unsigned int aStatus=0)  Line 5825	C++
 	docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x068248ec, nsIRequest * aRequest=0x0bb58168, int aStateFlags=131088, unsigned int aStatus=0)  Line 1335	C++
 	docshell.dll!nsDocLoader::doStopDocumentLoad(nsIRequest * request=0x0bb58168, unsigned int aStatus=0)  Line 953	C++
 	docshell.dll!nsDocLoader::DocLoaderIsEmpty(int aFlushLayout=1)  Line 820	C++
 	docshell.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x0bc02e38, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0)  Line 705	C++
 	necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x0bc02e38, nsISupports * ctxt=0x00000000, unsigned int aStatus=0)  Line 680 + 0x2e bytes	C++
 	gklayout.dll!nsDocument::DoUnblockOnload()  Line 7196	C++
 	gklayout.dll!nsDocument::UnblockOnload(int aFireSync=1)  Line 7137	C++
 	gklayout.dll!nsBindingManager::DoProcessAttachedQueue()  Line 1000	C++
 	gklayout.dll!nsRunnableMethodImpl<void (__thiscall nsBindingManager::*)(void),1>::Run()  Line 348	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0013db10)  Line 547 + 0x19 bytes	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x0003a890, int mayWait=1)  Line 250 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 178 + 0xc bytes	C++
 	gkwidget.dll!nsAppShell::Run()  Line 243 + 0x9 bytes	C++
 	toolkitcomps.dll!nsAppStartup::Run()  Line 191 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=4, char * * argv=0x00ed11c8, const nsXREAppData * aAppData=0x00ed1808)  Line 3662 + 0x25 bytes	C++
 	firefox.exe!NS_internal_main(int argc=4, char * * argv=0x00ed11c8)  Line 158 + 0x12 bytes	C++
 	firefox.exe!wmain(int argc=4, wchar_t * * argv=0x0003a5a8)  Line 120 + 0xd bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 583 + 0x19 bytes	C
 	firefox.exe!wmainCRTStartup()  Line 403	C
 	kernel32.dll!7c817077() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	mozjs.dll!js::TraceRecorder::monitorRecording(JSOp op=-1804729854)  Line 428 + 0x8 bytes	C++
 	946d0202()	

Right now I'm pretty stuck since I haven't been able to reproduce the original issue for some time now.

Comment 23

[Brian Birtles (:birtles)]

Wouldn't you know it, just as I'm about to step out the door I manage to reproduce this once again. I wrapped the call to compose attribute in some trace statements and here's the result:

AC(0x7664328): Composing result...
AC(0x7664328): ... finished composing result.
AC(0x7664328): Creating compositor table...
Building compositor table: for elem, 0x765ce50 adding func 0x765cf40
Building compositor table: for elem, 0x764a8a0 adding func 0x764a990
AC(0x7664328): Composing result...
AC(0x7664328): ... finished composing result.
AC(0x7664328): Creating compositor table...
Building compositor table: for elem, 0x765ce50 adding func 0x765cf40
Building compositor table: for elem, 0x764a8a0 adding func 0x764a990
AC(0x7664328): Composing result...
AF(0x76bb890): Created
AF(0x76bb890): Setting mAnimationElement to 0x76bb7a0
AC(0x7664328): Registering elem 0x76bb7a0 with function 0x76bb890
AC(0x7664328): Unregistering elem 0x764a8a0
    which has function 0x764a990
AC(0x7664328): Unregistering elem 0x764a8a0
    which has function 0x764a990
AC(0x7664328): Creating compositor table...
Building compositor table: for elem, 0x76bb7a0 adding func 0x76bb890
Building compositor table: for elem, 0x765ce50 adding func 0x765cf40
AC(0x7664328): Composing result...
AC(0x7664328): ... finished composing result.

For reference, when we break inside the animation function, the this pointer is 0x0764a990, i.e. the animation function that was previously unregistered. __vfptr is 0x000000ca suggesting this object has been destroyed by this point.
(Also AC=nsSMILAnimationFunction; AF=nsSMILAnimationFunction)

The particularly interesting lines are these:
AC(0x7664328): Composing result...
AF(0x76bb890): Created

That is, whilst in the compositing step we actually create a new animation function! Then we register it, unregister another animation function, run a sample and then pop the stack back and finish the original compositing step with the now-no-longer valid animation function (as it has been unregistered and destroyed).

Is it possible that script is running within our compositing step?

Comment 24

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

Not sure, but it seems like the thing to do is to add debug instrumentation so we assert when adding a new animation function during compositing. Then get a stack trace when we hit that assert.

Comment 25

[Brian Birtles (:birtles)]

The problem appears to be that calls to nsSMILCSSProperty::GetBaseValue() during compositing can cause anonymous content to be created and destroyed.

Following roc's suggestion in comment 24 I can see the following kinds of stack traces:

>	gklayout.dll!nsSVGAnimationElement::BindToTree(nsIDocument * aDocument=0x07d82028, nsIContent * aParent=0x07754a00, nsIContent * aBindingParent=0x07c64a30, int aCompileEventHandlers=1)  Line 293	C++
 	gklayout.dll!nsGenericElement::BindToTree(nsIDocument * aDocument=0x07d82028, nsIContent * aParent=0x07c64a30, nsIContent * aBindingParent=0x07c64a30, int aCompileEventHandlers=1)  Line 2966 + 0x20 bytes	C++
 	gklayout.dll!nsStyledElement::BindToTree(nsIDocument * aDocument=0x07d82028, nsIContent * aParent=0x07c64a30, nsIContent * aBindingParent=0x07c64a30, int aCompileEventHandlers=1)  Line 211 + 0x18 bytes	C++
 	gklayout.dll!nsSVGElement::BindToTree(nsIDocument * aDocument=0x07d82028, nsIContent * aParent=0x07c64a30, nsIContent * aBindingParent=0x07c64a30, int aCompileEventHandlers=1)  Line 225 + 0x1b bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::GetAnonymousContent(nsIContent * aParent=0x07c64a30, nsIFrame * aParentFrame=0x07cbb288, nsTArray<nsIContent *> & aContent={...})  Line 3975 + 0x20 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x07c64a30, nsStyleContext * aStyleContext=0x07cbafd8, nsIFrame * aFrame=0x07cbb288, const int aCanHaveGeneratedContent=0, nsFrameItems & aFrameItems={...}, const int aAllowBlockStyles=0, PendingBinding * aPendingBinding=0x00000000)  Line 9527	C++
 	gklayout.dll!nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem & aItem={...}, nsFrameConstructorState & aState={...}, nsIFrame * aParentFrame=0x07cbaf50, nsFrameItems & aFrameItems={...})  Line 3837 + 0x43 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState & aState={...}, nsCSSFrameConstructor::FrameConstructionItemList::Iterator & aIter={...}, nsIFrame * aParentFrame=0x07cbaf50, nsFrameItems & aFrameItems={...})  Line 5488 + 0x18 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState & aState={...}, nsCSSFrameConstructor::FrameConstructionItemList & aItems={...}, nsIFrame * aParentFrame=0x07cbaf50, nsFrameItems & aFrameItems={...})  Line 9466 + 0x18 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ContentRangeInserted(nsIContent * aContainer=0x079704a0, nsIContent * aStartChild=0x07c64a30, nsIContent * aEndChild=0x0813be20, nsILayoutHistoryState * aFrameState=0x0677c0e0, int aAllowLazyConstruction=0)  Line 7145	C++
 	gklayout.dll!nsCSSFrameConstructor::ContentInserted(nsIContent * aContainer=0x079704a0, nsIContent * aChild=0x07c64a30, nsILayoutHistoryState * aFrameState=0x0677c0e0, int aAllowLazyConstruction=0)  Line 6784	C++
 	gklayout.dll!nsCSSFrameConstructor::RecreateFramesForContent(nsIContent * aContent=0x07c64a30, int aAsyncInsert=0)  Line 9085 + 0x23 bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList & aChangeList={...})  Line 7954	C++
 	gklayout.dll!mozilla::css::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element * aElement=0x07c64a30, nsRestyleHint aRestyleHint=0, nsChangeHint aChangeHint=nsChangeHint_ReconstructFrame)  Line 164	C++
 	gklayout.dll!mozilla::css::RestyleTracker::ProcessRestyles()  Line 241	C++
 	gklayout.dll!nsCSSFrameConstructor::ProcessPendingRestyles()  Line 11607	C++
 	gklayout.dll!PresShell::FlushPendingNotifications(mozFlushType aType=Flush_Style)  Line 4782	C++
 	gklayout.dll!nsDocument::FlushPendingNotifications(mozFlushType aType=Flush_Style)  Line 6366	C++
 	gklayout.dll!nsComputedDOMStyle::GetPropertyCSSValue(const nsAString_internal & aPropertyName={...}, nsIDOMCSSValue * * aReturn=0x0013d54c)  Line 479	C++
 	gklayout.dll!nsComputedDOMStyle::GetPropertyValue(const nsAString_internal & aPropertyName={...}, nsAString_internal & aReturn={...})  Line 310 + 0x2a bytes	C++
 	gklayout.dll!nsComputedDOMStyle::GetPropertyValue(nsCSSProperty aPropID=eCSSProperty_fill, nsAString_internal & aValue={...})  Line 255 + 0x2b bytes	C++
 	gklayout.dll!GetCSSComputedValue(nsIContent * aElem=0x07578ad0, nsCSSProperty aPropID=eCSSProperty_fill, nsAString_internal & aResult={...})  Line 82	C++
 	gklayout.dll!nsSMILCSSProperty::GetBaseValue()  Line 138 + 0x20 bytes	C++
 	gklayout.dll!nsSMILCompositor::ComposeAttribute()  Line 112	C++
 	gklayout.dll!DoComposeAttribute(nsSMILCompositor * aCompositor=0x07e5a194, void * __formal=0x00000000)  Line 318	C++
 	gklayout.dll!nsTHashtable<nsSMILCompositor>::s_EnumStub(PLDHashTable * table=0x077a03a8, PLDHashEntryHdr * entry=0x07e5a194, unsigned int number=0, void * arg=0x0013d890)  Line 420 + 0x12 bytes	C++
 	xpcom_core.dll!PL_DHashTableEnumerate(PLDHashTable * table=0x077a03a8, PLDHashOperator (PLDHashTable *, PLDHashEntryHdr *, unsigned int, void *)* etor=0x0202d649, void * arg=0x0013d890)  Line 754 + 0x19 bytes	C
 	gklayout.dll!nsTHashtable<nsSMILCompositor>::EnumerateEntries(PLDHashOperator (nsSMILCompositor *, void *)* enumFunc=0x0203a448, void * userArg=0x00000000)  Line 241 + 0x13 bytes	C++
 	gklayout.dll!nsSMILAnimationController::DoSample(int aSkipUnchangedContainers=1)  Line 410	C++
 	gklayout.dll!nsSMILAnimationController::DoSample()  Line 325	C++
 	gklayout.dll!nsSMILTimeContainer::Sample()  Line 210	C++
 	gklayout.dll!nsSMILAnimationController::WillRefresh(mozilla::TimeStamp aTime={...})  Line 169	C++
 	gklayout.dll!nsRefreshDriver::Notify(nsITimer * __formal=0x0796d0b8)  Line 255	C++
 	xpcom_core.dll!nsTimerImpl::Fire()  Line 429	C++
 	xpcom_core.dll!nsTimerEvent::Run()  Line 519	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0013db10)  Line 547 + 0x19 bytes	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x0003a990, int mayWait=1)  Line 250 + 0x16 bytes	C++


And, more problematic is actually removing content:

>	gklayout.dll!nsSVGAnimationElement::UnbindFromTree(int aDeep=1, int aNullParent=0)  Line 334	C++
 	gklayout.dll!nsGenericElement::UnbindFromTree(int aDeep=1, int aNullParent=1)  Line 3041	C++
 	gklayout.dll!nsStyledElement::UnbindFromTree(int aDeep=1, int aNullParent=1)  Line 233	C++
 	gklayout.dll!AnonymousContentDestroyer::Run()  Line 4215	C++
 	gklayout.dll!nsContentUtils::RemoveScriptBlocker()  Line 4749	C++
 	gklayout.dll!nsAutoScriptBlocker::~nsAutoScriptBlocker()  Line 1890	C++
 	gklayout.dll!PresShell::FlushPendingNotifications(mozFlushType aType=Flush_Style)  Line 4787	C++
 	gklayout.dll!nsDocument::FlushPendingNotifications(mozFlushType aType=Flush_Style)  Line 6366	C++
 	gklayout.dll!nsComputedDOMStyle::GetPropertyCSSValue(const nsAString_internal & aPropertyName={...}, nsIDOMCSSValue * * aReturn=0x0013d54c)  Line 479	C++
 	gklayout.dll!nsComputedDOMStyle::GetPropertyValue(const nsAString_internal & aPropertyName={...}, nsAString_internal & aReturn={...})  Line 310 + 0x2a bytes	C++
 	gklayout.dll!nsComputedDOMStyle::GetPropertyValue(nsCSSProperty aPropID=eCSSProperty_fill, nsAString_internal & aValue={...})  Line 255 + 0x2b bytes	C++
 	gklayout.dll!GetCSSComputedValue(nsIContent * aElem=0x07498f00, nsCSSProperty aPropID=eCSSProperty_fill, nsAString_internal & aResult={...})  Line 82	C++
 	gklayout.dll!nsSMILCSSProperty::GetBaseValue()  Line 138 + 0x20 bytes	C++
 	gklayout.dll!nsSMILCompositor::ComposeAttribute()  Line 112	C++
 	gklayout.dll!DoComposeAttribute(nsSMILCompositor * aCompositor=0x0730d8e4, void * __formal=0x00000000)  Line 318	C++
 	gklayout.dll!nsTHashtable<nsSMILCompositor>::s_EnumStub(PLDHashTable * table=0x072d7b50, PLDHashEntryHdr * entry=0x0730d8e4, unsigned int number=0, void * arg=0x0013d890)  Line 420 + 0x12 bytes	C++
 	xpcom_core.dll!PL_DHashTableEnumerate(PLDHashTable * table=0x072d7b50, PLDHashOperator (PLDHashTable *, PLDHashEntryHdr *, unsigned int, void *)* etor=0x0202d649, void * arg=0x0013d890)  Line 754 + 0x19 bytes	C
 	gklayout.dll!nsTHashtable<nsSMILCompositor>::EnumerateEntries(PLDHashOperator (nsSMILCompositor *, void *)* enumFunc=0x0203a448, void * userArg=0x00000000)  Line 241 + 0x13 bytes	C++
 	gklayout.dll!nsSMILAnimationController::DoSample(int aSkipUnchangedContainers=1)  Line 410	C++
 	gklayout.dll!nsSMILAnimationController::DoSample()  Line 325	C++
 	gklayout.dll!nsSMILTimeContainer::Sample()  Line 210	C++
 	gklayout.dll!nsSMILAnimationController::WillRefresh(mozilla::TimeStamp aTime={...})  Line 169	C++
 	gklayout.dll!nsRefreshDriver::Notify(nsITimer * __formal=0x07472a18)  Line 255	C++
 	xpcom_core.dll!nsTimerImpl::Fire()  Line 429	C++
 	xpcom_core.dll!nsTimerEvent::Run()  Line 519	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0013db10)  Line 547 + 0x19 bytes	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x0003a990, int mayWait=1)  Line 250 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 178 + 0xc bytes	C++
 	gkwidget.dll!nsAppShell::Run()  Line 243 + 0x9 bytes	C++
 	toolkitcomps.dll!nsAppStartup::Run()  Line 191 + 0x1c bytes	C++

We have nsSMILAnimationFunctions which are not reference counted but owned by content (nsSVGAnimationElement's concrete subclasses). During a compositing step we take raw pointers to these functions and stick them in an array but this situation where content is removed during compositing means some of those pointers dangle.

I'm not quite sure how to go about solving this. Two possibilities are:

A) Flush style at the beginning of the sample. This seems to work but I'm not sure it's a good idea since animation is already an observer of style flushes. I'm not sure what the relationship is supposed to be between these two activities.

B) Alternatively, we can try to keep the content alive for the duration of the compositing step (e.g. create a temporary array of nsRefPtrs to the animation elements at the start of compositing and then drop it afterwards).

Comment 26

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

We do not want to be creating and destroying content during SMIL processing.

We need to ensure that styles are flushed before we run the SMIL sampling. And we must refrain from doing anything during SMIL processing that would trigger a style change.

I think we should probably flush style (not animations, just base style) before we run SMIL sampling, and do all the SMIL sampling stuff inside a scriptblocker to ensure that scripts and style flushes don't run. Then we'll be safe; if there are bugs, they'll be incorrect SMIL results rather than crashes.

Comment 27

[Brian Birtles (:birtles)]

Attachment: Proposed patch v1a

Here's a first pass at fixing this.

(In reply to comment #26)
> I think we should probably flush style (not animations, just base style) before
> we run SMIL sampling

I'm not sure I understand this. Calling nsDocument::FlushPendingNotifications(Flush_Style) will flush pending resample requests. Unless I should use Flush_Content instead?

> and do all the SMIL sampling stuff inside a scriptblocker
With a regular script blocker we run into assertions failing when we try to notify mutation observers. One example, is when animating a transform. When the animation is removed and we go to clear animation effects this notifies mutation observers.

Specifically, this test case (and many others): http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/smil/transform/rotate-angle-1.svg?raw=1

was hitting this assertion: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#3656

in response to this call: http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGTransformSMILAttr.cpp#106

I tried using mozAutoDocUpdate instead which fixes the assertions but causes several CSS animation tests to fail. For example, this test:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/smil/style/anim-css-color-1-by-ident-hex.svg?raw=1

I'm not sure how the CSS inheritance works, so I'm not quite sure why yet that fails.

So for now I have no script blocker in place but I've added some assertions to check we're not adding or removing animation elements during a sample.

Also, I'm not sure about the crashtest -- it relies on a 500ms setTimeout and that might not be suitable for our crashtest framework. I've yet to test if a 0ms timeout will still reproduce the problem.

Comment 28

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

Comment on attachment 472901 [details] [diff] [review]
Proposed patch v1a

I think NS_ASSERTION instead of ABORT_IF_FALSE, because it seems to me if we're firing mutation events during sampling those *can* happen, even though it's a bug.

Comment 29

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

(In reply to comment #27)
> Here's a first pass at fixing this.
> 
> (In reply to comment #26)
> > I think we should probably flush style (not animations, just base style)
> > before we run SMIL sampling
> 
> I'm not sure I understand this. Calling
> nsDocument::FlushPendingNotifications(Flush_Style) will flush pending resample
> requests. Unless I should use Flush_Content instead?

I think you solved this in a reasonable way.

> > and do all the SMIL sampling stuff inside a scriptblocker
> With a regular script blocker we run into assertions failing when we try to
> notify mutation observers. One example, is when animating a transform. When the
> animation is removed and we go to clear animation effects this notifies
> mutation observers.
> 
> Specifically, this test case (and many others):
> http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/smil/transform/rotate-angle-1.svg?raw=1
> 
> was hitting this assertion:
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#3656
> 
> in response to this call:
> http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGTransformSMILAttr.cpp#106
> 
> I tried using mozAutoDocUpdate instead which fixes the assertions but causes
> several CSS animation tests to fail. For example, this test:
> http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/smil/style/anim-css-color-1-by-ident-hex.svg?raw=1

I think we basically need to defer all the setting of values that could affect style computation until after we've finished sampling.

Having SMIL sampling both reading style data and performing DOM mutations that could affect style data (perhaps indirectly via mutation event handlers) (are these interleaved?) is a problem.

> Also, I'm not sure about the crashtest -- it relies on a 500ms setTimeout and
> that might not be suitable for our crashtest framework. I've yet to test if a
> 0ms timeout will still reproduce the problem.

Why is the timeout needed? If you just want to flush style, you can read some style-dependent value like getComputedStyle.

Comment 30

[Brian Birtles (:birtles)]

Attachment: Patch v1b, r=roc

Updated with review feedback. Split tests out to separate patch.

Also, fixed where the mRunningSample flag is reset since I was getting assertions failing in content/smil/crashtests/483584-2.svg

Running this through TryServer once more since I want to be sure the following random oranges really were random oranges:
11134 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_flush_on_paint.html | Test timed out.
Bug 569238 - Intermittent timeout in test_flush_on_paint.html
38174 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_mozPaintCount.html | Test timed out.
Bug 569237 - Intermittent timeout in test_mozPaintCount.html
39568 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_invalidate_during_plugin_paint.html | Test timed out.
Bug 580483 - Intermittent timeout in test_invalidate_during_plugin_paint.html
Bug 585438 - [SeaMonkey 2.1, mochitest-plain-4, Linux] test_invalidate_during_plugin_paint.html crashing
123752 ERROR TEST-UNEXPECTED-FAIL | /tests/modules/plugin/test/test_painting.html | Test timed out.
Bug 557456 - Intermittent test_painting.html | Test timed out.
Bug 557973 - Intermittent failure in test_painting.html | painted after invalidate - got 3, expected 2

Assuming TryServer is ok, I'll try to push this before beta6 freeze and then push the test case later.

Comment 31

[Brian Birtles (:birtles)]

Attachment: Test case patch, r=roc

Test case to be pushed separately, perhaps after beta 6 is shipped.

Comment 32

[Brian Birtles (:birtles)]

Pushed the fix: http://hg.mozilla.org/mozilla-central/rev/790ced23d60b

Yet to push the test case. Will wait until beta 6 is shipped for that unless someone suggests otherwise. Until then, I suppose it's ok to leave this bug open--not sure if that messes up our critsmash stats though?

Comment 34

[Brian Birtles (:birtles)]

Pushed test: http://hg.mozilla.org/mozilla-central/rev/6dcc21feccad