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.

Sign up for GitHub

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

Jump to bottom

Fix: regex execution Maximum regex stack depth reached error #23774

Merged
merged 1 commit into from
Aug 2, 2023
Merged

Fix: regex execution Maximum regex stack depth reached error #23774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
"babel-polyfill": "^6.26.0",
"dom-serializer": "^0.2.2",
"domhandler": "^4.3.0",
"expensify-common": "git+ssh://git@github.com/Expensify/expensify-common.git#98d8fea356f114f8b5b0cea889a41b355e5daf58",
"expensify-common": "git+ssh://git@github.com/Expensify/expensify-common.git#9940dd127c2d44809c98ee628a8057f08c93bfc9",
"fbjs": "^3.0.2",
"htmlparser2": "^7.2.0",
"jest-when": "^3.5.2",
Expand Down
10 changes: 9 additions & 1 deletion src/libs/Url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,15 @@ function addTrailingForwardSlash(url) {
*/
function getURLObject(href) {
const urlRegex = new RegExp(URL_WEBSITE_REGEX, 'gi');
const match = urlRegex.exec(href);
let match;
try {
if (!href.startsWith('mailto:')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not add this condition. I think a try/catch is sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it but I tend to add this improvement for the following reasons.

  1. The slow regex execution takes some time to throw the error, which means the UI will be blocked during the time.
  2. As we render the long email as anchor tag, so every time we open a chat includes a long email will result in a brief moment of UI block. Given that I think it's better to skip executing the regex for email here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@eh2077 I think a better idea would be to apply the regex only if the length is less than a specific limit (set for each platform). What do you think?

Copy link
Contributor Author

@eh2077 eh2077 Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@allroundexperts I think early return for the email case is enough to avoid executing slow regex matching in this method. We avoid parsing long link causes error to anchor tag in method ExpensiMark.replace, which means we will only possibly have long email.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not add a similar condition (which we use while parsing long link) here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we not add a similar condition (which we use while parsing long link) here as well?

I think we can't because we don't do domain(TLD) checking when parsing email in ExpensiMark

Copy link
Contributor

Choose a reason for hiding this comment

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

I might not be following you. Can you share a link of the condition you mentioned in your comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try to make it clear. There's no such similar condition for parsing link.
I mean only regex related to https://github.com/Expensify/expensify-common/blob/main/lib/Url.js causes the error and we catch it here https://github.com/Expensify/expensify-common/blob/56db2a0fc9df6b4270a99e4d3a9a7b0730ad2aa4/lib/ExpensiMark.js#L389. Note that the email regex https://github.com/Expensify/expensify-common/blob/56db2a0fc9df6b4270a99e4d3a9a7b0730ad2aa4/lib/CONST.jsx#L3 doesn't trigger the error(based testing), so ExpensiMark.replace will translate long email to anchor tag and this method will receive the long email of href parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Thanks!

match = urlRegex.exec(href);
}
} catch (e) {
// eslint-disable-next-line no-console
console.warn('Error parsing url in Url.getURLObject', {error: e});
}
if (!match) {
return {
href: undefined,
Expand Down
Loading