Closed Bug 537157 Opened 10 years ago Closed 10 years ago

Crash [@ GetCSSComputedValue] with animate in svg in binding

Categories

(Core :: SVG, defect, critical)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: dholbert)

Details

(Keywords: assertion, crash, testcase)

Crash Data

Attachments

(2 files, 2 obsolete files)

Attached image 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
Confirmed.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091229 Minefield/3.7a1pre
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
OS: Windows 7 → All
Version: unspecified → Trunk
In a debug build:

###!!! ABORT: actively animated document w/ no window: 'win', file /Users/jruderman/central/content/smil/nsSMILCSSProperty.cpp, line 66
Keywords: assertion
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.
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...
Attached patch wip (obsolete) — Splinter Review
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.
Attached patch fix v1 (obsolete) — Splinter Review
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
Attachment #420159 - Attachment is obsolete: true
Attachment #420186 - Flags: superreview?(roc) → superreview+
Comment on attachment 420186 [details] [diff] [review]
fix v1

You should update the IID of nsPIDOMWindow
Attachment #420186 - Flags: review?(Olli.Pettay) → review+
Thanks -- IID fixed in this version. Carrying forward r+sr.
Attachment #420186 - Attachment is obsolete: true
Attachment #420346 - Flags: superreview+
Attachment #420346 - Flags: review+
Pushed with crashtest:
http://hg.mozilla.org/mozilla-central/rev/15c10445598c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Crash Signature: [@ GetCSSComputedValue]
You need to log in before you can comment on or make changes to this bug.