Check for balanced parens before executing gradient regex

[redmunds]

The regex for gradients is particularly tricky with parens, which is why it's causing JS engine to go into an infinite loop in the case described in #4650. To workaround this problem, first verify that content has balanced parens.

Assigning to @JeffryBooher since we discussed this offline

[JeffryBooher]

you need a context shift for when you enter / exit a quoted string or escape character ("')

[TomMalbran]

Or could remove both strings and comments before checking if the parens are balanced?

[redmunds]

Good catch! Fixes pushed.

[JeffryBooher]

Initial review complete.

I still need to test it to make sure that it solves the hang but I'm a little concerned about the performance.

Maybe @njx could look at the performance.

[redmunds]

@JeffryBooher Using the QuickView test.css unit test file, I don't notice any difference between master and my branch.

[peterflynn]

@redmunds @JeffryBooher I wonder if we should add PerfUtils instrumentation to QuickView at some point. Since it runs on every mousemove it's fairly performance-sensitive bits of code... it'd be good to be able to measure it more precisely.

[peterflynn]

Also, random thought: will either the previous or this new code have perf problems with long lines, i.e. minified files? I wonder if we should check line.length at the start of each provider and give up if it's super long...

[njx]

On @peterflynn's last note...I wonder if we should change the behavior in QuickView so that it doesn't actually check any of the providers until the mouse has actually sat still for the hover time. Right now, it checks the providers, and then kicks off a timer to pop up the preview later. Maybe we could instead just kick off a timer on mousemove, reset it when the mouse moves, and then only query the providers when it hits the timeout.

[redmunds]

@peterflynn Regarding PerfUtils instrumentation, it's easy to add, but I'm wondering if anyone is checking the data. We generate a reasonable amount of data now, all the time, no matter if anyone looks at it or not. Seems like PerfUtils should be disabled by default, but easy to turn on for whoever's interested.

Yes, we scan current line every time mouse moves more than 1 char. Interesting that we then delay 350ms to display popover. Seems like we could save some processing by delaying the scanning by 350ms, then display popover when ready.

Good catch on long lines. We check char position to make sure it's in code to popover, but we don't bail out of the loop after we get past current position. Should be an easy fix.

[njx]

@redmunds yup, I think your 2nd para is exactly what I was suggesting.

[peterflynn]

@njx @redmunds I think that was by design for some reason -- I remember not liking it but finding it tricky to change. IIRC it was something like allowing images to start preloading during the hover delay period... but glancing at the code now it doesn't appear to actually do that...

[redmunds]

I added 2 PerfUtils timers to test performance:

1. QuickView balanced parens - measure areParensBalanced() function
2. QuickView scan line - measures scanning of line section of handleMouseMove(). Does not include determining whether to scan or timer to delay displaying popover, or displaying popover. This may also include 1 or more calls to areParensBalanced() function.

Here are the results after 30 seconds or so of mousing and scrolling in the QuicKView test.css file:

• QuickView balanced parens: 0/0.1(266)/3/0
• QuickView scan line: 0/9(254)/67/3

The format of the data is: min/avg(count)/max/last
Where numbers are in milliseconds. Note that I increased accuracy of average to tenths of a millisecond and added the "count" metric for this test.

For more of a stress test, I opened a large (60KB) minified css file: test/perf/OpenFile-perf-files/jquery.mobile-1.1.0.min.css

• QuickView balanced parens: 0/0.7(4195)/90/1
• QuickView scan line: 1/471.3(35)/12536/219

Summary
I think this shows that the areParensBalanced() function adds less than 2% (~27ms/~2300ms) cpu cycles, so is reasonable to use for now (hopefully only temporarily). @JeffryBooher let me know if you disagree.

In the test.css page, ~8% (~2.3 seconds) of the 30 second test was scanning for things to display in popover. In the jquery.mobile-1.1.0.min.css page, editor seemed to freeze at points, although I'm not 100% sure it was only the QuickView feature since these types of files are notoriously slow in CodeMirror and Brackets. I opened issue #4910 for performance improvements.

[redmunds]

@JeffryBooher Updated to new CSS Utils functions. Ready for another review.

[redmunds]

OK, let's actually push the changes this time. 👍

Also, I changed my mind about the PerfUtils timers and removed them. I think that the amount of data collected on mouse move operations could potentially be too large after hours of use.

[peterflynn]

@redmunds Removing the PerfUtils calls seems like a good call to me too. Nothing wrong with using those APIs just for some targeted one-off testing without permanently checking in!

[JeffryBooher]

Looks good. Just squash the commits and i'll merge.

[redmunds]

Squashed version is here: #4929

Closing this one.