Comprehensive fix for memory leak in highlight-matches component#2746
Comprehensive fix for memory leak in highlight-matches component#2746dimaMachina merged 3 commits intoshuding:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 2dd27e8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@jaem1n207 is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
A critical update has been made to prevent potential infinite loops that could occur in certain scenarios, such as when a match is followed by two spaces and a character.
4ea167b to
21f13ec
Compare
| } | ||
| const splitText = value.split('') | ||
| const escapedSearch = escapeStringRegexp(match.trim()) | ||
| const regexp = new RegExp(escapedSearch.replaceAll(' ', '|'), 'ig') |
There was a problem hiding this comment.
thank you for reporting the new issue with the memory leak, I am wondering if it is enough to just replace multiple whitespaces in this place?
const regexp = new RegExp(escapedSearch.replaceAll(/\s+/g, '|'), 'ig')There was a problem hiding this comment.
Thank you for your feedback!
Given that highlight-matches.tsx is working under the assumption that a search term has been entered, replacing multiple whitespaces is indeed sufficient to address the memory leak issue.
I've updated the PR to reflect this change.
| while ( | ||
| (result = regexp.exec(value)) && | ||
| // case `> ` replaced previously to `>||` + some character provoke memory leak because | ||
| // `RegExp#exec` will always return a match | ||
| regexp.lastIndex !== 0 | ||
| ) { | ||
| const before = splitText.splice(0, result.index - index).join('') | ||
| const after = splitText.splice(0, regexp.lastIndex - result.index).join('') | ||
| content.push( | ||
| before, | ||
| <span key={result.index} className="nx-text-primary-600"> | ||
| {after} | ||
| </span> | ||
| ) | ||
| index = regexp.lastIndex | ||
| while ((result = regexp.exec(value))) { | ||
| if (result.index === regexp.lastIndex) { | ||
| regexp.lastIndex++ | ||
| } else { |
There was a problem hiding this comment.
just curious, behaviour should be the same as previously?
result.index === regexp.lastIndex only when lastIndex = 0?
So you increment lastIndex to exit loop?
There was a problem hiding this comment.
The result.index === regexp.lastIndex condition in the highlight-matches component serves as a safeguard against infinite loops that can occur when the regular expression matches an empty string. Here's a simple example to illustrate this:
const value = 'Docs Theme';
const match = ' ';
const escapedSearch = escapeStringRegexp(match.trim());
const regexp = new RegExp(escapedSearch.replaceAll(/\s+/g, '|'), 'ig'); // -> /(?:)/gi
let result;
while ((result = regexp.exec(value))) {
if (result.index === regexp.lastIndex) {
// lastIndex: 0, 1, 2...10. (value.length === 10)
// This condition is true when the regex matches an empty string.
// Without incrementing lastIndex, the loop would be infinite.
regexp.lastIndex++;
} else {
// Process the match...
}
}In this example, the regex /(?:)/gi matches an empty string, and without incrementing regexp.lastIndex, the exec method would keep matching at the same position, causing an infinite loop.
However, the condition to increment lastIndex is not currently necessary in thehighlight-matches component. This is because, in the flexsearch component that utilizes highlight-matches, if search is an empty or whitespace-only string, highlight-matches is not invoked. Therefore, the match in highlight-matches will never be assigned a value that could trigger an infinite loop, and the condition can be safely removed.
Nonetheless, the result.index === regexp.lastIndex condition was included as a precaution to ensure that highlight-matches can handle all inputs without falling into an infinite loop, even in scenarios where it is not used within flexsearch component.
dimaMachina
left a comment
There was a problem hiding this comment.
Huge thanks for your fix 🙏
…y the insertion order (#2759) * fix: Invalid webkit style (#2750) * fix: Invalid webkit style "-webkit-background-clip" is an invalid style for JSX, this commit switches this to the "WebkitBackgroundClip" style. * refactor: Remove webkit styling * Comprehensive fix for memory leak in highlight-matches component (#2746) * fix: resolve memory leak issue in highlight-matches component A critical update has been made to prevent potential infinite loops that could occur in certain scenarios, such as when a match is followed by two spaces and a character. * refactor: apply simplified regex approach to resolve memory leak * add changeset --------- Co-authored-by: Dimitri POSTOLOV <[email protected]> * Version Packages (#2752) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * add changeset * update snapshots --------- Co-authored-by: Jun Xiang <[email protected]> Co-authored-by: 이재민 <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This PR provides an additional fix for the memory leak issue in the
highlight-matches.tsxcomponent that was previously addressed in #2152. While the initial fix resolved certain scenarios, it appears that a memory leak can still occur under specific conditions as I have recently discovered.Background and Context
The memory leak issue was initially reported in issue #2150 and a subsequent fix was proposed in PR #2152. while this fix aimed to address this, but it seems the problem persists in certain edge cases.
Continuation of Efforts
Building on the efforts of PR #2152, I have conducted further investigation and identified that the memory leak persists under specific conditions. To clearly demonstrate the issue, I have attached a video showing the memory leak with the current implementation.
Bug Reproduction Video:
memory-leak-reproduction.mov
New Solution
The proposed changes introduce a
processSearchTermfunction that robustly handles search terms by splitting, escaping, and joining them with an OR condition (|). This approach not only covers the previously unhandled edge cases but also prevents potential infinite loops inRegExp#execby manually incrementinglastIndexwhen necessary.Post-Fix Demonstration Video:
resolved-memory-leak.mov
Key Changes
processSearchTermfunction for improved search term handling.RegExp#execloop to prevent infinite iterations and ensure proper indexing.I believe the additional changes proposed in this PR will completely resolve the memory leak issue.
Testing and Verification
These modifications have been thoroughly tested in the development environment, and the issue could not be reproduced post-change. I anticipate that this fix will contribute positively to the stability and performance of the project's search functionality. The attached videos serve as a testament to the fix's effectiveness.
Request for Review
I respectfully request your review of these changes. Your insights and feedback are highly valued, and I am open to any further suggestions or discussions to refine the solution.
Thank you for considering this contribution.