implement dynamic config and check for max-widget-tree-depth recursion detection

[pmario]

This PR implements a dynamic config and check for max-widget-tree-depth recursion detection

There are 3 new js-macros that allow us to modify the widget local variables.

This method is significantly faster, than the first approach. We do not have to check variable validity everytime we use the variable. The validation can be done at write time.

• <<parent-ancestorcount>> ... read only ... reads the widgets this.parentWidget.ancestorCount().
• <<get_UNSAFE_max_widget_tree_depth>> ... read only ... reads the widget this.parentWidget.UNSAFE_max_widget_tree_depth for debugging
• <<set_UNSAFE_max_widget_tree_depth>> ... Allows us to set the UNSAFE_max_widget_tree_depth variable.
  • minimum value is 100, IMO any lower value will completely remove the UI if used in eg: the PageTemplate

get- and set_UNSAFE_max_widget_tree_depth should be part of the "Hidden Settings" elements. They should not be touched by standard users.

The first iteration caused a significant performance hit

There is a new tv- variable in widget.js

• tv-UNSAFE-max-widget-tree-depth - which may need a better name

There are new tv- variables in button.js

• tv-is-button ... set to "true" if the widget is a button
• tv-widget-ancestors ... variable for debugging

tv-UNSAFE-max-widget-tree-depth is set to a relative value, which allows us to dynamically limit the number of nested widgets inside the button widget. defaults to "50" atm see: MAX_WIDGET_TREE_DEPTH_RELATIVE

The Vercel CI created wiki also contains 2 test tiddlers for easy experiments.

@Jermolene - What do you think.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
tiddlywiki5	✅ Ready (Inspect)	Visit Preview	Apr 25, 2024 4:16pm

[Jermolene]

Hi @pmario I think I get the idea of driving the recursion limit from a variable, but I don't understand why there needs to be special treatment for the button widget?

[pmario]

I did just fix the tests. Commit is on the way.

... but I don't understand why there needs to be special treatment for the button widget?

Because the button widget causes a problem and we know, buttons should not contain an other +900 nested widgets. IMO 50 or 100 should do.

From: #8144

Found and tested with single file wiki on Windows 10 with TiddlyDesktop and the Brave browser (both Chromium-based)

TiddlyDesktop immediately crashes to desktop

Browser zoom-level at 100%, it looks like this, with no good way to scroll the button into view. If possible at all.

The dynamic limit 50 looks as follows. IMO even mobiles should be able to handle this.

FireFox on ubuntu 22.04 has a really hard time. It is completely bricked for a long time. At the end it shows the browser native RSOD with "max recursion reached". If the close button is clicked it crashes to desktop. (I did send several crash-reports to Mozilla. Not sure if they can/will do anything about it.)

[pmario]

TiddlyDesktop can handle the new button-recurson limit just fine

[Jermolene]

OK @pmario so the idea is that the impact of recursively nesting button widgets is so severe that it is worth making special arrangements to limit it.

That makes me wonder if an alternate approach might not be to simply suppress rendering nested button widgets? The HTML spec is clear that interactive content is not allowed within buttons.

[pmario]

That makes me wonder if an alternate approach might not be to simply suppress rendering nested button widgets? The HTML spec is clear that interactive content is not allowed within buttons.

I'll have a closer look.

[pmario]

OK. It says: There must be no interactive content. So we can block it.

[pmario]

I did update the OP with the new tv- variable names.

The button "recursion detection" looks like this now

[pmario]

@Jermolene ... I did update the OP with

This PR implements a dynamic config and check for max-widget-tree-depth recursion detection

There are 3 new js-macros that allow us to modify the widget local variables.

This method is significantly faster, than the first approach. We do not have to check variable validity everytime we use the variable. The validation can be done at write time.

• <<parent-ancestorcount>> ... read only ... reads the widgets this.parentWidget.ancestorCount().
• <<get_UNSAFE_max_widget_tree_depth>> ... read only ... reads the widget this.parentWidget.UNSAFE_max_widget_tree_depth for debugging
• <<set_UNSAFE_max_widget_tree_depth>> ... Allows us to set the UNSAFE_max_widget_tree_depth variable.
• minimum value is 100, IMO any lower value will completely remove the UI if used in eg: the PageTemplate

get- and set_UNSAFE_max_widget_tree_depth should be part of the "Hidden Settings" elements. They should not be touched by standard users.

just to let you know.

[Jermolene]

