x3dom / x3dom

X3DOM. A framework for integrating and manipulating X3D scenes as HTML5/DOM elements.
http://x3dom.org
Other
815 stars 271 forks source link

Adding / Removing Transforms dynamically can crash #1200

Open eamanola opened 2 years ago

eamanola commented 2 years ago

Hi!

in nodes/Grouping/X3DTransformNode.js, line 168

there is this commented out check

 // if (this._parentNodes.length == 0) {

Adding and removing transforms to the DOM crashes for us, with Uncaught TypeError: this._nameSpace is null, if this check is commented out.

Might be useful: IIRC adding 3 transforms was ok, and forth one gave the error. Did not matter which of the 4 transforms was removed from the list.

full trace:

findX3DDoc http://localhost:4001/x3dom/x3dom.debug.js:38638

parentRemoved http://localhost:4001/x3dom/x3dom.debug.js:40645

parentRemoved http://localhost:4001/x3dom/x3dom.debug.js:38543

parentRemoved http://localhost:4001/x3dom/x3dom.debug.js:40660

parentRemoved http://localhost:4001/x3dom/x3dom.debug.js:38543

parentRemoved http://localhost:4001/x3dom/x3dom.debug.js:38543

parentRemoved http://localhost:4001/x3dom/x3dom.debug.js:40660

parentRemoved http://localhost:4001/x3dom/x3dom.debug.js:38543

removeChild http://localhost:4001/x3dom/x3dom.debug.js:38507

onNodeRemoved http://localhost:4001/x3dom/x3dom.debug.js:22111

onMutation http://localhost:4001/x3dom/x3dom.debug.js:22263

X3DDocument http://localhost:4001/x3dom/x3dom.debug.js:21368

load http://localhost:4001/x3dom/x3dom.debug.js:2040

onload http://localhost:4001/x3dom/x3dom.debug.js:20081

reload http://localhost:4001/x3dom/x3dom.debug.js:20144

Is it possible to remove the commenting in upstream & dists as well?

Thanks in advance.

andreasplesch commented 2 years ago

Could you please share a minimal scene to reproduce this ? codepen or jsitor works well.

I think the check - now commented out - had introduced a memory leak which prevented dynamically removing Transforms in Inlines properly and caused slowdown and crashes after a while. So it would be best to find the root cause why this._nameSpace is null for the fourth transform. Perhaps a timing issue ?

Could you please see if a more specific guard would work for your case ?

if ( this._nameSpace ) {
...
}

Thanks.

eamanola commented 2 years ago

Hi Andreas,

Thanks for getting back.

Unfortunately, i could not reproduce the problem in a minimal scene.

if ( this._nameSpace ) { does help, and behaves in the same way as if (this._parentNodes.length == 0) {.

PS. You mentioned timing, and I did a monkey test. In case its useful: while no guard will reproduce the issue on every click, having either of the guards, and triggering the event fast enough (again manual mouse clicks) consecutively, will result in the same error, with slightly different trace:

findX3DDoc http://localhost:4000/x3dom/x3dom.debug.js:38638

onNodeRemoved http://localhost:4000/x3dom/x3dom.debug.js:22102

onMutation http://localhost:4000/x3dom/x3dom.debug.js:22263

X3DDocument http://localhost:4000/x3dom/x3dom.debug.js:21368
eamanola commented 2 years ago

while the problem is not reproduced, the basic idea is here:

https://jsitor.com/aZi9uwnB4

There is a large model (50-100MB), and based on user interaction, some elements, mostly Groups or Transforms, are either wrapped in a Transform and translated, or a Transform wrap is removed.

andreasplesch commented 2 years ago

Thanks. Glad it works in the example. Do you think the size of the model is partially responsible ? Do you use Inlines ? I suppose it is not possible to look at the model directly ?

eamanola commented 2 years ago

Unfortunately I cannot share the model, nda and all, however I assume any large model will do.

We don't use inlines. (could not figure out, how to access elements within the model using inline, any hints are very welcome).

The size of the model seems relevant. When moving elements (adding/removing the transforms) there is a acceptable and noticeable lag, say 500 ms, before the changes show on screen. If the user moves another element during this time, the second case (timing related above) happens.

PS Could be that, the magic 4 transforms is a special case of this too, can't say.

andreasplesch commented 2 years ago

https://doc.x3dom.org/tutorials/models/inline/index.html

shows how using nameSpaceName allows access to the loaded Inline. The content is then available in the dom as childNodes of the Inline. The double underscore stuff is not really necessary. I think that is from before .querySelector was widely implemented.

I could not reproduce even with large models in Transforms.

eamanola commented 2 years ago

https://doc.x3dom.org/tutorials/models/inline/index.html

Remember reading this, but somehow couldn't make it work. Will put it on backlog for next versions. It would make things simpler.

Not sure what is exactly the deal with the null. Wish I could provide more insight into it. It is 100% reproducible with 4 transforms, against no guards, on any of the models we have.

andreasplesch commented 2 years ago

Ok. Not sure where to go without a repro. Let's just leave this issue open as a reminder. The dev version should update by tomorrow.

eamanola commented 2 years ago

thanks!