Switch RegularExpression from YARR to V8

Switch RegularExpression from YARR to V8

Make RegularExpression use V8 instead of YARR. This removes all but one
last usage of YARR in ContentSearchUtils which can be removed in a follow
up patch.

It's a little awkward to use V8's regular expression features from C++ since
you need to interact with the JS object fields, but this is the best we can do
for now. Eventually it'd be nice if V8 exposed a RegExp API directly.

For this patch we need to stop using static RegularExpressions in the web
inspector code because RegularExpression is no longer thread safe and the
web inspector would previously use the same instance between multiple
workers in InspectorStyleSheet.cpp.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148958

[abarth-chromium, 2013-04-21 22:31:52]

How good is our test coverage of this code?  I guess input@pattern has some test
coverage.  Does it test things like malformed regexps?

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
File Source/core/platform/text/RegularExpression.cpp (right):

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:40: static
v8::Handle<v8::Context> regexContext()
This copies a persistent handle, which is a pattern we're trying to avoid....

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:50: v8::Context::Scope
scope(context);
It looks like regexContext() should return a new Local handle to the context. 
We never want to put a persistent handle into a v8::Context::Scope.

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:52: unsigned flags =
v8::RegExp::kNone;
Can we declare flags to be of type v8::RegExp::Flags and avoid the cast later?

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:58: v8::TryCatch tryCatch;
Do we need to check anything about this object at the end of this function? 
What happens if the pattern is malformed?

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:78: v8::Local<v8::Function> exec
= m_regex->Get(v8::String::NewSymbol("exec")).As<v8::Function>();
Does this ASSERT if we don't get a v8::Function out?  We're assuming that we
never run malicious script in this context.

[esprehn, 2013-04-21 23:11:10]

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
File Source/core/platform/text/RegularExpression.cpp (right):

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:40: static
v8::Handle<v8::Context> regexContext()
On 2013/04/21 22:31:52, abarth wrote:
> This copies a persistent handle, which is a pattern we're trying to avoid....

So I can do return v8::Local<v8::Context>::New(staticRegexContext->get()) ?

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:52: unsigned flags =
v8::RegExp::kNone;
On 2013/04/21 22:31:52, abarth wrote:
> Can we declare flags to be of type v8::RegExp::Flags and avoid the cast later?

Nope, the compiler won't let you do |= if you do that.

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:58: v8::TryCatch tryCatch;
On 2013/04/21 22:31:52, abarth wrote:
> Do we need to check anything about this object at the end of this function? 
> What happens if the pattern is malformed?

I don't think so, but adamk would know more. I suppose we don't report the
failure message if the regex is invalid anywhere though, we used to log it to
the console in debug builds I think. Since this is for input@pattern I'm not
sure we want to do that.

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:78: v8::Local<v8::Function> exec
= m_regex->Get(v8::String::NewSymbol("exec")).As<v8::Function>();
On 2013/04/21 22:31:52, abarth wrote:
> Does this ASSERT if we don't get a v8::Function out?  We're assuming that we
> never run malicious script in this context.

It does with V8_ENABLE_CHECKS enabled which could be used in debug. It won't in
a release build. Seems a little weird to harden this code against attacks when
the v8::Context is private to regex but I can add it if you want?

[abarth-chromium, 2013-04-21 23:57:00]

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
File Source/core/platform/text/RegularExpression.cpp (right):

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:40: static
v8::Handle<v8::Context> regexContext()
On 2013/04/21 23:11:10, esprehn wrote:
> On 2013/04/21 22:31:52, abarth wrote:
> > This copies a persistent handle, which is a pattern we're trying to
avoid....
> 
> So I can do return v8::Local<v8::Context>::New(staticRegexContext->get()) ?

Yep.

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:52: unsigned flags =
v8::RegExp::kNone;
On 2013/04/21 23:11:10, esprehn wrote:
> On 2013/04/21 22:31:52, abarth wrote:
> > Can we declare flags to be of type v8::RegExp::Flags and avoid the cast
later?
> 
> Nope, the compiler won't let you do |= if you do that.

Ok.

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:78: v8::Local<v8::Function> exec
= m_regex->Get(v8::String::NewSymbol("exec")).As<v8::Function>();
On 2013/04/21 23:11:10, esprehn wrote:
> On 2013/04/21 22:31:52, abarth wrote:
> > Does this ASSERT if we don't get a v8::Function out?  We're assuming that we
> > never run malicious script in this context.
> 
> It does with V8_ENABLE_CHECKS enabled which could be used in debug. It won't
in
> a release build. Seems a little weird to harden this code against attacks when
> the v8::Context is private to regex but I can add it if you want?

As long as it ASSERTs with V8_ENABLE_CHECKS, I think we're fine.

[adamk, 2013-04-22 16:40:38]

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
File Source/core/platform/text/RegularExpression.cpp (right):

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:42: static
ScopedPersistent<v8::Context>* staticRegexContext = new
ScopedPersistent<v8::Context>(v8::Context::New());
I think an ASSERT(isMainThread()) would be good at the top of this method.

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:87: double index =
result->Get(v8::String::NewSymbol("index")).As<v8::Number>()->Value();
Note that the V8 API exposes some more int-like types, so you could avoid a
static_cast later by using ToInt32().Value() instead of .As<v8::Number>().

https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
Source/core/platform/text/RegularExpression.cpp:90: v8::Local<v8::String> match
= result->Get(0).As<v8::String>();
To someone not familiar with the JS RegExp API, this 0 looks like a magic
number. Would it be bad of me to ask for a constant naming it?

[esprehn, 2013-04-22 17:35:44]

On 2013/04/22 16:40:38, adamk wrote:
> ...
>
https://codereview.chromium.org/13896017/diff/1/Source/core/platform/text/Reg...
> Source/core/platform/text/RegularExpression.cpp:90: v8::Local<v8::String>
match
> = result->Get(0).As<v8::String>();
> To someone not familiar with the JS RegExp API, this 0 looks like a magic
> number. Would it be bad of me to ask for a constant naming it?

I added comments explaining and made the types stricter. It's actually a
v8::Array of v8::Strings with some random properties tacked on because JS is
awesome.

[adamk, 2013-04-22 17:48:03]

https://codereview.chromium.org/13896017/diff/8001/Source/core/html/BaseTextI...
File Source/core/html/BaseTextInputType.cpp (right):

https://codereview.chromium.org/13896017/diff/8001/Source/core/html/BaseTextI...
Source/core/html/BaseTextInputType.cpp:48: if (!regex.isValid())
There are other uses of RegularExpression; is there a reason not to update them
too with an isValid call? It looks like you just added this check for clarity,
not correctness?

https://codereview.chromium.org/13896017/diff/8001/Source/core/platform/text/...
File Source/core/platform/text/RegularExpression.cpp (right):

https://codereview.chromium.org/13896017/diff/8001/Source/core/platform/text/...
Source/core/platform/text/RegularExpression.cpp:62:
m_regex.set(v8::RegExp::New(v8String(pattern, context->GetIsolate()),
static_cast<v8::RegExp::Flags>(flags)));
I think it would be clearer if you split this into two lines with an "if
(!regex.IsEmpty())" so it's more obvious that m_regex will sometimes not be set,
rather than relying on the API to return an empty handle in case of an
exception.

[esprehn, 2013-04-22 17:54:42]

On 2013/04/22 17:48:03, adamk wrote:
>
https://codereview.chromium.org/13896017/diff/8001/Source/core/html/BaseTextI...
> File Source/core/html/BaseTextInputType.cpp (right):
> 
>
https://codereview.chromium.org/13896017/diff/8001/Source/core/html/BaseTextI...
> Source/core/html/BaseTextInputType.cpp:48: if (!regex.isValid())
> There are other uses of RegularExpression; is there a reason not to update
them
> too with an isValid call? It looks like you just added this check for clarity,
> not correctness?
> 

It's for correctness, this patch fixes a webkit bug I noticed in the process of
trying to write a test per abarth's request. If the pattern attribute is an
invalid regex we should just ignore it while previously you'd end up with an
input that could never be valid because the regex never matches.

Regexes we use internal to Blink will just end up in a state where they can
never match, that's how this worked before.

[adamk, 2013-04-22 17:59:03]

lgtm (though I think you'll need abarth's stamp too as I'm not an OWNER)

[abarth-chromium, 2013-04-22 18:03:16]

LGTM2

[esprehn, 2013-04-22 18:28:48]

Committed patchset #3 manually as r148840 (presubmit successful).

[vsevik, 2013-04-23 12:44:19]

On 2013/04/22 18:28:48, esprehn wrote:
> Committed patchset #3 manually as r148840 (presubmit successful).

This was reverted in https://codereview.chromium.org/13950014.

The reason tests fail is that this patch does not take startFrom parameter of
RegularExpression::match() method into account.

Fix suggestion:

diff --git a/Source/core/platform/text/RegularExpression.cpp
b/Source/core/platform/text/RegularExpression.cpp
index 58cf72e..ee7690b 100644
--- a/Source/core/platform/text/RegularExpression.cpp
+++ b/Source/core/platform/text/RegularExpression.cpp
@@ -52,7 +52,7 @@ RegularExpression::RegularExpression(const String& pattern,
TextCaseSensitivity
     v8::Local<v8::Context> context(regexContext());
     v8::Context::Scope scope(context);
 
-    unsigned flags = v8::RegExp::kNone;
+    unsigned flags = v8::RegExp::kGlobal;
     if (caseSensitivity == TextCaseInsensitive)
         flags |= v8::RegExp::kIgnoreCase;
     if (multilineMode == MultilineEnabled)
@@ -82,6 +82,7 @@ int RegularExpression::match(const String& string, int
startFrom, int* matchLeng
 
     V8RecursionScope::MicrotaskSuppression microtaskScope;
 
+    m_regex->Set(v8::String::NewSymbol("lastIndex"), v8Integer(startFrom,
context->GetIsolate()));
     v8::Local<v8::Function> exec =
m_regex->Get(v8::String::NewSymbol("exec")).As<v8::Function>();
 
     v8::Handle<v8::Value> argv[] = { v8String(string, context->GetIsolate()) };

[esprehn, 2013-04-23 23:10:51]

On 2013/04/23 12:44:19, vsevik wrote:
> On 2013/04/22 18:28:48, esprehn wrote:
> > Committed patchset #3 manually as r148840 (presubmit successful).
> 
> This was reverted in https://codereview.chromium.org/13950014.
> 
> The reason tests fail is that this patch does not take startFrom parameter of
> RegularExpression::match() method into account.
> 
> Fix suggestion:

Turning on the global flag will make the regex more expensive since it'll keep
searching even after it matches once. I have another attempt to upload.

The other issue was that the web inspector is using regexes from multiple
threads (each worker) so this patch won't work as is anyway.

[abarth-chromium, 2013-04-24 00:57:53]

LGTM

[esprehn, 2013-04-24 03:08:45]

Committed patchset #7 manually as r148958 (presubmit successful).