If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
SVG
--
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Marek Raida, Assigned: dholbert)

Tracking

({crash, crashreportid, testcase})

Trunk
crash, crashreportid, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ccbr], crash signature, URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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 ;-)
I see indeed a crash: http://crash-stats.mozilla.com/report/index/d3f00c04-8067-474d-b8b0-dab302091019?p=1
Severity: normal → critical
Keywords: crash, crashreportid, testcase
Summary: CRASH on SMIL animate removal → CRASH on SMIL animate removal [@nsPresShellIterator::nsPresShellIterator(nsIDocument*) ]
Version: unspecified → Trunk
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
Whiteboard: [ccbr]
(Assignee)

Comment 3

8 years ago
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.
Assignee: nobody → dholbert
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 4

8 years ago
Created attachment 407210 [details]
non-testcase
(Assignee)

Comment 5

8 years ago
Created attachment 407216 [details]
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.
Attachment #407210 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #407210 - Attachment description: testcase → non-testcase
(Assignee)

Comment 6

8 years ago
Created attachment 407222 [details] [diff] [review]
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.
(Assignee)

Comment 7

8 years ago
(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.)
(Assignee)

Comment 8

8 years ago
Created attachment 407237 [details] [diff] [review]
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.
Attachment #407222 - Attachment is obsolete: true
Attachment #407237 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #407237 - Flags: review?(roc)
Attachment #407237 - Flags: review?(birtles)
Attachment #407237 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #407216 - Attachment description: testcase (crashes Firefox when loaded, if 'svg.smil.enabled' pref is on) → testcase (crashes nightly builds, if 'svg.smil.enabled' pref is on)
(Assignee)

Updated

8 years ago
OS: Windows XP → All
Hardware: x86 → All
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
Attachment #407237 - Flags: review?(roc) → review+
(Assignee)

Comment 9

8 years ago
(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).
Attachment #407237 - Flags: review?(birtles) → review+
(Assignee)

Comment 10

8 years ago
pushed: http://hg.mozilla.org/mozilla-central/rev/1ebdaad9b24a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Crash Signature: [@nsPresShellIterator::nsPresShellIterator(nsIDocument*) ]
You need to log in before you can comment on or make changes to this bug.