Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Command Queue Crash #50193

Merged
merged 1 commit into from
Jul 7, 2021
Merged

Conversation

reduz
Copy link
Member

@reduz reduz commented Jul 6, 2021

  • No longer allow sending an object (texture) to the server as material parameter
  • Keep a parameter cache locally in ShaderMaterial

Fixes #49895
Fixes #49201

@reduz reduz requested a review from a team as a code owner July 6, 2021 01:44
@lyuma
Copy link
Contributor

lyuma commented Jul 6, 2021

Here are two related issues reported which show CommandQueueMT in the stacktrace, both filed within the past two weeks (post rewrite):
#49895 and #50039 (marked as duplicate)

AndreaCatania reported a similar issue in rocketchat, but I think a GitHub issue was never filed. Here are the stacktraces from June 23, and later on June 27th respectively:
June 23
June 27

Interestingly, there was one issue filed on May 29th, before CommandQueue was rewritten, which indicates 8b19ffd as the commit which caused the issue:
#49201

@akien-mga
Copy link
Member

Here are two related issues reported which show CommandQueueMT in the stacktrace, both filed within the past two weeks (post rewrite):
#49895 and #50039 (marked as duplicate)

Tested, this PR doesn't fix #49895.

@reduz
Copy link
Member Author

reduz commented Jul 6, 2021

@akien-mga that is odd, judging from the stack trace it should, I will give it a check later today.

@reduz
Copy link
Member Author

reduz commented Jul 6, 2021

@akien-mga Not sure what you tested, but: #49895 does not crash for me, the new code catches the error properly:
ERROR: Condition "p_value.get_type() == Variant::OBJECT" is true.
That said, this still requires fixing because sending objects to the server is no longer allowed, I seem to have forgotten the particles here, so I will amend to the PR that fix.

@reduz
Copy link
Member Author

reduz commented Jul 6, 2021

Ok, I think it should be perfect now, went all cases of material_set_param and id not find anything else that could cause trouble.

@akien-mga
Copy link
Member

akien-mga commented Jul 7, 2021

Edit: Ignore this, I was testing an older version.

#49895 still crashes for me with the current commit of this PR:
set parameter: hue_variation_random
set parameter: anim_speed_random
set parameter: anim_offset_random
set parameter: color_value
ERROR: Condition "p_value.get_type() == Variant::OBJECT" is true.
   at: material_set_param (servers/rendering/renderer_rd/renderer_storage_rd.cpp:1595)

Thread 1 "godot-git" received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()
(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x0000000002348967 in CommandQueueMT::_flush (this=0xa8e8e08) at ./core/templates/command_queue_mt.h:373
#2  0x00000000053bc116 in CommandQueueMT::flush_if_pending (this=0xa8e8e08) at ./core/templates/command_queue_mt.h:404
#3  0x00000000054ae732 in RenderingServerDefault::free (this=0xa8e8c90, p_rid=...) at servers/rendering/rendering_server_default.h:860
#4  0x0000000004fcad2a in GradientTexture::~GradientTexture (this=0x7fff34003e30, __in_chrg=<optimized out>) at scene/resources/texture.cpp:1752
#5  0x000000000238c5b6 in memdelete<RefCounted> (p_class=0x7fff34003e30) at ./core/os/memory.h:115
#6  0x0000000005b724e2 in Variant::_clear_internal (this=0x7fff3400a388) at core/variant/variant.cpp:1309
#7  0x000000000216e7d7 in Variant::clear (this=0x7fff3400a388) at ./core/variant/variant.h:257
#8  0x000000000216e7fc in Variant::~Variant (this=0x7fff3400a388, __in_chrg=<optimized out>) at ./core/variant/variant.h:672
#9  0x00000000054d3796 in CommandQueueMT::Command3<RendererStorage, void (RendererStorage::*)(RID, StringName const&, Variant const&), RID, StringName, Variant>::~Command3 (this=0x7fff3400a358, 
    __in_chrg=<optimized out>) at ./core/templates/command_queue_mt.h:322
#10 0x0000000002348995 in CommandQueueMT::_flush (this=0xa8e8e08) at ./core/templates/command_queue_mt.h:375
#11 0x00000000053bc116 in CommandQueueMT::flush_if_pending (this=0xa8e8e08) at ./core/templates/command_queue_mt.h:404
#12 0x00000000054ab980 in RenderingServerDefault::canvas_item_add_triangle_array (this=0xa8e8c90, p1=..., p2=..., p3=..., p4=..., p5=..., p6=..., p7=..., p8=..., p9=-1)
    at servers/rendering/rendering_server_default.h:766
