529387 - Crash [@ GetCSSComputedValue] with xmlhttprequest, and "ABORT: actively animated document w/ no window"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase which crashes current trunk build on load.

http://crash-stats.mozilla.com/report/index/9b60501c-c3ff-4aa5-8835-886182091117
0	xul.dll	GetCSSComputedValue
1	xul.dll	nsSMILCSSProperty::GetBaseValue
2	xul.dll	nsSMILAnimationFunction::WillReplace
3	xul.dll	nsSMILCompositor::ComposeAttribute

Comment 1

[Daniel Holbert [:dholbert]]

When viewing this testcase in a debug build, I hit this NS_ABORT_IF_FALSE:
> ###!!! ABORT: actively animated document w/ no window: 'win', file ../../../mozilla/content/smil/nsSMILCSSProperty.cpp, line 77

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: (helper file for testcase 2)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 [doesn't actually reproduce bug]

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 414904 [details]
testcase 2 [doesn't actually reproduce bug]

Hrm -- so this testcase apparently doesn't reproduce the bug when hosted on Bugzilla, but it does on my local machine. (with the request modified to refer to a local copy of the "helper" file).

Anyway, the local copy of this testcase demonstrates that we fail, given a single <animate> node (targeting a CSS property) within an XMLHttpRequested SVG document.

Comment 5

[Daniel Holbert [:dholbert]]

So here's what basically happens in this bug:

We fire off an XMLHttpRequest for an SVG file that contains animations.  This file is loaded into a separate nsXMLDocument.  We start its animations when the request finishes loading. (via a call to "nsSVGSVGElement::PreHandleEvent" for the NS_SVG_LOAD event)

However, the problem is that this new nsXMLDocument lacks a nsPIDOMWindow -- not only at load-time, but for the rest of time after that as well. (at least in the attached testcases.)

So, we end up running animations in a nsXMLDocument that has no nsPIDOMWindow.  This is bad because "GetCSSComputedValue" function needs (and assumes we have) a nsPIDOMWindow in order to look up base values of CSS properties.

We could of course avoid the crash by bailing out of GetCSSComputedValue if we lack a nsPIDOMWindow....  However, I tend to think we shouldn't be running SVG animations in XMLHttpRequested documents in the first place.

Comment 6

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

(In reply to comment #5)
> However, the problem is that this new nsXMLDocument lacks a nsPIDOMWindow --
> not only at load-time, but for the rest of time after that as well. (at least
> in the attached testcases.)
Data documents don't have window. (Well, clone-doc-for-printing will be an exception.) Documents loaded using XHR are data documents.

> However, I tend to think we shouldn't be running SVG
> animations in XMLHttpRequested documents in the first place.
Indeed. I thought animations were somehow bound to presentation, but apparently
they aren't. (Data documents don't have presentation, excluding clone-doc-for-printing).

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to comment #5)
> However, I tend to think we shouldn't be running SVG
> animations in XMLHttpRequested documents in the first place.

Agreed.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Ok -- this patch makes us refuse to create an Animation Controller for data documents. (nsDocuments with mLoadedAsData == true)

Comment 10

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

Bug 487667 has similar fix for SMIL handling.

Comment 11

[Jonathan Watt [:jwatt]]

Daniel, we should have defined behavior for animation related objects/interfaces in data documents if we're not going to crash. ;-) Can you change the test to something that checks the animated value half way through the animation and check that it has the same value as the base value. Maybe also check that getCurrentTime returns zero at that point too.

Olli, do you mind if we land this separately with tests? Seems like it shouldn't be too hard to merge and will give more useful hg blame.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: mochitest (as patch)

Here's a mochitest to assert that animations are ineffective in a XMLHttpRequested document, as suggested by jwatt.

Comment 13

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

(In reply to comment #11)
> Olli, do you mind if we land this separately with tests? Seems like it
> shouldn't be too hard to merge and will give more useful hg blame.
This can land separately. I'll just remove one part of the fix for Bug 487667.

Comment 14

[Bob Clary [:bc] (inactive)]

http://codinginparadise.org/projects/svgweb/samples/demo.html?name=svgopen&svg.render.forceflash=true reproduces this on Mac at least.

Comment 15

[Daniel Holbert [:dholbert]]

(I have this in a queue of patches that are all waiting to land once m-c reopens)

Comment 16

[Daniel Holbert [:dholbert]]

Landed fix with crashtest:
http://hg.mozilla.org/mozilla-central/rev/a7b3540b0365
Mochitest in separate changeset:
http://hg.mozilla.org/mozilla-central/rev/6b63d9d09d37