Switch RegularExpression from YARR to V8

Source/core/platform/text/RegularExpression.cpp

Index: Source/core/platform/text/RegularExpression.cpp
diff --git a/Source/core/platform/text/RegularExpression.cpp b/Source/core/platform/text/RegularExpression.cpp
index 2b225ade22404e6a9f0898590695203021b4a10e..59b15f6a187d03086eb252253a7e8a7be0511c92 100644
--- a/Source/core/platform/text/RegularExpression.cpp
+++ b/Source/core/platform/text/RegularExpression.cpp
@@ -28,67 +28,70 @@
 #include "config.h"
 #include "RegularExpression.h"
 
-#include <wtf/BumpPointerAllocator.h>
-#include <yarr/Yarr.h>
-#include "Logging.h"
+// FIXME: These seem like a layering violation, but converting the strings manually
+// without v8String is difficult, and calling into v8 without V8RecursionScope will
+// assert. Perhaps v8 basic utilities shouldn't be in bindings, or we should put
+// RegularExpression as some kind of abstract interface that's implemented in bindings.
+#include "V8Binding.h"
+#include "V8RecursionScope.h"
 
 namespace WebCore {
 
-RegularExpression::RegularExpression(const String& pattern, TextCaseSensitivity caseSensitivity, MultilineMode multilineMode)
-    : m_numSubpatterns(0)
-    , m_regExpByteCode(compile(pattern, caseSensitivity, multilineMode))
+static v8::Handle<v8::Context> regexContext()

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

This copies a persistent handle, which is a pattern we're trying to avoid....

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

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

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

Yep.

 {
+    static ScopedPersistent<v8::Context>* staticRegexContext = new ScopedPersistent<v8::Context>(v8::Context::New());

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

I think an ASSERT(isMainThread()) would be good at the top of this method.

+    return staticRegexContext->get();
 }
 
-PassOwnPtr<JSC::Yarr::BytecodePattern> RegularExpression::compile(const String& patternString, TextCaseSensitivity caseSensitivity, MultilineMode multilineMode)
+RegularExpression::RegularExpression(const String& pattern, TextCaseSensitivity caseSensitivity, MultilineMode multilineMode)
 {
-    const char* constructionError = 0;
-    JSC::Yarr::YarrPattern pattern(patternString, (caseSensitivity == TextCaseInsensitive), (multilineMode == MultilineEnabled), &constructionError);
-    if (constructionError) {
-        LOG_ERROR("RegularExpression: YARR compile failed with '%s'", constructionError);
-        return nullptr;
-    }
-
-    m_numSubpatterns = pattern.m_numSubpatterns;
-
-    return JSC::Yarr::byteCompile(pattern, &m_regexAllocator);
+    v8::HandleScope handleScope;
+    v8::Handle<v8::Context> context(regexContext());
+    v8::Context::Scope scope(context);

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

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.

+
+    unsigned flags = v8::RegExp::kNone;

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

Can we declare flags to be of type v8::RegExp::Flags and avoid the cast later?

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

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

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

Ok.

+    if (caseSensitivity == TextCaseInsensitive)
+        flags |= v8::RegExp::kIgnoreCase;
+    if (multilineMode == MultilineEnabled)
+        flags |= v8::RegExp::kMultiline;
+
+    v8::TryCatch tryCatch;

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

Do we need to check anything about this object at the end of this function? 
What happens if the pattern is malformed?

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

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.

+    m_regex.set(v8::RegExp::New(v8String(pattern, context->GetIsolate()), static_cast<v8::RegExp::Flags>(flags)));
 }
 
-int RegularExpression::match(const String& str, int startFrom, int* matchLength) const
+int RegularExpression::match(const String& string, int startFrom, int* matchLength) const
 {
-    if (!m_regExpByteCode)
+    if (m_regex.isEmpty() || string.isNull())
         return -1;
 
-    if (str.isNull())
+    // v8 strings are limited to int.
+    if (string.length() > INT_MAX)
         return -1;
 
-    int offsetVectorSize = (m_numSubpatterns + 1) * 2;
-    unsigned* offsetVector;
-    Vector<unsigned, 32> nonReturnedOvector;
+    v8::HandleScope handleScope;
+    v8::Handle<v8::Context> context(regexContext());
+    v8::Context::Scope scope(context);
+    v8::TryCatch tryCatch;
 
-    nonReturnedOvector.resize(offsetVectorSize);
-    offsetVector = nonReturnedOvector.data();
+    V8RecursionScope::MicrotaskSuppression microtaskScope;
 
-    ASSERT(offsetVector);
-    for (unsigned j = 0, i = 0; i < m_numSubpatterns + 1; j += 2, i++)
-        offsetVector[j] = JSC::Yarr::offsetNoMatch;
+    v8::Local<v8::Function> exec = m_regex->Get(v8::String::NewSymbol("exec")).As<v8::Function>();

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

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]

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]

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

 
-    unsigned result;
-    if (str.length() <= INT_MAX)
-        result = JSC::Yarr::interpret(m_regExpByteCode.get(), str, startFrom, offsetVector);
-    else {
-        // This code can't handle unsigned offsets. Limit our processing to strings with offsets that 
-        // can be represented as ints.
-        result = JSC::Yarr::offsetNoMatch;
-    }
+    v8::Handle<v8::Value> argv[] = { v8String(string, context->GetIsolate()) };
+    v8::Local<v8::Value> returnValue = exec->Call(m_regex.get(), 1, argv);
 
-    if (result == JSC::Yarr::offsetNoMatch)
+    if (!returnValue->IsObject())
         return -1;
 
-    // 1 means 1 match; 0 means more than one match. First match is recorded in offsetVector.
-    if (matchLength)
-        *matchLength = offsetVector[1] - offsetVector[0];
-    return offsetVector[0];
+    v8::Local<v8::Object> result = returnValue.As<v8::Object>();
+    double index = result->Get(v8::String::NewSymbol("index")).As<v8::Number>()->Value();

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

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>().

+
+    if (matchLength) {
+        v8::Local<v8::String> match = result->Get(0).As<v8::String>();

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

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?

+        *matchLength = match->Length();
+    }
+
+    return static_cast<int>(index);
 }
 
 void replace(String& string, const RegularExpression& target, const String& replacement)