Repository navigation
fix(core): stop the chat link regex backtracking on long words - #1499
ANDROIDHASSAN wants to merge 2 commits into
Conversation
|
@ANDROIDHASSAN is attempting to deploy a commit to the LiveKit Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: 90c389b The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| for (const text of ['a'.repeat(3000), 'x:'.repeat(1500)]) { | ||
| const start = performance.now(); | ||
| expect(text.match(createUrlRegExp({}))).toBeNull(); | ||
| expect(performance.now() - start).toBeLessThan(1000); |
There was a problem hiding this comment.
thought: How reliable will this test be across different environments? Is there a different way to test this?
I know 1000 is meant to be very high, but I bring it up because we've run into situations in the past where slow github actions ci workers can cause tests to take much longer than they would normally in the past, and in general I worry about a limit like this resulting in a flaky test when the github actions worker is busy and the job is unable to get a lot of cpu time.
There was a problem hiding this comment.
Fair point. I measured both on a heavily loaded machine (load average ~14): 'a'.repeat(3000) takes 17.6 s with the old regex and 50 ms with the new one; 'x:'.repeat(1500) takes 8.6 s and 17 ms. I raised the bound to 5000 ms in 90c389b, so a starved worker has ~70x headroom and main still fails clearly. JS gives no step counter for a RegExp, so a time bound is the only check I found. Happy to drop the test if you'd prefer.
Summary
ChatEntryruns every received chat message throughcreateDefaultGrammar(), which usescreateUrlRegExp.The
hostanddomainparts are written as(?:x[-_]*)*x+, so a run of letters can be split between the two quantifiers in many ways.On a long word with no dot in it the regex tries all of them before giving up, and the page freezes while it does.
Measured on
mainwith Node 26,text.match(createUrlRegExp({})):'a'.repeat(1000)'a'.repeat(4000)'x:'.repeat(1000)Any participant can send such a message, and every other participant's tab hangs while rendering it.
The fix writes each label as
x+(?:[-_]+x+)*, which matches the same strings but can split them only one way.(?:\S+(?::\S*)?@)?becomes(?:\S+@)?::is already in\S, so the optional group only added more ways to split.Testing
'x:'.repeat(1500)match nothing and finish in under a second. Onmainit fails (expected 18383.37 to be less than 1000); with this changenpx vitest run src/helperpasses, 29 tests.- _ . : / @ ? # ( ) ,,é,www.andhttp://, withexactboth off and on: 0 differences.Changeset
Patch for
@livekit/components-core.