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

Fix crash when class regex matches an empty string #897

Merged
merged 9 commits into from
Jan 17, 2024

Conversation

thecrypticace
Copy link
Contributor

This removes becke-ch--regex--s0-0-v1--base--pl--lib in lieu of native support for capture group indices. This is supported in Node 16+ so will work in VSCode — additionally all modern browsers have supported this since late September 2021 (Safari 15, Chrome/Edge 90, Opera 76, FF 88) so this shouldn't be an issue for Tailwind Play either.

As part of this change I've also done some refactoring to reduce duplication in code that used the above package — and added more tests!

Fixes #893

Copy link

@mfb-davidmay mfb-davidmay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grateful for this fix, it improves the readability of text processing method substantially.

One minor end user comment I have is could we get some kind of log to signify the start and end of file processing? I only ask because it's hard to determine from the VSCode Ext Output what the extension is doing after initialising.

@thecrypticace
Copy link
Contributor Author

@mfb-davidmay I think that's worth doing but as a separate thing. Adding logging here could be quite noisy as it's used during editing when triggering completions.

@thecrypticace
Copy link
Contributor Author

thecrypticace commented Jan 5, 2024

Bit of a stopper at the moment — running this with the regexes:

/tv\((([^()]*|\([^()]*\))*)\)/gd
/["'`]([^"'`]*).*?["'`]/gd

Results in what looks to be an infinite loop in v8. It'll proceed after exec() a handful of times and then it'll all of a sudden stop working and exec never returns.

Running a simple profile against the process looks like its spending time inside v8::internal::RegExpMacroAssembler::LoadCurrentCharacter

It happens reliably in my environment:

Version: 1.85.1 (Universal)
Commit: 0ee08df0cf4527e40edc9aa28f4b5bd38bbff2b2
Date: 2023-12-13T09:48:06.308Z (3 wks ago)
Electron: 25.9.7
ElectronBuildId: 25551756
Chromium: 114.0.5735.289
Node.js: 18.15.0
V8: 11.4.183.29-electron.0
OS: Darwin arm64 23.2.0

I need to test this against a similar node.js / v8 version to see if I can reproduce it there.

@thecrypticace
Copy link
Contributor Author

Okay I guess I wasn't paying too close attention to the regex but it has nested * specifiers which means that it's subject to catastrophic backtracking.

Take the regex:
/tv\((([^()]*|\([^()]*\))*)\)/gd

If you look closely you can see an external group with * and, inside that group, another one that uses * too. This means that the internal group can match zero or more times AND the external group can.

Here is a simplified regex with the same situation — but less capture groups and alternations:
/v\(((?:[^()]*)*)\)/gd

On a regex like this it takes almost 20s to match the string v({ foobar: '' // aaaaaaaaaaaaaaa on my M3 Max. And this is exponentially worse for every character added on the order of 2^n. At only 79 as it'd take my computer nearly 1,000x longer than the current age of the universe to compute. I think thats a little longer than I'd be willing to wait.

All in all, this isn't a bug, just a regex that's going to take quite literally forever if it doesn't immediately find a match.

That means this should be safe to merge. 😀

@thecrypticace thecrypticace force-pushed the fix-memory-crash-class-regex branch 2 times, most recently from 48d99b4 to d96c3a2 Compare January 17, 2024 18:25
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 this pull request may close these issues.

JavaScript heap out of memory when using tailwindCSS.experimental.classRegex
2 participants