webots
webots copied to clipboard
cyberbotics
Webots crashes for null radius in `Extrusion`
Describe the Bug
As surfaced here https://github.com/cyberbotics/webots/pull/3627, when changing the radius of the signs (CautionSign, ExitSign,...) to 0 or to a negative value, it crashes Webots. In practice the problem likely stems from the Extrusion sub PROTO.
Although it's good practice to sanitize the parameters, it isn't normal that it crashes regardless.
It is not a regression.
Is that one already fixed? I am getting that message when I tried to change a CautionSign (yes it was a CautionSign and not a StopSign as noted below) WARNING: StopSign (PROTO): Invalid 'radius' changed to 1. The value should be positive. Tested on Master branch.
Originally it was crashing on feature-enu-plan branch, however I just tested and it still crashes both in that branch and in master (ubuntu 20).
I could make it crash on Windows, by pressing the little "down" button several times until the value of radius reaches 0. But if I enter 0 or -1, it doesn't crash and I am getting the same warning as @llessieux.
Indeed I can reproduce the crash that way too. I might have a look at it next week too. Crashes tend to be easy to fix ;)
I had a look at that and it is very strange. The checks are in place and it does prevent the value to go negative but somehow after the value is set to the default value of 1.0 it crashes elsewhere. I was also wondering why such tests were done so late in the process. By that I mean wouln't it be better to prevent the UI to try to set invalid values in the first place.
But it seems to be a problem caused by a re-entry loop inside the WbDoubleEditor when it is applying a value and a check is setting the value to something else than what was passed.
Basically things happen like that , the spin box changes the value to something below 0, the double editor notifies. it goes to the WbField in the Cylinder which checks the value and set it to 1. That triggers a regeneration, which triggers a clearSelection of the WbSceneTree. The clearSelection is calling the editField of the mFieldEditor with NULL. This checks the current Editor and call applyIfNeeded. It goes back to the editor and check if the spin value is different from the mDouble (which has been reset to 1) so it tries to apply again and something goes wrong later.
Adding a mApplyingValue set to true when an apply call is in progress and back to false at the end and checking if that flag is set in applyIfNeeded and returning if it is already applying a value seems to fix it but I really wonder if that is the correct way. This displays the warning in the console and the value is reset to 1 (instead of the default value specified in the Proto, I guess that one is lost). I really feels that an editor should not have re-entering apply calls but I am not familiar with all use cases to say that it is the correct fix. Now this probably should be applied to other editors too and perhaps factorized out in the WbValueEditor (somehow).
I can create a branch (master / develop?) to show the minimal fix then try to provide a more extensive solution that would cover all editors. (probably preApply/postApply/isApplying methods added to the WbValueEditor and called from all editors, with the inner code in apply in a try/catch to make sure that the postApply is called or if you have guard class to force operations on function exit use that.)
Thank you for the detailed report. Yes, you go ahead with a fix, preferably on the master branch. We will be happy to review it.
Ok it doesn't seem to be as simple as I described it :( My code prevents the first crash but clicking on the spin box again crashes in a completely different place. And it doesn't seem to crash at always the same location either so some memory must be corrupted or something like that. I will dig deeper but I have the impression that it is not just one bug ;)
By the way, is there anyway to build Webots using Visual Studio? The new one can debug with gdb information but it seems pretty slow to get the info.
By the way, is there anyway to build Webots using Visual Studio?
I never tried, but in theory, it should be possible.
I might have a look at that at some point. That would make debugging easier for me ;) I am seeing some really strange behavior in this bug... scene tree being called twice in different threads (apparently since I am getting interleaved logs added into it), WbField with null mValue etc..
Yes there is something really wrong somewhere. Deleted WbField are receiving notifications :( I have logs inserted in the WbField/WbNode creation too but none in that section.
WbNode::notifyFieldChanged: Group Deleting WbNode 0000021beecb30a0 Deleting WbField 0000021be08eb710 Deleting WbField 0000021be08ebb30 ... Deleting WbNode 0000021bc3962be0 Deleting WbField 0000021bfe793c60 Deleting WbField 0000021bfe793a20 Deleting WbField 0000021bfe7909c0 Deleting WbField 0000021bfe790fc0 :boom: Deleting WbField 0000021bfe790960 Deleting WbField 0000021bfe790180 ... Deleting WbField 0000021be070ef90 Deleting WbNode 0000021bc3a1bcd0 Deleting WbField 0000021be070eff0 ... Deleting WbField 0000021ba245bf90 Deleting WbField 0000021be070dbb0 Deleting WbField 0000021be070e3f0 :boom: ... Deleting WbField 0000021be070d610 WbFieldChecker::resetDoubleIfNonPositive WbFieldChecker::resetDoubleIfNonPositive WbFieldChecker::resetDoubleIfNonPositive WbFieldChecker::resetDoubleIfNonPositive WbSceneTree::applyNodeRegeneration: WbSceneTree::updateSelection this = 0000021bcf27d2d0 ThreadID = 5348 WbSceneTree::updateSelection: WbSceneTree::updateSelection: mFieldEditor->editField WbSceneTree::updateSelection: Done WbSceneTree::updateSelection this = 0000021bcf27d2d0 ThreadID = 5348 WbSceneTree::updateSelection: WbValueEditor::edit: Node Name=CautionSign Field Name=radius WbSceneTree::updateSelection: Done WbSceneTree::updateSelection this = 0000021bcf27d2d0 ThreadID = 5348 WbSceneTree::updateSelection: WbSceneTree::updateSelection: Done WbSceneTree::applyNodeRegeneration: Done WbField::copyValueFrom this = 0000021bfe790fc0 :boom: other = 0000021be070e3f0 :boom: other Value = 6a6f72702f73746f Exception thrown at 0x00007FF7754A1681 in webots-bin.exe: 0xC0000005: Access violation reading location 0x0000000000000000.
webots-bin.exe!WbField::copyValueFrom(WbField * volatile this, volatile WbField * other) Line 441 C++ webots-bin.exe!WbField::parameterChanged(WbField * volatile this) Line 388 C++ webots-bin.exe!00007ff7757f3359() Line 152 C++ webots-bin.exe!00007ff77581bcac() Line 186 C++ webots-bin.exe!00007ff7758100b5() Line 419 C++ [External Code] webots-bin.exe!WbField::valueChanged(WbField * volatile this) Line 203 C++ webots-bin.exe!00007ff7757f3359() Line 152 C++ webots-bin.exe!00007ff77581bcac() Line 186 C++ webots-bin.exe!00007ff7758100b5() Line 419 C++ [External Code] webots-bin.exe!WbValue::changed(WbValue * volatile this) Line 167 C++ webots-bin.exe!WbSFDouble::setValue(WbSFDouble * volatile this, double d) Line 62 C++ webots-bin.exe!WbEditCommand::resetValue(WbEditCommand * volatile this, volatile WbVariant & newValue) Line 98 C++ webots-bin.exe!WbEditCommand::redo(WbEditCommand * volatile this) Line 59 C++ [External Code] webots-bin.exe!00007ff775602608() Line 54 C++ webots-bin.exe!WbValueEditor::apply(WbValueEditor * volatile this) Line 124 C++ webots-bin.exe!WbDoubleEditor::apply(WbDoubleEditor * volatile this) Line 107 C++ webots-bin.exe!00007ff7757f0129() Line 152 C++ webots-bin.exe!00007ff7758183ac() Line 186 C++ webots-bin.exe!00007ff775803775() Line 419 C++ [External Code] webots-bin.exe!WbFieldDoubleSpinBox::valueApplied(WbFieldDoubleSpinBox * volatile this) Line 152 C++ webots-bin.exe!WbFieldDoubleSpinBox::stepBy(WbFieldDoubleSpinBox * volatile this, int steps) Line 93 C++ [External Code] webots-bin.exe!WbGuiApplication::exec(WbGuiApplication * volatile this) Line 343 C++ webots-bin.exe!main(int argc, char * * argv) Line 207 C++ [External Code] webots-bin.exe!mainCRTStartup() Line 204 C++
Also why are QPointer not used to prevent that kind of things? I think that there are made for that.
I think that I got it now but I instrumented and modified the code so much that it will take me a while to figure out the minimum set of changes for it. Basically I think that the editors are keeping references to deleted object due to the regeneration somehow. Adding connect to the destroyed signal to disable the editor might have done the trick. But I am certainly not sure that something more fundamental is not broken here. The editors should probably have been cleared and recreated but somehow the event might have been queued.
Also why are QPointer not used to prevent that kind of things?
Yes, that could help indeed...
One thing that bothers me though, is that I am not sure why the scene tree is regenerating the node in the first place. It is not as if the field or node associated with the editor was changing when the value was updated... I can understand doing something when a field is added or removed but regenerating the full top node seems a bit excessive from an UI point of view. Is there any particular reason for doing it?
This is because the node you are modifying (Extrusion) is a PROTO node which runs Javascript templating. Therefore, if you modify a field parameter, the node needs to be regenerated as the change in field parameter may impact outcome of the execution of the Javascript code that generates the node.
Still not sure that I fixed it. This one is really annoying. I am probably missing the real cause.