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

Protos that Redefine Field Names Cause Unexpected Behavior #6630

Open
CoolSpy3 opened this issue Aug 17, 2024 · 0 comments
Open

Protos that Redefine Field Names Cause Unexpected Behavior #6630

CoolSpy3 opened this issue Aug 17, 2024 · 0 comments
Labels
bug Something isn't working crash Cause a crash

Comments

@CoolSpy3
Copy link
Contributor

Describe the Bug
It looks like this is a bug in the handling of proto fields with the same names. My best guess is that it's maybe a result of WbNode:1740 checking for field existence by name rather than something more robust.

I did some basic debugging and it looks like, for the example below, loading the proto causes Webots to recurse indefinitely on WbNode:1305 because node == this. In essence, this means that one of the nodes is considered its own subnode. My guess is that Webots thinks SolidProto.children = Group.children, so Group is contained in Group.children.

I've also noticed a few other cases where the code assumes that field names dictate their behavior (ex. WbField:336-343). (Although this example is not likely to cause this crash.)

Steps to Reproduce

  1. Create the following proto:
#VRML_SIM R2024a utf8

EXTERNPROTO "SolidProto.proto"

PROTO SolidProtoContainer [
  field MFNode children []
]
{
SolidProto {
  children [
    Group {
      children IS children
    }
  ]
}
}

(Note that this depends on the SolidProto.proto file in tests/api/protos)
2. Try to add the proto to a world
3. Webots crashes
4. Change the field name to children1 and try to load the proto again
5. Webots successfully loads the proto and does not crash.

Expected behavior
Webots should load the proto without crashing regardless of the field name.

System

  • Operating System: Windows 10
  • Graphics Card: NVIDIA GeForce GTX 1050 Ti
@CoolSpy3 CoolSpy3 added bug Something isn't working crash Cause a crash labels Aug 17, 2024
CoolSpy3 added a commit to DeepBlueRobotics/webots that referenced this issue Aug 17, 2024
CoolSpy3 added a commit that referenced this issue Sep 7, 2024
* fix typo

* send proto ancestry to controllers

* expose proto ancestry in controller api

* send proto ancestry in all relevant messages

* relocate proto data to WbProto

* rename WbProto to WbNodeProtoInfo

* revert last two commits

* keep track of internal proto fields

* basic supervisor proto implementation

* swap meanings of wb_supervisor_node_get_field and wb_supervisor_node_get_parameter

* use an array list rather than a linked list for keeping track of node proto parents

* allow supervisor to query protos from Webots

* use correct read_only behavior

* fix get_proto api

* update supervisor/Webots field query api

* fix Makefile

* fix return reference to temporary

* supervisor fixes

* more supervisor fixes

* fix variable name

* fix field get value impl

* fix controller field lookup

* fix get number of fields/parameters

* fix function name

* fix wb_supervisor_proto_get_parent

* better handling of hidden parameters

* remove wb_supervisor_node_get_proto_ancestor

* revert 30953fc...01e067d

* remove accidental deletion

* fix indentation

* forward proto field lookups to the actual parameters

* fix typo

* update supervisor_notify_import_remove test

* null safety

* add proto tests

* fix C_SUPERVISOR_FIELD_GET_FROM_NAME implementation

* test fixes

* update supervisor_proto test

* fix supervisor_proto_fields compilation errors

* fix proto import paths

* fix bracket type

* fix memory leak

* remove unused SolidProtoHierarchy.children field (#6630)

* add additional log info

* bugfix to previous commit

* step the simulation after setting field values

* fix null return value in wb_supervisor_proto_get_number_of_parameters

* fix actual parameter lookup in proto parameter lookup functions

* use wb_supervisor_node_get_from_proto_def to retrieve the internal node

* fix def name

* fix proto names in test

* initialize struct members to NULL

* initialize node proto_info to NULL

* update expected proto hierarchy

* increment loop counter

* alternate method of determining the actual parameter of a field

* initialize field to NULL

* fix SolidProtoHierarchyBase

* use correct parameter retrieval method

* use internal=true for proto parameter lookups

* fix bug when retrieving the first field of a proto

* fix base node field query

* refresh internal node reference after regeneration

* fix internal_proto nullity check

* fix base node field lookup error messages

* fix retrieval of internal node fields

* don't expect changes in read-only fields

* use correct check value in read-only assertion

* don't check non-existent parameters

* fix array in if condition

* add wb_supervisor_proto_is_derived

* fix wb_proto_is_derived

* update c++ api

* fix variable name conflict

* add Node::getProto and update style

* update java api

* add todo

* update python api

* update the matlab api

* use camelCase in python api

* add wb_supervisor_proto_is_derived to python and matlab apis

* update WbLanguage.cpp

* rename methods for clarity

* fix circular dependency

* fix other circular dependency

* unset allow_search_in_proto

* swap field read only logic

* update proto_get_parent method name

* update references to renamed supervisor methods

* update ros api

* run clang-format

* fix cppcheck warnings

* fix Controller.def method names

* fix function name

* python formatting

* ros fixes

* run clang-format

* more ros fixes

* pin empy version

* bump webots_ros

* bump webots_ros

* bump webots_ros

* bump webots_ros

* fix lua syntax

* account for proto regeneration in test suite

* verify field integrity before checking lookup_field

* run clang-format

* don't return invalid proto references

* fix FieldImportBase proto definition

* fix race condition (see long commit message)
Before this commit, the code would update one of the fields, wait one timestep, and verify the change while updating the next field (two separate controllers). However, because the field updates regenerated the node, this led some references used in the check to be invalidated during the check. This was not an issue before, because no internal fields were used in the previous version of the test (so nothing was invalidated). This commit adds an additional step to each field update so that the check gets its own timestep. This means that the node is no longer regenerated while its being validated.

* use snake_case in supervisor_notify_import_remove_mf

* get rid of FieldImportBase

* use spaces instead of tabs

* cleanup proto map

* run clang-format

* remove const for consistency

* make wb_supervisor_proto_get_type_name(NULL) return "" for consistency

* update documentation

* fix references to old proto field access methods

* run clang-format

* fix proto regeneration check

* update the changelog

* bug fixes

* always get fields by name

* run clang-format

* remove explicit field name array size

* Bump actions/download-artifact from 3 to 4.1.7 in /.github/workflows (#6652)

Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 3 to 4.1.7.
- [Release notes](https://github.com/actions/download-artifact/releases)
- [Commits](actions/download-artifact@v3...v4.1.7)

---
updated-dependencies:
- dependency-name: actions/download-artifact
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* add comment

* Bump actions/upload-artifact to v4 (#6654)

* include proto id in field query when processing value get requests

* check for proto parent validity instead of just NULL
(this shouldn't be necessary because if the parent ref is invalidated, the child should've been as well, but it's good practice)

* fix variable name

* add changelog entry for matlab method signature change

* fix test assert message

* formatting

* fix types in supervisor documentation

Co-authored-by: Olivier Michel <[email protected]>

* add missing references to wb_supervisor_node_get_proto

* fix ros service ids in documentation

* add wb_supervisor_node_get_proto to Controller.def

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Dean Brettle <[email protected]>
Co-authored-by: Olivier Michel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working crash Cause a crash
Development

No branches or pull requests

1 participant