Assertion failure with default PROTO parameter declaration

[stefaniapedrazzi]

Webots crashes due to an assertion failure when opening the derived_proto_with_template_default_parameter.wbt test. After the load the scene tree selection is updated to the TemplateSolidCylinder parameter node of the ProtoWithTemplateDefaultParameter PROTO.

TemplateSolidCylinder is correctly declared in the ProtoWithTemplateDefaultParameter PROTO file but not in the world file/scene tree level and causes the following assertion failure:

webots-bin: vrml/WbProtoManager.cpp:826: QString WbProtoManager::externProtoDeclaration(const QString&, bool) const: Assertion `false' failed.

Thread 1 "webots-bin" received signal SIGABRT, Aborted.
__pthread_kill_implementation (no_tid=0, signo=6, threadid=140737251429696) at ./nptl/pthread_kill.c:44
44	./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140737251429696) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=140737251429696) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=140737251429696, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ffff4bb1476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff4b977f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ffff4b9771b in __assert_fail_base
    (fmt=0x7ffff4d4c150 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5555561b8e7b "false", file=0x5555561b8664 "vrml/WbProtoManager.cpp", line=826, function=<optimized out>)
    at ./assert/assert.c:92
#6  0x00007ffff4ba8e96 in __GI___assert_fail
    (assertion=0x5555561b8e7b "false", file=0x5555561b8664 "vrml/WbProtoManager.cpp", line=826, function=0x5555561b8e30 "QString WbProtoManager::externProtoDeclaration(const QString&, bool) const")
    at ./assert/assert.c:101
#7  0x0000555555dd1a4c in WbProtoManager::externProtoDeclaration(QString const&, bool) const (this=0x555556971fc0, protoName=..., formatted=true) at vrml/WbProtoManager.cpp:826
#8  0x0000555555d748d4 in WbNodeEditor::update() (this=0x5555565800e0) at scene_tree/WbNodeEditor.cpp:160
#9  0x0000555555d745fa in WbNodeEditor::edit(bool) (this=0x5555565800e0, copyOriginalValue=true) at scene_tree/WbNodeEditor.cpp:128
#10 0x0000555555f3ad33 in WbValueEditor::edit(WbNode*, WbField*, int) (this=0x5555565800e0, node=0x0, field=0x55555c16bb20, index=0) at scene_tree/WbValueEditor.cpp:79
#11 0x0000555555d7a4f2 in WbNodePane::edit(WbNode*, WbField*, int) (this=0x555556965f30, node=0x0, field=0x55555c16bb20, index=0) at scene_tree/WbNodePane.cpp:70
#12 0x0000555555c6c993 in WbFieldEditor::editField(WbNode*, WbField*, int) (this=0x5555566a6240, node=0x0, field=0x55555c16bb20, item=0) at scene_tree/WbFieldEditor.cpp:251
#13 0x0000555555e3776e in WbSceneTree::updateSelection() (this=0x55555693a550) at scene_tree/WbSceneTree.cpp:1143

[stefaniapedrazzi]

Default parameters are a special case because the node doesn't appear in the world file and thus it seems correct that it is not declared in the world but just in the PROTO fle. We can easily detect if the field is using default values and in this case search in the PROTO file instead of the world file for the declaration.

[ad-daniel]

I'm not entirely convinced it's necessary nor fundamentally correct. Although the TemplateSolidCylinder is not expressed in the world file since it's a default value, it is nonetheless there (i.e. is visible from the scene tree), which makes it for all intents and purposes indistinguishable from any other node available in the world. As such, even though it's a default value, it should nonetheless be declared as EXTERNPROTO as well in the world (which solves the assert).

I'd go as far as say it's not even really an open question, not declaring it in the world is fundamentally wrong. The reason is that in this particular case it happened to be a local PROTO, but if it was a remotely defined one, then it wouldn't even be possible to build the node tree unless a declaration for this node was provided in the world.

EDIT: I'm surprised it even works, it shouldn't be able to build the node tree irrespective of whether it's local or remote. It is however more evident in the remote case since all the necessary PROTO are downloaded prior to generating the node structure, which if there wasn't the intermediary ProtoWithTemplateDefaultParameter proto offering a declaration, TemplateSolidCylinder wouldn't even be downloaded.

[stefaniapedrazzi]

In any case, it is not just a matter of removing the assertion but there are at least these additional issues:

• PROTO should not even been created and an error message should be printed in the console if the declaration is missing
• when inserting a PROTO from the Add Node dialog the EXTERNPROTO declaration for all the default parameters should be automatically added to the world

Note that if we force to have the declaration in the world too, then for default parameters the EXTERNPROTO will always be declared twice: in the current PROTO file and in the ancestor PROTO/world file.

[omichel]

I believe this simple rule should be always enforced:

if and only if a proto is instantiated in a saved world file (.wbt file), then it should be declared as an EXTERNPROTO in the world file.

In case of a default proto parameter containing another proto, this second proto name will not appear in the saved world file and hence it shouldn't be declared as an EXTERNPROTO in the world file.

[stefaniapedrazzi]

It is fine that if a PROTO B is defined as default parameter of another PROTO A it should not be declared in the world.

But following this rule: when saving the world after modifying the PROTO B parameter, PROTO B is now saved in the world file and should be declared in the WBT file. However this is not currently the case: after saving the PROTO B declaration is not added to the world file but everything loads correctly.

[ygoumaz]

Just in case, I found an additional case which triggers this assertion. I don't know if your current fix includes this case.

1. Open a world containing a local PROTO at the first level of the Scene tree, but remove the declaration from the world file before opening it (which should be in relative format EXTERNPROTO "../protos/example.proto").
2. The console displays a warning which explains that the declaration is missing.
3. Click on the corresponding PROTO in the scene tree.
4. The assertion triggers and Webots crashes but it shouldn't.

[stefaniapedrazzi]

Just in case, I found an additional case which triggers this assertion. I don't know if your current fix includes this case.

I just checked and it works also in this case. Currently the fix is generic and should work for any PROTO node.