TiddlyWiki / TiddlyWiki5

A self-contained JavaScript wiki for the browser, Node.js, AWS Lambda etc.
https://tiddlywiki.com/
Other
8.01k stars 1.19k forks source link

[BUG] Crash on empty transclusion syntax inside button widget #8144

Open bluepenguindeveloper opened 6 months ago

bluepenguindeveloper commented 6 months ago

Describe the bug

Wikitext such as <$button>{{}}</$button> (or unclosed button <$button>{{}}) results in an immediate crash.

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

Expected behavior

Recursive transclusion error (without crashing TiddlyWiki)

To Reproduce

  1. Click Create New Tiddler button
  2. Enter <$button>{{}}</$button> or <$button>{{}} into the text field
  3. Turn on side by side preview
  4. Note that TiddlyWiki crashes

Steps 2 and 3 can be done in reverse order, in which case, the crash will occur as soon as the second } is typed if typing manually.

Screenshots

No response

TiddlyWiki Configuration

Additional context

No response

pmario commented 6 months ago

I can confirm the problem. With FF on Widows, I can still close the preview-area. So the wiki stays responsive.

FireFox on ubuntu 22.04 has a really hard time. It's close to impossible to get the wiki "unbricked"

@Jermolene

The current MAX_WIDGET_TREE_DEPTH inside a button-widget can cause data loss.

I do have an idea, which would allow us to create a "per-widget" max depth limit. Currently the code is simple.

Do you think it's worth to create a PR?

Jermolene commented 6 months ago

I've prepared a PR that takes a different approach and resolves this at the parsing level, I'll commit the changes shortly.

Jermolene commented 6 months ago

Thanks @bluepenguindeveloper @pmario I've prepared a fix over at #8145, please test it if you can.

pmario commented 6 months ago

@Jermolene My idea can be seen in the screenshot.

Since "button inside button inside a button ... " does not make much sense, we know that we should stop it early. In the screenshot it stops at ancestorCount() 334, which should not cause too many problems. But I would need to test this one with FF on ubuntu again.

image

The idea would need some refinement. Since I think "button in button" should only be allowed eg: 10 times for any weird usecase.

I think this.getAncestorCount() could be overwritten by the button-widget to detect a "button inside button" situation and handle that one individually.

Just some thoughts.

Jermolene commented 6 months ago

Hi @pmario in my tests, your example of <$button><$transclude> doesn't crash the browser if I reduce MAX_WIDGET_TREE_DEPTH to 300. So I wonder if a reasonable solution with your example isn't just to reduce that constant. I chose 1000 as a reasonable guess, but there is now evidence that it is too high.

pmario commented 6 months ago

I did not hardcode the constant. I used MAX_WIDGET_TREE_DEPTH - (this.getAncestorCount() * 2) which is not optimal. Just for testing.


There is a Talk thread Repeat expression (n) times? where Scott Sayet creates an interesting recursive function collatz(x) where our hadcoded limit 1000 actually creates a "false positive" check.

So I did suggest to create a possibility to actually "widen" the limit.

I think it should be possible to change the limit very specifically. My suggestion was a tv-max-widget-tree-depth variable. The docs for this variable may be in the Hidden Settings area of the tw-docs.

pmario commented 6 months ago

Here is the full code I used in the button.

this.setVariable("maxWidgetTreeDepth", MAX_WIDGET_TREE_DEPTH - this.getAncestorCount() * 2);

Where the widget check uses a "variable" and looks like this:

    if(this.getAncestorCount() > this.variables.maxWidgetTreeDepth.value) {
pmario commented 6 months ago

This screenshot is not possible at tiddlywiki.com atm. But it shows the possibility to dynamically raise the max widget tree depth and still use buttons within the higher limits.

The button-widget uses a dynamic maximum of +20 to the existing maximum.

The \procedure test uses a "local max" of 2000

This allows us to still use buttons inside the "test-collatz" example.

image