Changeset 233196 in webkit

Timestamp:

Jun 26, 2018 5:30:19 AM (6 years ago)

Author:

zandobersek@gmail.com

Message:

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/wpe/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/WebAnimation.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/platform/wpe/TestExpectations

@@ -1225 +1225 @@
 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 ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/animation/WebAnimation.cpp

@@ -836 +836 @@
         // 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;
@@ -963 +967 @@
     //    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.