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

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

Open
bluepenguindeveloper opened this issue Apr 15, 2024 · 8 comments · May be fixed by #8155
Open

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

bluepenguindeveloper opened this issue Apr 15, 2024 · 8 comments · May be fixed by #8155

Comments

@bluepenguindeveloper
Copy link

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

  • TW Version [e.g. v5.3.3]

  • Saving mechanism: TiddlyDesktop, or none (occurs if opening single file wiki in the browser (Brave), and can be done in the TiddlyWiki documentation simply viewed in the browser); seems to be independent of saving mechanism

  • OS: Windows 10

  • Browser: Brave (Version 1.64.116; Chromium: 123.0.6312.105)

Additional context

No response

@pmario
Copy link
Contributor

pmario commented Apr 15, 2024

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
Copy link
Owner

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

@Jermolene
Copy link
Owner

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

@pmario
Copy link
Contributor

pmario commented Apr 15, 2024

@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
Copy link
Owner

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
Copy link
Contributor

pmario commented Apr 15, 2024

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
Copy link
Contributor

pmario commented Apr 15, 2024

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
Copy link
Contributor

pmario commented Apr 15, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants