Bug 47906

Summary: The number of recursive match calls isn't limited in YARR Interpreter
Product: WebKit Reporter: Peter Varga <pvarga>
Component: JavaScriptCoreAssignee: Peter Varga <pvarga>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, barraclough, commit-queue, msaboff, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 46719    
Attachments:
Description Flags
proposed patch
none
proposed patch v2 none

Peter Varga
Reported 2010-10-19 08:08:40 PDT
The YARR Interpreter fails on the fast/regex/slow.html layout test because the number of recursive match calls is unbounded.
Attachments
proposed patch (19.33 KB, patch)
2010-10-19 08:16 PDT, Peter Varga
no flags
proposed patch v2 (19.41 KB, patch)
2010-10-19 10:26 PDT, Peter Varga
no flags
Peter Varga
Comment 1 2010-10-19 08:16:19 PDT
Created attachment 71172 [details] proposed patch
Andras Becsi
Comment 2 2010-10-19 09:31:01 PDT
(In reply to comment #1) > Created an attachment (id=71172) [details] > proposed patch If I'm correct this should fix the fast/regex/slow.html time out of bug 46719. Does this patch affect the performance somehow, especially because of the newly introduced checks? Would be great to see some perf results, too. Great work, btw!
Peter Varga
Comment 3 2010-10-19 09:59:43 PDT
(In reply to comment #2) > If I'm correct this should fix the fast/regex/slow.html time out of bug 46719. Does this patch affect the performance somehow, especially because of the newly introduced checks? Would be great to see some perf results, too. > Great work, btw! Performance results: ref mod regexp-dna: 1.008x as fast 564.7ms +/- 0.2% 560.3ms +/- 0.3% v8-regexp: 1.016x as fast 3497.1ms +/- 0.3% 3441.3ms +/- 0.3%
Peter Varga
Comment 4 2010-10-19 10:26:24 PDT
Created attachment 71181 [details] proposed patch v2 Fix to free allocated memory in case of failure.
Gavin Barraclough
Comment 5 2010-10-19 11:03:49 PDT
Comment on attachment 71181 [details] proposed patch v2 Hmmm, I'm pondering. So, I don't think this is the perfect solution, but it's probably the best solution right now. I think we want to move away as much as possible from arbitrary limits like this, and instead be using the timeout approach used by the JS language VM. This presently requires polling, but we may be able to make this more asynchronous. But this is likely a much more involved change, and best handled separately – just reintroducing the fixed limit for now is probably sufficient, and a sensible step. Let me cogitate a little more. By the way, 1000000 is the same limit as PCRE I assume?
Peter Varga
Comment 6 2010-10-19 11:07:33 PDT
(In reply to comment #5) > (From update of attachment 71181 [details]) > By the way, 1000000 is the same limit as PCRE I assume? Yes, it's same. The limit and the logic of check are from pcre.
Peter Varga
Comment 7 2010-11-04 04:28:47 PDT
ping
WebKit Commit Bot
Comment 8 2010-11-16 15:27:29 PST
Comment on attachment 71181 [details] proposed patch v2 Clearing flags on attachment: 71181 Committed r72140: <http://trac.webkit.org/changeset/72140>
WebKit Commit Bot
Comment 9 2010-11-16 15:27:34 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.