Bug 187139 – [Web Animations] Crash in KeyframeEffectReadOnly::applyPendingAcceleratedActions()

• Component: Animations
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

This is yet another crash in animations code due to nullopt value being used without check. This is very easy to reproduce for me just visiting gitlab.gnome.org. The workaround as usual is easy:

-    auto timeOffset = animation()->currentTime().value().seconds();
+    auto timeOffset = animation()->currentTime().value_or(0_s).seconds();

but I guess that's not the right fix.

[Carlos Garcia Campos]

Thread 1 "WebKitWebProces" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51	../sysdeps/unix/sysv/linux/raise.c: No existe el fichero o el directorio.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007fe7def36231 in __GI_abort () at abort.c:79
#2  0x00007fe7ec014e31 in WebCore::KeyframeEffectReadOnly::applyPendingAcceleratedActions() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#3  0x00007fe7ec015505 in WebCore::DocumentTimeline::applyPendingAcceleratedAnimations() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#4  0x00007fe7ec015597 in WebCore::DocumentTimeline::performInvalidationTask() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#5  0x00007fe7ec73ef18 in WebCore::TaskDispatcher<WebCore::Timer>::dispatchOneTask() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#6  0x00007fe7ec73f029 in WebCore::TaskDispatcher<WebCore::Timer>::sharedTimerFired() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#7  0x00007fe7ec76e8ce in WebCore::ThreadTimers::sharedTimerFiredInternal() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#8  0x00007fe7e9530ec3 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::_FUN(void*) () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.0.so.18
#9  0x00007fe7e1920495 in g_main_dispatch (context=0x5633b8505e60) at gmain.c:3177
#10 g_main_context_dispatch (context=context@entry=0x5633b8505e60) at gmain.c:3830
#11 0x00007fe7e1920838 in g_main_context_iterate (context=0x5633b8505e60, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3903
#12 0x00007fe7e1920b42 in g_main_loop_run (loop=0x5633b8592160) at gmain.c:4099
#13 0x00007fe7e9531298 in WTF::RunLoop::run() () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.0.so.18
#14 0x00007fe7eba405c0 in WebProcessMainUnix () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#15 0x00007fe7def21a87 in __libc_start_main (main=0x5633b7862c30 <main>, argc=3, argv=0x7ffc5b441b78, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, 
    stack_end=0x7ffc5b441b68) at ../csu/libc-start.c:310
#16 0x00005633b7862cba in _start ()

[Frédéric Wang (:fredw)]

(In reply to Carlos Garcia Campos from comment #0)
> This is yet another crash in animations code due to nullopt value being used
> without check. This is very easy to reproduce for me just visiting
> gitlab.gnome.org. The workaround as usual is easy:
> 
> -    auto timeOffset = animation()->currentTime().value().seconds();
> +    auto timeOffset = animation()->currentTime().value_or(0_s).seconds();
> 
> but I guess that's not the right fix.

FWIW, !animation()->currentTime() also happens on macOS except that the program does not crash and seems to continue with a dummy value. Maybe that's because clang does not implement std::optional the correct way.

I wonder whether the function should instead just exist if the time value is unresolved?

[Carlos Garcia Campos]

(In reply to Frédéric Wang (:fredw) from comment #2)
> (In reply to Carlos Garcia Campos from comment #0)
> > This is yet another crash in animations code due to nullopt value being used
> > without check. This is very easy to reproduce for me just visiting
> > gitlab.gnome.org. The workaround as usual is easy:
> > 
> > -    auto timeOffset = animation()->currentTime().value().seconds();
> > +    auto timeOffset = animation()->currentTime().value_or(0_s).seconds();
> > 
> > but I guess that's not the right fix.
> 
> FWIW, !animation()->currentTime() also happens on macOS except that the
> program does not crash and seems to continue with a dummy value. Maybe
> that's because clang does not implement std::optional the correct way.
> 
> I wonder whether the function should instead just exist if the time value is
> unresolved?

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

[Antoine Quint]

I'll take a look and fix tomorrow.

[Michael Catanzaro]

*** Bug 187036 has been marked as a duplicate of this bug. ***

[Antoine Quint]

Actually, this will have to wait until monday.

[Frédéric Wang (:fredw)]

(In reply to Antoine Quint from comment #4)
> I'll take a look and fix tomorrow.

(In reply to Antoine Quint from comment #6)
> Actually, this will have to wait until monday.

@Antoine Quint: Do you plan to take a look at bug 187145 too? It seems quite similar...

[Antoine Quint]

Yes, they're both on my list.

[Antoine Quint]

Created attachment 344098 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 344098 [details]
Patch

Thanks, this is what I had in mind. LGTM, but probably someone who is more familiar with the WebAnimation code should r+

[EWS Watchlist]

Comment on attachment 344098 [details]
Patch

Attachment 344098 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8411969

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344102 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Dean Jackson]

Comment on attachment 344098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344098&action=review

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:1250
> -    auto timeOffset = animation()->currentTime().value().seconds();
> +    auto currentTime = animation()->currentTime();
> +    if (!currentTime)
> +        return;
> +
> +    auto timeOffset = currentTime.value().seconds();

I don't understand this. Is currentTime a reference?

[Antoine Quint]

(In reply to Dean Jackson from comment #13)
> Comment on attachment 344098 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344098&action=review
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:1250
> > -    auto timeOffset = animation()->currentTime().value().seconds();
> > +    auto currentTime = animation()->currentTime();
> > +    if (!currentTime)
> > +        return;
> > +
> > +    auto timeOffset = currentTime.value().seconds();
> 
> I don't understand this. Is currentTime a reference?

It's an std::optional<Seconds>.

[Frédéric Wang (:fredw)]

Comment on attachment 344098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344098&action=review

>>> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:1250
>>> +    auto timeOffset = currentTime.value().seconds();
>> 
>> I don't understand this. Is currentTime a reference?
> 
> It's an std::optional<Seconds>.

You can probably write it currentTime->seconds() if that make it easier to read.

[Antoine Quint]

(In reply to Frédéric Wang (:fredw) from comment #15)
> Comment on attachment 344098 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344098&action=review
> 
> >>> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:1250
> >>> +    auto timeOffset = currentTime.value().seconds();
> >> 
> >> I don't understand this. Is currentTime a reference?
> > 
> > It's an std::optional<Seconds>.
> 
> You can probably write it currentTime->seconds() if that make it easier to
> read.

I'll land it that way, thanks Frédéric.

[Antoine Quint]

Committed r233429: <https://trac.webkit.org/changeset/233429>

[Radar WebKit Bug Importer]

<rdar://problem/41739017>

[Antoine Quint]

*** Bug 187463 has been marked as a duplicate of this bug. ***