Attachment 343596 Details for Bug 186189 – Patch

[patch] Patch

bug-186189-20180626103318.patch (text/plain), 7.01 KB, created by Zan Dobersek on 2018-06-26 01:33:19 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Zan Dobersek

Created: 2018-06-26 01:33:19 PDT

Size: 7.01 KB

patch

obsolete

>Subversion Revision: 233190
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index e7623c788adec1e7390dbb9fc19824c187ceae92..3b625a916b7dfc729fc5ad9e89758aa3acfafbfb 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 NOBODY (OOPS!).
>+
>+        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-25  Keith Rollin  <krollin@apple.com>
> 
>         Adjust WEBCORE_EXPORT annotations for LTO
>diff --git a/Source/WebCore/animation/WebAnimation.cpp b/Source/WebCore/animation/WebAnimation.cpp
>index 9753c0183c094f3340524fb4222739838d1ccdcf..f56389a91c59b5ee8475042eaf630841c438c3c9 100644
>--- a/Source/WebCore/animation/WebAnimation.cpp
>+++ b/Source/WebCore/animation/WebAnimation.cpp
>@@ -835,7 +835,11 @@ void WebAnimation::runPendingPlayTask()
>     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 @@ void WebAnimation::runPendingPauseTask()
>     //    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);
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 0b77ba53bfdbe197010d3cc4d0d713c2bef53887..02e5e1c38793fbb51551fadc3487f80aca449a43 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 NOBODY (OOPS!).
>+
>+        * platform/wpe/TestExpectations: Remove crashing expectations for fixed tests.
>+
> 2018-06-25  David Fenton  <david_fenton@apple.com>
> 
>         LayoutTest imported/w3c/web-platform-tests/WebCryptoAPI/generateKey/successes_RSASSA-PKCS1-v1_5.https.any.worker.html is flaky.
>diff --git a/LayoutTests/platform/wpe/TestExpectations b/LayoutTests/platform/wpe/TestExpectations
>index 3132d0e707bb36cf6391c11f78e0888d40340efd..fdec7e3bb7fe8eb8842202dad9d5638c462d8924 100644
>--- a/LayoutTests/platform/wpe/TestExpectations
>+++ b/LayoutTests/platform/wpe/TestExpectations
>@@ -1224,12 +1224,6 @@ webkit.org/b/186752 fast/css-grid-layout/flex-sizing-rows-min-max-height.html [
> 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 ]

Actions: View | Formatted Diff | Diff

Attachments on bug 186189: 341746 | 341747 | 341748 | 341749 | 341750 | 341756 | 343591 | 343596