Cleanly handle excessively long JS source strings.

Source/bindings/core/v8/V8ScriptRunner.cpp

Index: Source/bindings/core/v8/V8ScriptRunner.cpp
diff --git a/Source/bindings/core/v8/V8ScriptRunner.cpp b/Source/bindings/core/v8/V8ScriptRunner.cpp
index e7d3fa1a89b20c2f24dfd4b0d08ebfbd463e98ec..36182aa62a12e896c6319ea2532a496a26d9fe8a 100644
--- a/Source/bindings/core/v8/V8ScriptRunner.cpp
+++ b/Source/bindings/core/v8/V8ScriptRunner.cpp
@@ -359,7 +359,15 @@ PassOwnPtr<CompileFn> selectCompileFunction(ScriptResource* resource, ScriptStre
 
 v8::Local<v8::Script> V8ScriptRunner::compileScript(const ScriptSourceCode& source, v8::Isolate* isolate, AccessControlStatus corsStatus, V8CacheOptions cacheOptions)
 {
-    return compileScript(v8String(isolate, source.source()), source.url(), source.startPosition(), isolate, source.resource(), source.streamer(), source.resource() ? source.resource()->cacheHandler() : nullptr, corsStatus, cacheOptions);
+    v8::Handle<v8::String> sourceAsV8String(v8String(isolate, source.source()));
+    if (sourceAsV8String.IsEmpty()) {
+        // String conversion fails when the source size exceeds the V8 string
+        // length of (currently) 256MB.
+        V8ThrowException::throwGeneralError(isolate, "JavaScript source exceeds maximum string length.");

[vogelheim, 2015/03/06 17:07:52]

Does this even make any sense?

When I reproduce the test case in the browser it will now cleanly abort
execution instead of crashing the tab. However, merely returning the empty
handle below does that, too. I've been looking for a user-visible indication of
the error, such as a console message or exception log or whatnot, so that the
web developer has a chance of knowing what's going on, but I can't find any of
it in devtools, so I wonder if I'm doing it wrong.

The way I read ScriptController::executeScriptAndReturnValue, it purposely
ignores whatever the v8::TryCatch catches, which I guess is why I'm not seeing
anything.

So... should I just remove the throwGeneralError as it's a no-op in this case;
or keep it in for the sake of other code paths? Or... something else?

[marja, 2015/03/06 18:10:10]

Hmm, there is some mechanism so that we get a console message when there is a
compile error, and I think this should just do the same. I don't know by heart
where that message comes from.

[haraken, 2015/03/07 04:17:15]

A short answer is that I don't think this check makes much sense. I think we can
remove it.

If I'm understanding correctly, V8 does not support a string larger than 200 MB
(right?). So, in the first place, you cannot pass a string larger than 200 MB
from V8 to Blink. You'll hit a crash in V8 before falling into this code path.
Thus the only case we hit the code path is that Blink tries to compile a string
larger than 256 MB. However, in this case, there is no way to see the error in
console, so throwGeneralError doesn't do anything meaningful. When Blink passes
a string larger than 256 MB, we can just let V8 crash in the following
compileScript.

In summary, I think we can remove the check.

[vogelheim, 2015/03/09 17:36:52]

Good plan, and with a detour it led to a solution:
- A C++ function that calls V8ThrowException::throwRangeError (or whatever error
we pick)
- compileScript creating a v8::Function based on the above and calls it.

It's just a few lines of code, but it doesn't make me feel pretty. :-|

[vogelheim, 2015/03/09 17:36:52]

I disagree (on that part), and/or there's a misunderstanding.

Presently it crashes in compileScript, as you suggest, but that means we keep
getting crash reports when someone actually includes gigantic JS files. Which
people do. IMHO, Jakob was very much right to classify this as a proper bug and
tossing it over to me. 

We shouldn't really crash the tab based on perfectly reproducable user
behaviour. The question that's open - if you can agree with me on the above - is
how to fix it. The current CL will silently drop the script, with no hint to the
JS developer what's going wrong. Based on Marja's comment, I'll upload a new
version with a somewhat hack-ish code that will output a console message.

+        return v8::Local<v8::Script>();
+    }
+
+    return compileScript(sourceAsV8String, source.url(), source.startPosition(), isolate, source.resource(), source.streamer(), source.resource() ? source.resource()->cacheHandler() : nullptr, corsStatus, cacheOptions);
 }
 
 v8::Local<v8::Script> V8ScriptRunner::compileScript(v8::Handle<v8::String> code, const String& fileName, const TextPosition& scriptStartPosition, v8::Isolate* isolate, ScriptResource* resource, ScriptStreamer* streamer, CachedMetadataHandler* cacheHandler, AccessControlStatus corsStatus, V8CacheOptions cacheOptions, bool isInternalScript)