[Webkit-unassigned] [Bug 186189] Crash in WebAnimation::runPendingPlayTask

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=186189

Zan Dobersek <zan at falconsigh.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |zan at falconsigh.net

--- Comment #24 from Zan Dobersek <zan at falconsigh.net> ---
(In reply to Carlos Garcia Campos from comment #13)
> (In reply to Antoine Quint from comment #10)
> > The change makes sense, I wonder why
> > fast/animation/css-animation-resuming-when-visible-with-style-change.html is
> > timing out on Mac now. It's been consistently timing out on GTK but has been
> > stable on other ports.
> 
> The problem, at least in GTK+, is that
> window.getComputedStyle(testDiv).transform doesn't change after resuming
> animations, even though I can see the animation happening. So,
> shouldBecomeEqual("internals.numberOfActiveAnimations()", "1",
> checkTransformAndFinishTest); never happens and the test doesn't finish.

The test doesn't time out in WPE, but I don't know the reason for that.

Crashes occur because of value retrieval on uninitialized std::optional<> objects that represent readyTime values. The Cocoa ports also operate on such uninitialized objects, but avoid crashes because the WTF's std::optional implementation intentionally doesn't trigger them. Instead, a zero value is used for computation, and that works as far as tests are concerned.

This access into uninitialized std::optional<> objects occurs in both runPendingPlayTask() and runPendingPauseTask() methods in WebAnimation. Causes are different.

With runPendingPlayTask(), in the tests where crashes are observed, issue occurs because the animations are suspended via DocumentTimeline::suspendAnimations() before the async pending play task is executed. Once suspended, the current time for that timeline is unresolved (i.e. std::nullopt). So the runPendingPlayTask() is called while the animations are suspended, nullopt readyTime value is retrieved, and things explode.

With runPendingPauseTask(), the crashes that are observed in tests are due to the timeline becoming inactive because the DocumentTimeline is detached from the owning Document instance. In WebAnimation::updatePendingTasks(), any pause task is posted to the Document. But the Document might soon after start being destroyed, and the DocumentTimeline gets detached in Document::prepareForDestruction(). This doesn't prevent task dispatch, and WebAnimation::runPendingPauseTask() can end up being dispatched with the DocumentTimeline in an inactive state. DocumentTimeline::currentTime() then ends up returning std::nullopt, and a crash occurs.

In both cases, WebAnimation ends up using DocumentTimeline as if it was guaranteed to be active, when in reality it isn't. It's not clear to me whether the current implementation correctly follows the specification when determining the 'ready time' value in both runPendingPlayTasks() and runPendingPauseTasks().

As a temporary solution, I would like the GTK and WPE ports to sync with the behavior of Cocoa ports by adjusting the readyTime value usage in runPendingPlayTask() and runPendingPauseTask() methods to fall back to a zero-Seconds value in case of an uninitialized std::optional<> instance. This would primarily avoid the crashes. Timeouts and spec compliance would be addressed later.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180625/94c0aea1/attachment.html>