WebKit Bugzilla
Attachment 343596 Details for
Bug 186189
: Crash in WebAnimation::runPendingPlayTask
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
Patch
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 ]
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 186189
:
341746
|
341747
|
341748
|
341749
|
341750
|
341756
|
343591
| 343596