Cleanly handle excessively long JS source strings.

Source/bindings/core/v8/V8ScriptRunner.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 341 matching lines @@
     // Failed resources should never get this far.
     ASSERT(!resource->errorOccurred());
     ASSERT(streamer->isFinished());
     ASSERT(!streamer->streamingSuppressed());
     return WTF::bind<v8::Isolate*, v8::Handle<v8::String>, v8::ScriptOrigin>(postStreamCompile, resource->cacheHandler(), streamer);
 }
 } // namespace
 
 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)
 {
     TRACE_EVENT1("v8", "v8.compile", "fileName", fileName.utf8());
     TRACE_EVENT_SCOPED_SAMPLING_STATE("v8", "V8Compile");
 
     ASSERT(!streamer || resource);
     ASSERT(!resource || resource->cacheHandler() == cacheHandler);
 
@@ skipping 132 matching lines @@
 {
     return cacheTag(CacheTagParser, cacheHandler);
 }
 
 unsigned V8ScriptRunner::tagForCodeCache(CachedMetadataHandler* cacheHandler)
 {
     return cacheTag(CacheTagCode, cacheHandler);
 }
 
 } // namespace blink