Crash in WebAnimation::runPendingPlayTask
https://bugs.webkit.org/show_bug.cgi?id=186189
Reviewed by Carlos Garcia Campos.
Source/WebCore:
Avoid crashes on nullopt std::optional dereference in the
runPendingPlayTask() and runPendingPauseTask() methods of the
WebAnimation class by defaulting to a Seconds(0) value.
In both cases the std::optional value is the current time retrieved from
the associated DocumentTimeline object. But there's no guarantee that
the timeline is active and the resulting time value is resolved (i.e.
not nullopt). Dereferencing the nullopt Seconds value doesn't cause a
problem on configurations still building as C++14 and the fallback
std::optional implementation provided by WTF -- no signal is raised, and
a 0 value is returned. Configurations building as C++17 on the other
hand use the stdlib-provided std::optional that does raise a signal on
invalid access, leading to crashes.
The default-to-Seconds(0) solution avoids crashes on configurations
that build with C++17 support enabled, and thus match configurations
that are still using WTF's std::optional. This still doesn't address the
underlying problem of retrieving current time from an inactive document
timeline and using it as ready time for the pending play/pause task
execution.
runPendingPlayTask() change addresses crashes in the following tests:
- fast/animation/css-animation-resuming-when-visible.html
- fast/animation/css-animation-resuming-when-visible-with-style-change.html
- imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html
- imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/getAnimations.html
runPendingPauseTask() change addresses crashes in the following tests:
- animations/multiple-animations-timing-function.html
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::runPendingPlayTask):
(WebCore::WebAnimation::runPendingPauseTask):
LayoutTests:
* platform/wpe/TestExpectations: Remove crashing expectations for fixed tests.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@233196 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index c53f6fc..ccb32d7 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,12 @@
+2018-06-26 Zan Dobersek <zdobersek@igalia.com>
+
+ Crash in WebAnimation::runPendingPlayTask
+ https://bugs.webkit.org/show_bug.cgi?id=186189
+
+ Reviewed by Carlos Garcia Campos.
+
+ * platform/wpe/TestExpectations: Remove crashing expectations for fixed tests.
+
2018-06-25 Youenn Fablet <youenn@apple.com>
Import WPT fetch destination tests
diff --git a/LayoutTests/platform/wpe/TestExpectations b/LayoutTests/platform/wpe/TestExpectations
index 3132d0e..fdec7e3 100644
--- a/LayoutTests/platform/wpe/TestExpectations
+++ b/LayoutTests/platform/wpe/TestExpectations
@@ -1224,12 +1224,6 @@
webkit.org/b/186752 fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html [ Crash ]
webkit.org/b/186752 fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html [ Crash ]
-webkit.org/b/186753 animations/multiple-animations-timing-function.html [ Crash ]
-webkit.org/b/186753 fast/animation/css-animation-resuming-when-visible-with-style-change.html [ Crash ]
-webkit.org/b/186753 fast/animation/css-animation-resuming-when-visible.html [ Crash ]
-webkit.org/b/186753 imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html [ Crash ]
-webkit.org/b/186753 imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/getAnimations.html [ Crash ]
-
webkit.org/b/186100 css3/color-filters/color-filter-color-property-list-item.html [ ImageOnlyFailure ]
webkit.org/b/186100 css3/color-filters/color-filter-ignore-semantic.html [ ImageOnlyFailure ]
webkit.org/b/186100 css3/color-filters/color-filter-opacity.html [ ImageOnlyFailure ]
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index abef484..114ba5e 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,44 @@
+2018-06-26 Zan Dobersek <zdobersek@igalia.com>
+
+ Crash in WebAnimation::runPendingPlayTask
+ https://bugs.webkit.org/show_bug.cgi?id=186189
+
+ Reviewed by Carlos Garcia Campos.
+
+ Avoid crashes on nullopt std::optional dereference in the
+ runPendingPlayTask() and runPendingPauseTask() methods of the
+ WebAnimation class by defaulting to a Seconds(0) value.
+
+ In both cases the std::optional value is the current time retrieved from
+ the associated DocumentTimeline object. But there's no guarantee that
+ the timeline is active and the resulting time value is resolved (i.e.
+ not nullopt). Dereferencing the nullopt Seconds value doesn't cause a
+ problem on configurations still building as C++14 and the fallback
+ std::optional implementation provided by WTF -- no signal is raised, and
+ a 0 value is returned. Configurations building as C++17 on the other
+ hand use the stdlib-provided std::optional that does raise a signal on
+ invalid access, leading to crashes.
+
+ The default-to-Seconds(0) solution avoids crashes on configurations
+ that build with C++17 support enabled, and thus match configurations
+ that are still using WTF's std::optional. This still doesn't address the
+ underlying problem of retrieving current time from an inactive document
+ timeline and using it as ready time for the pending play/pause task
+ execution.
+
+ runPendingPlayTask() change addresses crashes in the following tests:
+ - fast/animation/css-animation-resuming-when-visible.html
+ - fast/animation/css-animation-resuming-when-visible-with-style-change.html
+ - imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html
+ - imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/getAnimations.html
+
+ runPendingPauseTask() change addresses crashes in the following tests:
+ - animations/multiple-animations-timing-function.html
+
+ * animation/WebAnimation.cpp:
+ (WebCore::WebAnimation::runPendingPlayTask):
+ (WebCore::WebAnimation::runPendingPauseTask):
+
2018-06-26 Antoine Quint <graouts@apple.com>
[Web Animations] Show the feature as "Supported in Preview"
diff --git a/Source/WebCore/animation/WebAnimation.cpp b/Source/WebCore/animation/WebAnimation.cpp
index 9753c01..f56389a 100644
--- a/Source/WebCore/animation/WebAnimation.cpp
+++ b/Source/WebCore/animation/WebAnimation.cpp
@@ -835,7 +835,11 @@
if (!m_startTime) {
// 1. Let new start time be the result of evaluating ready time - hold time / animation playback rate for animation.
// If the animation playback rate is zero, let new start time be simply ready time.
- auto newStartTime = readyTime.value();
+ // FIXME: Implementation cannot guarantee an active timeline at the point of this async dispatch.
+ // Subsequently, the resulting readyTime value can be null. Unify behavior between C++17 and
+ // C++14 builds (the latter using WTF's std::optional) and avoid null std::optional dereferencing
+ // by defaulting to a Seconds(0) value. See https://bugs.webkit.org/show_bug.cgi?id=186189.
+ auto newStartTime = readyTime.value_or(0_s);
if (m_playbackRate)
newStartTime -= m_holdTime.value() / m_playbackRate;
// 2. If animation's playback rate is not 0, make animation's hold time unresolved.
@@ -962,8 +966,13 @@
// evaluating (ready time - start time) × playback rate.
// Note: The hold time might be already set if the animation is finished, or if the animation is pending, waiting to begin
// playback. In either case we want to preserve the hold time as we enter the paused state.
- if (animationStartTime && !m_holdTime)
- setHoldTime((readyTime.value() - animationStartTime.value()) * m_playbackRate);
+ if (animationStartTime && !m_holdTime) {
+ // FIXME: Implementation cannot guarantee an active timeline at the point of this async dispatch.
+ // Subsequently, the resulting readyTime value can be null. Unify behavior between C++17 and
+ // C++14 builds (the latter using WTF's std::optional) and avoid null std::optional dereferencing
+ // by defaulting to a Seconds(0) value. See https://bugs.webkit.org/show_bug.cgi?id=186189.
+ setHoldTime((readyTime.value_or(0_s) - animationStartTime.value()) * m_playbackRate);
+ }
// 3. Make animation's start time unresolved.
setStartTime(std::nullopt);
