When cancelling an animation, add missing timeline-detach check. (issue 1154423006 by sigbjornf@opera.com)

3 views
Skip to first unread message

sigb...@opera.com

unread,
May 31, 2015, 2:25:26 PM5/31/15
to lo...@chromium.org, dstoc...@chromium.org, blink-...@chromium.org, sh...@chromium.org, blink-revie...@chromium.org, ericwi...@chromium.org, rjwr...@chromium.org, alexis...@intel.com
Reviewers: loyso, dstockwell,

Message:
please take a look.

Description:
When cancelling an animation, add missing timeline-detach check.

Speculative fix for fuzzer-reported null derefence on cancel().

R=
BUG=491847

Please review this at https://codereview.chromium.org/1154423006/

Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+16, -1 lines):
A LayoutTests/web-animations-api/animation-timeline-detached-no-crash.html
M Source/core/animation/Animation.cpp


Index:
LayoutTests/web-animations-api/animation-timeline-detached-no-crash.html
diff --git
a/LayoutTests/web-animations-api/animation-timeline-detached-no-crash.html
b/LayoutTests/web-animations-api/animation-timeline-detached-no-crash.html
new file mode 100644
index
0000000000000000000000000000000000000000..46e569e3172c14ee2162d389330faade4be42d26
--- /dev/null
+++
b/LayoutTests/web-animations-api/animation-timeline-detached-no-crash.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+
+<script>
+test(function() {
+ var doc = document.implementation.createDocument("", "", null);
+ doc.createElement("div").animate([], 1000);
+ var anim = doc.timeline.getAnimations()[0];
+ doc = null;
+ gc();
+ anim.cancel();
+}, 'Calling cancel() on an animation detached from its timeline should not
crash.');
+</script>
Index: Source/core/animation/Animation.cpp
diff --git a/Source/core/animation/Animation.cpp
b/Source/core/animation/Animation.cpp
index
8e1e3d64816ef2671ff01bfd0e8e3761ac3d8b53..5103120d88fef661182dd039d0726bcd241fb484
100644
--- a/Source/core/animation/Animation.cpp
+++ b/Source/core/animation/Animation.cpp
@@ -836,7 +836,8 @@ void Animation::cancel()
m_startTime = nullValue();
m_currentTimePending = false;

- InspectorInstrumentation::didCancelAnimation(timeline()->document(),
this);
+ if (timeline())
+
InspectorInstrumentation::didCancelAnimation(timeline()->document(), this);
}

void Animation::beginUpdatingState()


lo...@chromium.org

unread,
May 31, 2015, 9:50:31 PM5/31/15
to sigb...@opera.com, dstoc...@chromium.org, blink-...@chromium.org, sh...@chromium.org, blink-revie...@chromium.org, ericwi...@chromium.org, rjwr...@chromium.org, alexis...@intel.com
On 2015/05/31 18:25:26, sof wrote:
> please take a look.
Non-owner Looks Good To Me.

https://codereview.chromium.org/1154423006/

dstoc...@chromium.org

unread,
Jun 1, 2015, 8:13:19 PM6/1/15
to sigb...@opera.com, lo...@chromium.org, alexis...@intel.com, blink-...@chromium.org, blink-revie...@chromium.org, ericwi...@chromium.org, rjwr...@chromium.org, sh...@chromium.org
lgtm



https://codereview.chromium.org/1154423006/

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 2, 2015, 1:38:36 AM6/2/15
to sigb...@opera.com, dstoc...@chromium.org, lo...@chromium.org, commi...@chromium.org, alexis...@intel.com, blink-...@chromium.org, blink-revie...@chromium.org, ericwi...@chromium.org, rjwr...@chromium.org, sh...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1154423006/20001


https://codereview.chromium.org/1154423006/

'commit-bot: I haz the power' via codereview.chromium.org

unread,
Jun 2, 2015, 4:32:41 AM6/2/15
to sigb...@opera.com, dstoc...@chromium.org, lo...@chromium.org, commi...@chromium.org, alexis...@intel.com, blink-...@chromium.org, blink-revie...@chromium.org, ericwi...@chromium.org, rjwr...@chromium.org, sh...@chromium.org
Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196299

https://codereview.chromium.org/1154423006/
Reply all
Reply to author
Forward
0 new messages