Bug 47906 - The number of recursive match calls isn't limited in YARR Interpreter
Summary: The number of recursive match calls isn't limited in YARR Interpreter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Peter Varga
URL:
Keywords:
Depends on:
Blocks: 46719
  Show dependency treegraph
 
Reported: 2010-10-19 08:08 PDT by Peter Varga
Modified: 2010-11-16 15:27 PST (History)
5 users (show)

See Also:


Attachments
proposed patch (19.33 KB, patch)
2010-10-19 08:16 PDT, Peter Varga
no flags Details | Formatted Diff | Diff
proposed patch v2 (19.41 KB, patch)
2010-10-19 10:26 PDT, Peter Varga
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Varga 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.
Comment 1 Peter Varga 2010-10-19 08:16:19 PDT
Created attachment 71172 [details]
proposed patch
Comment 2 Andras Becsi 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!
Comment 3 Peter Varga 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%
Comment 4 Peter Varga 2010-10-19 10:26:24 PDT
Created attachment 71181 [details]
proposed patch v2

Fix to free allocated memory in case of failure.
Comment 5 Gavin Barraclough 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?
Comment 6 Peter Varga 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.
Comment 7 Peter Varga 2010-11-04 04:28:47 PDT
ping
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2010-11-16 15:27:34 PST
All reviewed patches have been landed.  Closing bug.