naturalcrit / homebrewery

Create authentic looking D&D homebrews using only markdown
https://homebrewery.naturalcrit.com
MIT License
1.1k stars 327 forks source link

Markdown parser freezes on particular .wide,.toc combo #2686

Closed calculuschild closed 1 year ago

calculuschild commented 1 year ago

Renderer

v3

Browser

Chrome

Operating System

Windows

What happened?

A couple users have reported the browser freezing when editing a table of contents in V3. Minimal example here:

{{wide,toc
- Hello
    - 
}}

Attempting to add a # after the second bullet point will cause the browser to freeze. This only seems to occur if both .wide and .toc classes are set. Similarly:

{{wide,to
- Hello
    - #
}}

Completing the toc above will cause this to crash.

G-Ambatte commented 1 year ago

I have been able to reproduce the issue on the live site using the following text:

{{wide,toc
- Hello
  - #
}}

From my testing, it looks like the website freezes up for about 42 seconds, give or take.

From this code, I created the following test case:

/* eslint-disable max-lines */

const Markdown = require('naturalcrit/markdown.js');

test('Check for wide ToC crash', function() {
    const source = '{{wide,toc\n- Hello\n  - #\n}}';
    const rendered = Markdown.render(source);
    expect(rendered).toMatch('<div class=\"block wide toc\"  ><ul>\n<li>Hello<ul>\n<li><h1 id=\"\"></h1>\n</li>\n</ul>\n</li>\n</ul>\n</div>');
});

This test completes in ~20ms. Modifying it to include text in the H1 list element did not change the test duration.

From these results, I can only conclude that the issue is NOT in Markdown, but rather some other issue causing the freeze.


I will note here that after the ~42s delay, the H1 element does not appear on the page. Locating it using the Inspection tool shows that it is well off the end of the page; my best guess at the moment is that it is wider than the class='wide' element, so is pushed off the page entirely.

calculuschild commented 1 year ago

Testing this step by step through the code, I also see that it generated the Markdown correctly, but then has an issue when placing the column break div at the end of the page (each page gets a column break added artificially to make the columns work properly.) Not sure exactly why that breaks this particular case though.

G-Ambatte commented 1 year ago

Working on the theory that the h1 element's column-span: all inside the wide element that is what's causing things to choke, I added the following to the Style Editor:

.page .toc h1 {
    column-span: inherit;
}

So far, my testing shows that the ~42s delay is completely removed.

If this correct, we might be able to resolve the issue by making the column-span styling more specific, so it breaks once the h1 element is no longer a direct descendant of .page (or .page .columnWrapper).

G-Ambatte commented 1 year ago

I've put together an example brew (https://homebrewery.naturalcrit.com/share/mwEFvx4n4Yct), in which:

This brew does not exhibit the ~42 second delay when the H1 header in the Table of Contents is changed.


CSS:

.page h1 {
    column-span: inherit;
    -webkit-column-span: inherit;
}

.page > .columnWrapper > h1 {
    column-span: all;
    -webkit-column-span: all;
}
5e-Cleric commented 1 year ago

Opera GX user mentions that having this issue in their new brew blocks the ability to save and create new brews, this would move this issue to a P0, anyone able to confirm?

calculuschild commented 1 year ago

If this correct, we might be able to resolve the issue by making the column-span styling more specific, so it breaks once the h1 element is no longer a direct descendant of .page (or .page .columnWrapper).

This might hide the issue, we still need to troubleshoot why this situation causes a crash to occur in the first place, because CSS on its own shouldn't cause crashes. There is a deeper bug in our brew preview generation that it can't handle certain inputs.

At worst, we should have garbage in/garbage out. Not garbage in/crash.

G-Ambatte commented 1 year ago

Testing this step by step through the code, I also see that it generated the Markdown correctly, but then has an issue when placing the column break div at the end of the page (each page gets a column break added artificially to make the columns work properly.) Not sure exactly why that breaks this particular case though.

I was poking at this issue again last night - I removed the artificial column break completely by commenting out brewRenderer.jsx:L146 and confirmed that it no longer appears in the page structure at all:

image

But when entering the reproduction text, the delay still occurs. I think that this means that we can eliminate the column split as the cause of the delay.

G-Ambatte commented 1 year ago

Attempting to investigate this again in the last few days, I've found that the delay is now absent when following the reproduction steps. Can anyone else confirm?

Running Chrome Version 117.0.5938.132 (Official Build) (64-bit) for Win 10 x64.

calculuschild commented 1 year ago

Yup, looks like this was a Chrome issue. Testing on Chrome 110 from last Feb has the issue, but on the latest version it works fine.