Animation Worklet - constructing worklet animation for an unregistered animator should throw |
||||
Issue description
I suggest that we throw if someone tries to create a worklet animation for an animator which is not yet registered.
Alternative is to not throw but instead keep the animation around and silently upgrade and play it once animator is registered. I argue this is not a very good API as it does not allow the user to detect if the animator they are interested is registered or failed to register. Also, it is counter intuitive that playing and animation becomes a no-op if animator is not registered.
Note that the audio worklet behaves similar to the proposed approach while the paint worklet uses the second approach. My take is that this has to do with the fact that AudioWorklet exposes a JS API while paint worklet has only a CSS API where silent failure is the normal behavior.
Currently, we don't do either. We basically fail silently without trying!
Basically the proposal is the following behavior:
before = new WorkletAnimation('custom', ....); // should throw
await CSS.animationWorklet('custom-animation.js'); // registers 'custom' animator
after = new WorkletAnimation('custom', ....); // works fine
If we are to implement this behavior we need to sync a list of all registered animators back to the main thread as part of processing a module.
I don't think this adds a lot of complexity. We can probably look at how AudioWorklet is already achieving this and perhaps come up with a common
way of doing this.
,
Jan 8
,
Jan 9
,
Jan 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b48a21c421756109b23f9cb9c639be6c48d371a0 commit b48a21c421756109b23f9cb9c639be6c48d371a0 Author: Yi Gu <yigu@chromium.org> Date: Tue Jan 15 05:25:57 2019 [Animation Worklet] Throw upon constructing WorkletAnimation for unregistered animator Currently the WorkletAnimation construction silently fails if the animator has not yet been registered. Similar to AudioWorklet, the proper way of handling such scenario is throwing an exception. Bug: 875262 Change-Id: I1be22f1b8c07b5e83b4071b92a70f991449874a5 Reviewed-on: https://chromium-review.googlesource.com/c/1403930 Commit-Queue: Yi Gu <yigu@chromium.org> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Reviewed-by: Majid Valipour <majidvp@chromium.org> Cr-Commit-Position: refs/heads/master@{#622741} [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/core/animation/worklet_animation_controller.cc [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/core/animation/worklet_animation_controller.h [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/modules/animationworklet/animation_worklet_global_scope.cc [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/modules/animationworklet/animation_worklet_global_scope_test.cc [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/modules/animationworklet/animation_worklet_proxy_client.cc [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/modules/animationworklet/animation_worklet_proxy_client.h [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/modules/animationworklet/worklet_animation.cc [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/modules/animationworklet/worklet_animation_test.cc [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/platform/graphics/animation_worklet_mutator_dispatcher_impl.cc [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/platform/graphics/animation_worklet_mutator_dispatcher_impl.h [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/platform/graphics/compositor_mutator_client.h [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/platform/graphics/main_thread_mutator_client.cc [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/platform/graphics/main_thread_mutator_client.h [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/renderer/platform/graphics/mutator_client.h [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/web_tests/animations/animationworklet/scroll-timeline-dispose.html [modify] https://crrev.com/b48a21c421756109b23f9cb9c639be6c48d371a0/third_party/blink/web_tests/animations/animationworklet/worklet-animation-creation.html
,
Jan 15
This CL breaks the leaddetector bots https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Leak%20Detection%20Linux/30156 (ERROR) 2019-01-14 21:38:36,724 story_runner.ProcessError:116 Possibly handleable error. Will try to restart shared state Traceback (most recent call last): File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/story_runner.py", line 134, in _RunStoryAndProcessErrorIfNeeded state.WillRunStory(story) File "/b/s/w/ir/third_party/catapult/common/py_trace_event/py_trace_event/trace_event_impl/decorators.py", line 75, in traced_function return func(*args, **kwargs) File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/page/shared_page_state.py", line 258, in WillRunStory self.browser.tabs[0].WaitForDocumentReadyStateToBeComplete() File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/browser/tab_list.py", line 18, in __getitem__ return self._tab_list_backend.__getitem__(index) File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend_list.py", line 64, in __getitem__ return self.GetBackendFromContextId(context_id) File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend_list.py", line 75, in GetBackendFromContextId context_id) File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py", line 598, in GetInspectorBackend self._app_backend.app, self._devtools_client, context) File "/b/s/w/ir/third_party/catapult/common/py_trace_event/py_trace_event/trace_event_impl/decorators.py", line 75, in traced_function return func(*args, **kwargs) File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py", line 72, in __init__ self._log = inspector_log.InspectorLog(self._websocket) File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_log.py", line 19, in __init__ self._Enable() File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_log.py", line 29, in _Enable self._inspector_websocket.SyncRequest({'method': 'Log.enable'}, timeout) File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_websocket.py", line 132, in SyncRequest res = self._Receive(timeout) File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_websocket.py", line 190, in _Receive self._HandleNotification(result) File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_websocket.py", line 203, in _HandleNotification self._domain_handlers[domain_name](result) File "/b/s/w/ir/third_party/catapult/common/py_trace_event/py_trace_event/trace_event_impl/decorators.py", line 75, in traced_function return func(*args, **kwargs) File "/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py", line 484, in _HandleInspectorDomainNotification raise exception DevtoolsTargetCrashException: Devtools target crashed ******************************************************************************** (/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py:539 _AddDebuggingInformation) Received a socket error in the browser connection and the tab no longer exists. The tab probably crashed. ******************************************************************************** (/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome_inspector/inspector_backend.py:540 _AddDebuggingInformation) Debugger url: ws://127.0.0.1:41811/devtools/page/23126E774713652EEB2609488AE2B651 ******************************************************************************** (/b/s/w/ir/third_party/catapult/telemetry/telemetry/internal/backends/chrome/tab_list_backend.py:113 _HandleDevToolsConnectionError) The browser exists and can be reached. The devtools target probably crashed. Found Minidump: True Stack Trace: ******************************************************************************** Operating system: Linux 0.0.0 Linux 4.4.0-78-generic #99~14.04.2-Ubuntu SMP Thu Apr 27 18:49:46 UTC 2017 x86_64 CPU: amd64 family 6 model 79 stepping 0 8 CPUs GPU: UNKNOWN Crash reason: SIGSEGV Crash address: 0x0 Process uptime: not available Thread 0 (crashed) 0 chrome!blink::WorkletAnimationController::MutateAnimations() + 0x1f rax = 0x01c80070ebdcdb83 rdx = 0x00007ffecb2c35a8 rcx = 0x000055624b0b67e8 rbx = 0x0000241ba5d42be0 rsi = 0x0000000000000001 rdi = 0x0000241ba5d42be0 rbp = 0x00007ffecb2c35f0 rsp = 0x00007ffecb2c35c0 r8 = 0x0000000000000000 r9 = 0x0000000000000001 r10 = 0x0000000000000000 r11 = 0x00007f7174535310 r12 = 0x00007ffecb2c3660 r13 = 0x00007ffecb2c36a8 r14 = 0x0000000000000001 r15 = 0x00007ffecb2c3670 rip = 0x000055625100b9ef Found by: given as instruction pointer in context 1 chrome!blink::WorkletAnimationController::UpdateAnimationTimings(blink::TimingUpdateReason) + 0x1d rbx = 0x0000241ba5d42be0 rbp = 0x00007ffecb2c3620 rsp = 0x00007ffecb2c3600 r12 = 0x00007ffecb2c3660 r13 = 0x00007ffecb2c36a8 r14 = 0x0000000000000001 r15 = 0x00007ffecb2c3670 rip = 0x000055625100b93d Found by: call frame info 2 chrome!blink::PageAnimator::ServiceScriptedAnimations(base::TimeTicks) + 0x27c rbx = 0x00007ffecb2c36a0 rbp = 0x00007ffecb2c37d0 rsp = 0x00007ffecb2c3630 r12 = 0x00007ffecb2c3660 r13 = 0x00007ffecb2c36a8 r14 = 0x000000006d32825c r15 = 0x00007ffecb2c3670 rip = 0x000055625168efec Found by: call frame info 3 chrome!blink::WebViewImpl::BeginFrame(base::TimeTicks) + 0xa5 rbx = 0x00007ffecb2c37e8 rbp = 0x00007ffecb2c3840 rsp = 0x00007ffecb2c37e0 r12 = 0x000022a6b241d0c0 r13 = 0x0000000000000000 r14 = 0x000000006d32825c r15 = 0x000032aec3e80000 rip = 0x000055625114e885 Found by: call frame info 4 chrome!cc::ProxyMain::BeginMainFrame(std::__1::unique_ptr<cc::BeginMainFrameAndCommitState, std::__1::default_delete<cc::BeginMainFrameAndCommitState> >) + 0x2ac rbx = 0x000022a6b62dc700 rbp = 0x00007ffecb2c3980 rsp = 0x00007ffecb2c3850 r12 = 0x000022a6b241d0c0 r13 = 0x00007ffecb2c3990 r14 = 0x000022a6b62c1900 r15 = 0x000022a6b6215888 rip = 0x000055624f4e908c Found by: call frame info 5 chrome!void base::internal::Invoker<base::internal::BindState<void (cc::ProxyMain::*)(std::__1::unique_ptr<cc::BeginMainFrameAndCommitState, std::__1::default_delete<cc::BeginMainFrameAndCommitState> >), base::WeakPtr<cc::ProxyMain>, base::internal::PassedWrapper<std::__1::unique_ptr<cc::BeginMainFrameAndCommitState, std::__1::default_delete<cc::BeginMainFrameAndCommitState> > > >, void ()>::RunImpl<void (cc::ProxyMain::*)(std::__1::unique_ptr<cc::BeginMainFrameAndCommitState, std::__1::default_delete<cc::BeginMainFrameAndCommitState> >), std::__1::tuple<base::WeakPtr<cc::ProxyMain>, base::internal::PassedWrapper<std::__1::unique_ptr<cc::BeginMainFrameAndCommitState, std::__1::default_delete<cc::BeginMainFrameAndCommitState> > > >, 0ul, 1ul>(void (cc::ProxyMain::*&&)(std::__1::unique_ptr<cc::BeginMainFrameAndCommitState, std::__1::default_delete<cc::BeginMainFrameAndCommitState> >), std::__1::tuple<base::WeakPtr<cc::ProxyMain>, base::internal::PassedWrapper<std::__1::unique_ptr<cc::BeginMainFrameAndCommitState, std::__1::default_delete<cc::BeginMainFrameAndCommitState> > > >&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>) + 0xc7 rbx = 0x000022a6b62dc7b0 rbp = 0x00007ffecb2c3ae0 rsp = 0x00007ffecb2c3990 r12 = 0x000055624f4e8de0 r13 = 0x0000556252579eb0 r14 = 0x000022a6b62c19a0 r15 = 0x0000000000000000 rip = 0x000055624f4ee5c7 Found by: call frame info Reverting.
,
Jan 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a commit 304e0d5a9053b04bef7e4f277aae0aa6e5ce785a Author: Dominic Battré <battre@chromium.org> Date: Tue Jan 15 08:36:17 2019 Revert "[Animation Worklet] Throw upon constructing WorkletAnimation for unregistered animator" This reverts commit b48a21c421756109b23f9cb9c639be6c48d371a0. Reason for revert: Memory leaks, see crbug.com/875262 Original change's description: > [Animation Worklet] Throw upon constructing WorkletAnimation for unregistered animator > > Currently the WorkletAnimation construction silently fails if the > animator has not yet been registered. Similar to AudioWorklet, the > proper way of handling such scenario is throwing an exception. > > Bug: 875262 > Change-Id: I1be22f1b8c07b5e83b4071b92a70f991449874a5 > Reviewed-on: https://chromium-review.googlesource.com/c/1403930 > Commit-Queue: Yi Gu <yigu@chromium.org> > Reviewed-by: Jeremy Roman <jbroman@chromium.org> > Reviewed-by: Majid Valipour <majidvp@chromium.org> > Cr-Commit-Position: refs/heads/master@{#622741} TBR=jbroman@chromium.org,majidvp@chromium.org,yigu@chromium.org Change-Id: I23ef3c2bb7d3605f17b8be5fdbc447dc2b9401c2 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 875262 Reviewed-on: https://chromium-review.googlesource.com/c/1411334 Reviewed-by: Dominic Battré <battre@chromium.org> Commit-Queue: Dominic Battré <battre@chromium.org> Cr-Commit-Position: refs/heads/master@{#622781} [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/core/animation/worklet_animation_controller.cc [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/core/animation/worklet_animation_controller.h [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/modules/animationworklet/animation_worklet_global_scope.cc [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/modules/animationworklet/animation_worklet_global_scope_test.cc [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/modules/animationworklet/animation_worklet_proxy_client.cc [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/modules/animationworklet/animation_worklet_proxy_client.h [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/modules/animationworklet/worklet_animation.cc [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/modules/animationworklet/worklet_animation_test.cc [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/platform/graphics/animation_worklet_mutator_dispatcher_impl.cc [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/platform/graphics/animation_worklet_mutator_dispatcher_impl.h [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/platform/graphics/compositor_mutator_client.h [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/platform/graphics/main_thread_mutator_client.cc [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/platform/graphics/main_thread_mutator_client.h [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/renderer/platform/graphics/mutator_client.h [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/web_tests/animations/animationworklet/scroll-timeline-dispose.html [modify] https://crrev.com/304e0d5a9053b04bef7e4f277aae0aa6e5ce785a/third_party/blink/web_tests/animations/animationworklet/worklet-animation-creation.html
,
Jan 17
(6 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba commit a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba Author: Yi Gu <yigu@chromium.org> Date: Thu Jan 17 01:52:44 2019 Reland "[Animation Worklet] Throw upon constructing WorkletAnimation for unregistered animator" This reverts commit 304e0d5a9053b04bef7e4f277aae0aa6e5ce785a. Reason for revert: The revert was in build 30164 but the memory leak was gone before that. [1] The reverted patch actually didn't touch the path that was in the crash report. [1] https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Leak%20Detection%20Linux?limit=200 Original change's description: > Revert "[Animation Worklet] Throw upon constructing WorkletAnimation for unregistered animator" > > This reverts commit b48a21c421756109b23f9cb9c639be6c48d371a0. > > Reason for revert: Memory leaks, see crbug.com/875262 > > Original change's description: > > [Animation Worklet] Throw upon constructing WorkletAnimation for unregistered animator > > > > Currently the WorkletAnimation construction silently fails if the > > animator has not yet been registered. Similar to AudioWorklet, the > > proper way of handling such scenario is throwing an exception. > > > > Bug: 875262 > > Change-Id: I1be22f1b8c07b5e83b4071b92a70f991449874a5 > > Reviewed-on: https://chromium-review.googlesource.com/c/1403930 > > Commit-Queue: Yi Gu <yigu@chromium.org> > > Reviewed-by: Jeremy Roman <jbroman@chromium.org> > > Reviewed-by: Majid Valipour <majidvp@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#622741} > > TBR=jbroman@chromium.org,majidvp@chromium.org,yigu@chromium.org > > Change-Id: I23ef3c2bb7d3605f17b8be5fdbc447dc2b9401c2 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 875262 > Reviewed-on: https://chromium-review.googlesource.com/c/1411334 > Reviewed-by: Dominic Battré <battre@chromium.org> > Commit-Queue: Dominic Battré <battre@chromium.org> > Cr-Commit-Position: refs/heads/master@{#622781} TBR=battre@chromium.org,jbroman@chromium.org,majidvp@chromium.org,yigu@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 875262 Change-Id: I108089ad956f31c920a352608553d374cc677e89 Reviewed-on: https://chromium-review.googlesource.com/c/1415831 Commit-Queue: Yi Gu <yigu@chromium.org> Reviewed-by: Yi Gu <yigu@chromium.org> Cr-Commit-Position: refs/heads/master@{#623507} [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/core/animation/worklet_animation_controller.cc [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/core/animation/worklet_animation_controller.h [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/modules/animationworklet/animation_worklet_global_scope.cc [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/modules/animationworklet/animation_worklet_global_scope_test.cc [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/modules/animationworklet/animation_worklet_proxy_client.cc [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/modules/animationworklet/animation_worklet_proxy_client.h [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/modules/animationworklet/worklet_animation.cc [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/modules/animationworklet/worklet_animation_test.cc [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/platform/graphics/animation_worklet_mutator_dispatcher_impl.cc [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/platform/graphics/animation_worklet_mutator_dispatcher_impl.h [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/platform/graphics/compositor_mutator_client.h [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/platform/graphics/main_thread_mutator_client.cc [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/platform/graphics/main_thread_mutator_client.h [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/renderer/platform/graphics/mutator_client.h [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/web_tests/animations/animationworklet/scroll-timeline-dispose.html [modify] https://crrev.com/a0e5541641b1c252ce6e7fc4d1cb6a2a454abfba/third_party/blink/web_tests/animations/animationworklet/worklet-animation-creation.html
,
Jan 17
(5 days ago)
The optimization is tracked via issue 920722. Close this one to free AnimationWorklet-MVP. |
||||
►
Sign in to add a comment |
||||
Comment 1 by majidvp@chromium.org
, Jan 8