Thanks @pmario, I appreciate your work on this. It is very helpful to have concrete code to help us explore the options.

I remain fundamentally sceptical of the way that this PR focuses on the button widget. I am concerned that this is only because of the specifics of the bug report that prompted this PR. We know that the problems with recursion are widespread because they are fundamental to the design of the rendering process. We have no way of knowing whether the case with the button is just one of many manifestations of the problem, or whether there are many more undiscovered scenarios that lead to even worse behaviour.

I appreciate that the introduction of the UNSAFE_max_widget_tree_depth variable may be a useful starting point for other fixes, perhaps more subtle ones. But it also comes at a considerable cost: it exposes another internal implementation detail in a way that may limit us in the future due to backwards compatibility constraints. Also, while you have demonstrated that it can have a role in fixing problems, it is also likely to introduce new problems due to mistakes. It's asking a lot of users to understand what it does conceptually. I worry that we'd end up with a situation like the wikify widget where users accumulate superstitions and supposition.

Stepping back, perhaps the problem is that we currently measure the overall depth of the widget tree. However, all the evidence is that what actually matters is the depth of the DOM tree. I'd be interested to think through the implications of adding widget.getAncestorCountDOM().

[pmario]

I remain fundamentally sceptical of the way that this PR focuses on the button widget.

I'm sorry I should have removed the button handling from this PR, because it got it's own PR already. But I wanted to test the stuff together to see if there are any side effects. So this PR should be viewed independent from the button problem.

Stepping back, perhaps the problem is that we currently measure the overall depth of the widget tree. However, all the evidence is that what actually matters is the depth of the DOM tree. I'd be interested to think through the implications of adding widget.getAncestorCountDOM().

Not necessarily. For recursive functions like the collatz which is part of the Vercel preview (for testing) the dom is not the problem. It's the number of the recursions which creates nested widgets.

So the DOM is not deep here. But the widget depth is 1583 max. The yellow number is the ancestorCount

Depth image from the DOM

[pmario]

Stepping back, perhaps the problem is that we currently measure the overall depth of the widget tree. However, all the evidence is that what actually matters is the depth of the DOM tree. I'd be interested to think through the implications of adding widget.getAncestorCountDOM().

I came back to this one and analysing the DOM screenshot. I think you are right. If we have a deep widget-tree, that also creates a deep DOM-tree there is a high chance that browsers get a problem.

A deep widget tree alone is not necessarily a problem. It could be intended. The only problem with a deep widget-tree alone is a) {{}} or b) <$transclude/>

Where a) can be handled with the wikitext parser and b) has to be limited by MAX_WIDGET_TREE_DEPTH.

If we add widget.getAncestorCountDOM() we could limit MAX_DOM_DEPTH to 200 or 300, which should be enough evidence that there is something wrong. Browsers should be able to handle that and we could set the default limit for the widget-tree much higher.

I'll have a closer look at widget.getAncestorCountDOM()

[Jermolene]

Thanks @pmario

[pmario]

@Jermolene -- There is a ancestor dom count: <<parent-ancestorcount-dom>> macro now, that can read the DOM parents up to the <div class="tc-page-container ..."> which is the 4th div and it is created at startup. So it cannot be counted.

The maximum DOM depth from the TW UI in "view mode" imo is about 30 in the right sidebar -> Search input

The editor toolbar buttons have about 20, similar to the "menu-bar plugin" search text input.

So I did set the MAX_DOM_TREE_DEPTH = 100 in widget.js - That should be plenty room for plugin authors.

I did test all the changes with TiddlyDesktop, which also survives the test tiddlers in the Vercel preview edition.

---

The test-collatz tiddler uses <<set_UNSAFE_max_widget_tree_depth 5000>> which works with FF, TD and Edge - But this is already near the limit of the browser stack.

Edge did throw a stak-error with 5500 :/

---

There is still one problem: <$transclusion> <$transclusion> which will need the PR from @flibbles

• Introduced preliminary idea for infinite recurse exception #7882

IMO we will need a combination of flibbles's PR and this PR: <$button><$transclude/> is a problem with flibbles's version since it draws 1000 buttons, which crashes TDesktop

---

For this draft PR I did add a \define tv-limit-nested-buttons() yes which is "no" by default. It allows me to limit button nesting to 3 buttons.

The tiddler: test-recursive-buttons is way to "destructive" for my taste, so I would like to have a possibility to limit it.

have fun!
-mario

[pmario]

Also see: #8155 (comment)