Optimize method of stripping validation response of affixed HTML comments

[pierlon]

Summary

Fixes #6011

Checklist

• My code is tested and passes existing tests.
• My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

[codecov]

Codecov Report

Merging #6021 (219a7ef) into develop (d955987) will increase coverage by 0.00%.
The diff coverage is 88.88%.

❗ Current head 219a7ef differs from pull request most recent head b222c5d. Consider uploading reports for the commit b222c5d to get more accurate results

@@            Coverage Diff             @@
##             develop    #6021   +/-   ##
==========================================
  Coverage      75.26%   75.27%           
- Complexity      5703     5709    +6     
==========================================
  Files            218      218           
  Lines          17275    17283    +8     
==========================================
+ Hits           13002    13009    +7     
- Misses          4273     4274    +1     

Flag	Coverage Δ	Complexity Δ
javascript	80.05% <ø> (ø)	0.00 <ø> (ø)
php	75.06% <88.88%> (+<0.01%)	0.00 <0.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...s/amp-validation-status/revalidate-notification.js	80.00% <ø> (ø)	0.00 <0.00> (?)
...nents/amp-validation-status/status-notification.js	100.00% <ø> (ø)	0.00 <0.00> (?)
...block-validation/components/error/error-content.js	93.54% <ø> (ø)	0.00 <0.00> (?)
...k-validation/components/error/error-panel-title.js	100.00% <ø> (ø)	0.00 <0.00> (?)
...ock-validation/components/error/error-type-icon.js	100.00% <ø> (ø)	0.00 <0.00> (?)
...idation/components/error/get-error-source-title.js	93.75% <ø> (ø)	0.00 <0.00> (?)
...ets/src/block-validation/components/error/index.js	100.00% <ø> (ø)	0.00 <0.00> (?)
...sets/src/block-validation/components/icon/index.js	100.00% <ø> (ø)	0.00 <0.00> (?)
...alidation/components/sidebar-notification/index.js	100.00% <ø> (ø)	0.00 <0.00> (?)
...dation/components/with-amp-toolbar-button/index.js	100.00% <ø> (ø)	0.00 <0.00> (?)
... and 5 more

[github-actions]

Plugin builds for b222c5d are ready 🛎️!

• Download development build
• Download production build

[westonruter]

The removal of the s will cause a problem in that it will not match a comment like this:

…
</body></html>
<!--
	generated 2 seconds ago
	generated in 1.134 seconds
	served from batcache in 0.003 seconds
	expires in 298 seconds
-->

I've added a failing test case to demonstrate this: 9f7ef9c.

[pierlon]

Added the s flag in b7c7e29.

[westonruter]

I think an alternative approach is needed, perhaps atomic grouping? This would seem to be avoidable if the regex engine started searching from the end of the string rather than the beginning.

[pierlon]

I think an alternative approach is needed, perhaps atomic grouping? This would seem to be avoidable if the regex engine started searching from the end of the string rather than the beginning.

I've tried several different variants of atomic grouping, but all either failed to find a match or led to catastrophic backtracing, unfortunately. I've also looked into searching from the end of the string, but PCRE doesn't support that feature.

One flaw I just noticed in the current regex is that it is possible for catastrophic backtracing to occur if the HTML comment being matched contains too much text, as demonstrated here. I'm not sure how to resolve that as yet.

[schlessera]

How about not using a regex, but rather fseek() or similar from the end and cycle through the string?

I'm not entirely sure what that means in terms of performance, but:

• You can bail very early if the response ends with a } and not a -->. So, most of the times, it might just take a look at the last char and bail.
• It cannot lead to catastrophic backtracking. Worst case is cycling over the entire string character by character once.

[schlessera]

Actually, I'm not sure it is even possible to cycle backwards char-by-char when you can have multibyte chars. I don't think you can detect that when scanning from back to front.

[schlessera]

How about something like this: https://3v4l.org/orG5Y

[pierlon]

How about something like this: 3v4l.org/orG5Y

That seems like a great alternative. Any objections @westonruter?

[westonruter]

Yes, that looks good. I made a small tweak to make sure that the $length is large enough to subtract from: https://3v4l.org/BTeIn

[westonruter]

I love this:

Thanks for the co-authorship attribution!