537157 - Crash [@ GetCSSComputedValue] with animate in svg in binding

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase.

http://crash-stats.mozilla.com/report/index/c749df82-a453-4c17-9f31-7e2402091229
0  	xul.dll  	GetCSSComputedValue  	 content/smil/nsSMILCSSProperty.cpp:68
1 	xul.dll 	nsSMILCSSProperty::GetBaseValue 	content/smil/nsSMILCSSProperty.cpp:107
2 	xul.dll 	nsSMILAnimationFunction::WillReplace 	content/smil/nsSMILAnimationFunction.cpp:347
3 	xul.dll 	nsSMILCompositor::ComposeAttribute 	content/smil/nsSMILCompositor.cpp:157

Comment 1

[Daniel Holbert [:dholbert]]

Confirmed.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091229 Minefield/3.7a1pre

Comment 2

[Jesse Ruderman]

In a debug build:

###!!! ABORT: actively animated document w/ no window: 'win', file /Users/jruderman/central/content/smil/nsSMILCSSProperty.cpp, line 66

Comment 3

[Daniel Holbert [:dholbert]]

So, there's apparently a "helper" nsXMLDocument that gets created for the XBL binding.

This isn't a data document, so when we call nsDocument::GetAnimationController on it, it happily (lazily) creates an animation controller, with which we can register animations.  But this helper document doesn't ever get a window, so we die as soon as we try to sample an animation of a CSS property. (because those currently assume they have a window, which they use for looking up their "base value" (the computed value)

We can avoid this by just adding a null-check for mWindow to the initial "immediate return" cases, at the beginning of nsDocument::GetAnimationController.

Comment 4

[Daniel Holbert [:dholbert]]

smaug

Comment 5

[Daniel Holbert [:dholbert]]

Rrgh - sorry, I hit 'tab+enter' or something while I'd just started still typing that previous comment. I was going to say:

smaug pointed out a few useful things in IRC:
 - We can check for this case in nsDocument::GetAnimationController by checking mLoadedAsInteractiveData  (though we need to move that flag up to nsDocument from nsXMLDocument, in order to do this)
 - We don't actually need the window pointer in the first place, to look up the computed value.  We can just look up the presShell & call NS_NewComputedDOMStyle. (which is all that the window's computed-style accessors do anyway)

Patch to accomplish the above coming up...

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: wip

This WIP patch accomplishes the second point from my previous comment (ditching the window pointer in favor of a null-checked PresShell & a call to NS_NewComputedDOMStyle).

This turns this bug's crash into warning-spam (with the warning added in this patch being spammed once per sample).  After I've got the mLoadedAsInteractiveData check, that will prevent us from animating at all, and that will fix the warning-spam.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This version adds the mLoadedAsInteractiveData check.

This also removes the "LookupComputedStyleFor" convenience method (and related code) from dom/base/ns*Window*.  This method was added in bug 474049, and its only caller is removed in this bug's patch. To remove it, I just directly reverted bug 474049's changes to ns*Window*, with these commands:
> hg diff -r da6d67c2c140  -r 0187a51241b7 dom/base/nsGlobalWindow.* | patch -p1 -R
> hg diff -r da6d67c2c140  -r 0187a51241b7 dom/base/nsPIDOMWindow.h | patch -p1 -R

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 420186 [details] [diff] [review]
fix v1

You should update the IID of nsPIDOMWindow

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix v1a (with IID update)

Thanks -- IID fixed in this version. Carrying forward r+sr.

Comment 10

[Daniel Holbert [:dholbert]]

Pushed with crashtest:
http://hg.mozilla.org/mozilla-central/rev/15c10445598c