Bug 187139 - [Web Animations] Crash in KeyframeEffectReadOnly::applyPendingAcceleratedActions()
Summary: [Web Animations] Crash in KeyframeEffectReadOnly::applyPendingAcceleratedActi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: Gtk, InRadar
: 187036 187463 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-06-28 06:24 PDT by Carlos Garcia Campos
Modified: 2018-07-19 16:00 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.62 KB, patch)
2018-07-02 06:10 PDT, Antoine Quint
dino: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.80 MB, application/zip)
2018-07-02 07:51 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-06-28 06:24:38 PDT
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.
Comment 1 Carlos Garcia Campos 2018-06-28 06:24:58 PDT
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 ()
Comment 2 Frédéric Wang (:fredw) 2018-06-28 08:29:31 PDT
(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?
Comment 3 Carlos Garcia Campos 2018-06-28 08:34:19 PDT
(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
Comment 4 Antoine Quint 2018-06-28 10:21:55 PDT
I'll take a look and fix tomorrow.
Comment 5 Michael Catanzaro 2018-06-28 10:47:35 PDT
*** Bug 187036 has been marked as a duplicate of this bug. ***
Comment 6 Antoine Quint 2018-06-29 05:38:03 PDT
Actually, this will have to wait until monday.
Comment 7 Frédéric Wang (:fredw) 2018-06-29 07:57:22 PDT
(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...
Comment 8 Antoine Quint 2018-06-30 07:57:35 PDT
Yes, they're both on my list.
Comment 9 Antoine Quint 2018-07-02 06:10:54 PDT
Created attachment 344098 [details]
Patch
Comment 10 Frédéric Wang (:fredw) 2018-07-02 06:19:17 PDT
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+
Comment 11 Build Bot 2018-07-02 07:51:34 PDT
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
Comment 12 Build Bot 2018-07-02 07:51:46 PDT
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
Comment 13 Dean Jackson 2018-07-02 12:44:56 PDT
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?
Comment 14 Antoine Quint 2018-07-02 13:10:49 PDT
(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>.
Comment 15 Frédéric Wang (:fredw) 2018-07-02 13:15:08 PDT
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.
Comment 16 Antoine Quint 2018-07-02 13:30:54 PDT
(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.
Comment 17 Antoine Quint 2018-07-02 13:45:37 PDT
Committed r233429: <https://trac.webkit.org/changeset/233429>
Comment 18 Radar WebKit Bug Importer 2018-07-02 13:47:13 PDT
<rdar://problem/41739017>
Comment 19 Antoine Quint 2018-07-19 16:00:27 PDT
*** Bug 187463 has been marked as a duplicate of this bug. ***