#13 0x0000000004f73218 in StyleBoxFlat::draw (this=0xd665fd0, p_canvas_item=..., p_rect=...) at scene/resources/style_box.cpp:774
#14 0x0000000004873ca1 in Tabs::_notification (this=0xcd9f720, p_what=30) at scene/gui/tabs.cpp:353
#15 0x000000000487f838 in Tabs::_notificationv (this=0xcd9f720, p_notification=30, p_reversed=false) at scene/gui/tabs.h:38
#16 0x0000000005dd32a5 in Object::notification (this=0xcd9f720, p_notification=30, p_reversed=false) at core/object/object.cpp:841
#17 0x000000000458501f in CanvasItem::_update_callback (this=0xcd9f720) at scene/main/canvas_item.cpp:427
#18 0x00000000023992fd in call_with_variant_args_helper<__UnexistingClass>(__UnexistingClass*, void (__UnexistingClass::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (p_instance=0xcd9f720, 
--Type <RET> for more, q to quit, c to continue without paging--
    p_method=(void (__UnexistingClass::*)(__UnexistingClass * const)) 0x4584f30 <CanvasItem::_update_callback()>, p_args=0x7fffffffc9c8, r_error=...) at ./core/variant/binder_common.h:218
#19 0x0000000002398a3b in call_with_variant_args_dv<__UnexistingClass> (p_instance=0xcd9f720, p_method=(void (__UnexistingClass::*)(__UnexistingClass * const)) 0x4584f30 <CanvasItem::_update_callback()>, 
    p_args=0x0, p_argcount=0, r_error=..., default_values=...) at ./core/variant/binder_common.h:365
#20 0x0000000002397a2e in MethodBindT<>::call(Object*, Variant const**, int, Callable::CallError&) (this=0xb60aff0, p_object=0xcd9f720, p_args=0x0, p_arg_count=0, r_error=...) at ./core/object/method_bind.h:285
#21 0x0000000005dd3188 in Object::call (this=0xcd9f720, p_method=..., p_args=0x0, p_argcount=0, r_error=...) at core/object/object.cpp:832
#22 0x0000000005b69d35 in Callable::call (this=0x7ffff5a43038, p_arguments=0x0, p_argcount=0, r_return_value=..., r_call_error=...) at core/variant/callable.cpp:62
#23 0x0000000005dcbeb7 in MessageQueue::_call_function (this=0xa0c75c0, p_callable=..., p_args=0x7ffff5a43050, p_argcount=0, p_show_error=false) at core/object/message_queue.cpp:258
#24 0x0000000005dcc1d6 in MessageQueue::flush (this=0xa0c75c0) at core/object/message_queue.cpp:304
#25 0x00000000045f568c in SceneTree::process (this=0xab6a720, p_time=0.149999619) at scene/main/scene_tree.cpp:449
#26 0x00000000021afc6d in Main::iteration () at main/main.cpp:2541
#27 0x0000000002170937 in OS_LinuxBSD::run (this=0x7fffffffd270) at platform/linuxbsd/os_linuxbsd.cpp:278
#28 0x000000000216d8da in main (argc=2, argv=0x7fffffffd788) at platform/linuxbsd/godot_linuxbsd.cpp:58

System specs:

System:    Host: cauldron Kernel: 5.12.14-desktop-3.mga9 x86_64 bits: 64 Desktop: KDE Plasma 5.21.5 Distro: Mageia 9 mga9 
CPU:       Info: Quad Core model: Intel Core i7-8705G bits: 64 type: MT MCP cache: L2: 8 MiB 
           Speed: 1716 MHz min/max: 800/3100 MHz Core speeds (MHz): 1: 1716 2: 1067 3: 902 4: 856 5: 834 6: 831 7: 860 8: 845 
Graphics:  Device-1: Intel HD Graphics 630 driver: i915 v: kernel 
           Device-2: Advanced Micro Devices [AMD/ATI] Polaris 22 XL [Radeon RX Vega M GL] driver: amdgpu v: kernel 
           Device-3: Cheng Uei Precision Industry (Foxlink) HP Wide Vision FHD Camera type: USB driver: uvcvideo 
           Display: x11 server: Mageia X.org 1.20.11 driver: loaded: intel,v4l resolution: 1920x1080~60Hz 
           OpenGL: renderer: Mesa Intel HD Graphics 630 (KBL GT2) v: 4.6 Mesa 21.1.4

* No longer allow sending an object (texture) to the server as material parameter
* Keep a parameter cache locally in ShaderMaterial
@reduz reduz requested review from a team as code owners July 7, 2021 13:58
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was testing the wrong commit previously. After testing the right commit, I can confirm it works fine and fixes #49895.

@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants