Bug 187139

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 ()

(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?

(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

I'll take a look and fix tomorrow.

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

Actually, this will have to wait until monday.

(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...

Yes, they're both on my list.

Created attachment 344098 [details]
Patch

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 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

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 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?

(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 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.

(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.

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

<rdar://problem/41739017>

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