523188 - CRASH on SMIL animate removal [@nsPresShellIterator::nsPresShellIterator(nsIDocument*) ]

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Marek Raida]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091019 Minefield/3.7a1pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091019 Minefield/3.7a1pre (.NET CLR 3.5.30729)

When running SMIL animation element with floating point values used is removed from DOM, browser crashes (http://svg.kvalitne.cz/zx/imggallery2o.svg). Removing it with finished animation is fine (http://svg.kvalitne.cz/zx/imggallery2.svg)

Reproducible: Always

Steps to Reproduce:
1. create smil animation with floating point values
2. during run, try to remove it from DOM via scripting
Actual Results:  
crash

Expected Results:  
element removed and browser still running ;-)

Comment 1

[Ria Klaassen (not reading all bugmail)]

I see indeed a crash: http://crash-stats.mozilla.com/report/index/d3f00c04-8067-474d-b8b0-dab302091019?p=1

Comment 2

[Ria Klaassen (not reading all bugmail)]

0  	xul.dll  	nsPresShellIterator::nsPresShellIterator  	 obj-firefox/dist/include/nsPresShellIterator.h:50
1 	xul.dll 	nsGenericElement::SetSMILOverrideStyleRule 	content/base/src/nsGenericElement.cpp:2979
2 	xul.dll 	xul.dll@0x3db934 	
3 	xul.dll 	xul.dll@0x3dc204 	
4 	xul.dll 	nsDOMCSSDeclaration::SetPropertyValue 	layout/style/nsDOMCSSDeclaration.cpp:99
5 	xul.dll 	nsSMILCSSProperty::ClearAnimValue 	content/smil/nsSMILCSSProperty.cpp:190
6 	xul.dll 	nsSMILCompositor::ClearAnimationEffects 	content/smil/nsSMILCompositor.cpp:187
7 	xul.dll 	DoClearAnimationEffects 	content/smil/nsSMILAnimationController.cpp:281
8 	xul.dll 	nsTHashtable<nsSessionStorageEntry>::s_EnumStub 	obj-firefox/dist/include/nsTHashtable.h:420
9 	xul.dll 	PL_DHashTableEnumerate 	obj-firefox/xpcom/build/pldhash.c:754
10 	xul.dll 	nsTHashtable<nsCookieEntry>::EnumerateEntries 	obj-firefox/dist/include/nsTHashtable.h:241
11 	xul.dll 	nsSMILAnimationController::DoSample 	content/smil/nsSMILAnimationController.cpp:362
12 	xul.dll 	nsSMILAnimationController::DoSample 	content/smil/nsSMILAnimationController.cpp:296
13 	xul.dll 	nsSMILTimeContainer::Sample 	content/smil/nsSMILTimeContainer.cpp:152
14 	xul.dll 	nsSMILAnimationController::Notify 	content/smil/nsSMILAnimationController.cpp:234
15 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:427
16 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:519
17 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:527
18 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
19 	nspr4.dll 	PR_GetEnv 	
20 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:110
21 	firefox.exe 	__tmainCRTStartup 	obj-firefox/memory/jemalloc/crtsrc/crtexe.c:591
22 	kernel32.dll 	BaseThreadInitThunk 	
23 	ntdll.dll 	__RtlUserThreadStart 	
24 	ntdll.dll 	_RtlUserThreadStart

Comment 3

[Daniel Holbert [:dholbert]]

I'll look into this.  I think nsSMILCSSProperty::ClearAnimValue can just be a no-op if the target element isn't in the document anymore.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: non-testcase

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase (crashes nightly builds, if 'svg.smil.enabled' pref is on)

Sorry, previous testcase was neutered.  This one triggers the crash.  Patch coming up next.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: first-pass fix, with crashtest

Here's a first pass at a fix, with a crashtest.  It fixes the crash for me.

However, I think it leaves unwanted animation effects around in this scenario:
  - start with <rect><animate/></rect>
  - remove the rect node from the document
  - remove the animate node from the rect
  - reinsert the rect

We'd like the rect to be reinserted without any animation effects remaining on it, since it's no longer an animation target.  But this patch would leave the effects of the last animation sample on the rect.

So, I think we still want to call nsISMILAttr::ClearAnimValue, and it needs to work regardless of whether the target is in a document or not.  This probably just means removing the assumption that we have a document in nsGenericElement::SetSMILOverrideStyleRule.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> Created an attachment (id=407222) [details]
> first-pass fix, with crashtest
> 
> Here's a first pass at a fix, with a crashtest.  It fixes the crash for me.

D'oh -- turns out this only "fixed" the crash for me because I was using a fresh profile which had SMIL disabled. :) This patch needs a  "!" before the inserted "mKey.mElement->GetCurrentDoc()" to actually do any good.  (But it's still not a good solution, due to the issue brought up at the end of comment 6.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: better fix, with crashtest & reftests

Here's a fix that does what I suggested in comment 6 -- it removes the assumption in nsGenericElement::SetSMILOverrideStyleRule that an animation target is in a document (because it might not be, if we're clearing animation effects on it).

I've also added two reftests to check for the unwanted side effect scenario that I laid out in comment 6.

Here's a summary of how the tests do (with svg.smil.enabled forced-on in my local build now :))
* UNPATCHED:
  - content/smil/crashtests/523188-1.svg           -- crashes
  - layout/reftests/svg/smil/anim-remove-8css.svg  -- crashes
  - layout/reftests/svg/smil/anim-remove-8xml.svg  -- passes

* Patched with "first-pass fix" (plus the missing "!" per comment 7):
  - content/smil/crashtests/523188-1.svg           -- passes
  - layout/reftests/svg/smil/anim-remove-8css.svg  -- fails
  - layout/reftests/svg/smil/anim-remove-8xml.svg  -- fails

* Patched with "better fix":
  - content/smil/crashtests/523188-1.svg           -- passes
  - layout/reftests/svg/smil/anim-remove-8css.svg  -- passes
  - layout/reftests/svg/smil/anim-remove-8xml.svg  -- passes

I've pushed this to the try-server to double-check the unit tests on other platforms.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> I've pushed this to the try-server to double-check the unit tests on other
> platforms.

This patch has now passed try-servers on Windows/Mac/Linux (with an unrelated sporadic timeout on Windows, which I filed as Bug 523319).

Comment 10

[Daniel Holbert [:dholbert]]

pushed: http://hg.mozilla.org/mozilla-central/rev/1ebdaad9b24a