Closed
Bug 523188
Opened 15 years ago
Closed 15 years ago
CRASH on SMIL animate removal [@nsPresShellIterator::nsPresShellIterator(nsIDocument*) ]
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: marek.raida, Assigned: dholbert)
References
()
Details
(Keywords: crash, crashreportid, testcase, Whiteboard: [ccbr])
Crash Data
Attachments
(2 files, 2 obsolete files)
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•15 years ago
|
||
I see indeed a crash: http://crash-stats.mozilla.com/report/index/d3f00c04-8067-474d-b8b0-dab302091019?p=1
Severity: normal → critical
Summary: CRASH on SMIL animate removal → CRASH on SMIL animate removal [@nsPresShellIterator::nsPresShellIterator(nsIDocument*) ]
Version: unspecified → Trunk
Comment 2•15 years ago
|
||
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
Updated•15 years ago
|
Whiteboard: [ccbr]
Assignee | ||
Comment 3•15 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•15 years ago
|
||
Assignee | ||
Comment 5•15 years ago
|
||
Sorry, previous testcase was neutered. This one triggers the crash. Patch coming up next.
Attachment #407210 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #407210 -
Attachment description: testcase → non-testcase
Assignee | ||
Comment 6•15 years ago
|
||
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•15 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•15 years ago
|
||
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•15 years ago
|
Attachment #407237 -
Flags: review?(roc)
Attachment #407237 -
Flags: review?(birtles)
Attachment #407237 -
Flags: review?
Assignee | ||
Updated•15 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•15 years ago
|
OS: Windows XP → All
Hardware: x86 → All
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Attachment #407237 -
Flags: review?(roc) → review+
Assignee | ||
Comment 9•15 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).
Updated•15 years ago
|
Attachment #407237 -
Flags: review?(birtles) → review+
Assignee | ||
Comment 10•15 years ago
|
||
pushed: http://hg.mozilla.org/mozilla-central/rev/1ebdaad9b24a
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Crash Signature: [@nsPresShellIterator::nsPresShellIterator(nsIDocument*) ]
You need to log in
before you can comment on or make changes to this bug.
Description
•