-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
report: avoid really slow regexes for long urls #14745
Conversation
@@ -188,6 +188,8 @@ class Util { | |||
|
|||
const MAX_LENGTH = 64; | |||
if (parsedUrl.protocol !== 'data:') { | |||
// Even non-data uris can be 10k characters long. | |||
name = name.slice(0, 200); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for correctness within this function, this should 1) detect if string is greater than 200 characters and 2) if so, slice and append ELLIPSIS
otherwise we can show a cutoff URL without any indication we cut it off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps this just works if you swap L214-223 to be first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind, since this is taking the first 200 characters and later we trim to 64 this will never impact the result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add some tests for very long non-data urls?
@@ -188,6 +188,8 @@ class Util { | |||
|
|||
const MAX_LENGTH = 64; | |||
if (parsedUrl.protocol !== 'data:') { | |||
// Even non-data uris can be 10k characters long. | |||
name = name.slice(0, 200); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind, since this is taking the first 200 characters and later we trim to 64 this will never impact the result
I was looking at report perf and this regex stood out:
We already ran into this in #13785 (and fixed by #13791)
But.. theverge has a non-datauri URL that's 10kb long. and this nasty regex (my fault apparently) really doesn't enjoy working through such a big string.