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