Fix out-of-memory crashes related to ArrayBuffer allocation

third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp

Index: third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
diff --git a/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp b/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
index aa8877c95d1639df5c10f447eaa832765ef66e77..b0d854ce7b81ab03baa67496542bf16bfc8cfefb 100644
--- a/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
+++ b/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
@@ -96,9 +96,16 @@ v8::Local<v8::Value> toV8(const IDBKey* key, v8::Local<v8::Object> creationConte
     case IDBKey::StringType:
         return v8String(isolate, key->string());
     case IDBKey::BinaryType:
-        // Experimental feature: binary keys
-        // https://w3c.github.io/IndexedDB/#steps-to-convert-a-key-to-a-value
-        return toV8(DOMArrayBuffer::create(reinterpret_cast<const unsigned char*>(key->binary()->data()), key->binary()->size()), creationContext, isolate);
+        {
+            // Experimental feature: binary keys
+            // https://w3c.github.io/IndexedDB/#steps-to-convert-a-key-to-a-value
+            RefPtr<DOMArrayBuffer> buffer = DOMArrayBuffer::createOrNull(reinterpret_cast<const unsigned char*>(key->binary()->data()), key->binary()->size());
+            if (!buffer) {
+                isolate->ThrowException(v8::Exception::RangeError(v8::String::NewFromUtf8(isolate, "Out of memory. Failed to allocate ArrayBuffer.")));

[jsbell, 2015/10/16 22:12:24]

This is going to be weird behavior! The scenario will be something like this:

var request = index.getKey(query); // assume result has binary key
request.onerror = () => console.log(request.error);
request.onsuccess = () => console.log(request.result); // throws here

The request will fire 'success' rather than 'error' since the query succeeded.
It's only at the point where `request.result` is called that we hit this code
path to convert the internal IDBKey type to an array buffer, and boom!

What's even stranger is that the result of the conversion is *cached* so if
`request.result` is called again... I'm not sure what will happen. Relevant code
is:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Also, we're thinking of adding Promise helpers here, so it might read as:

index.getKey(query).ready.then((result) => {
  ...
});

That's not spec'd/coded yet but... wow, not sure what should happen there.
Convert the fulfill into a reject halfway through processing??? Fun!

(The good news is that binary key support is experimental, so we have
flexibility - it might make the most sense to artificially cap the key size and
crash on OOM here etc)

[jsbell, 2015/10/16 22:16:26]

Hrm, I'm an idiot... this *also* applies to *values* returned by Indexed DB,
that's just hidden inside the SerializedScriptValue code, no module-specific
bindings code. Same basic code flows will be affected though, e.g. 

var request = store.get(key); // assume result contains cloned arraybuffer
request.onerror = () => console.log(request.error);
request.onsuccess = () => console.log(request.result); // throws here

At the point where `request.result` is accessed we dive into the
SerializedScriptValue code to deserialize, and boom. So... not just an
"experimental" issue. Fun.

[Justin Novosad, 2015/10/19 16:42:52]

For this initial CL, I want to stay clear of anything controversial, So I could
just avoid the change in behavior here by calling
DOMArrayBuffer::deprecatedCreateOrCrash, and file a separate issue so that this
case can be discussed and resolved in isolation.

+                return v8Undefined();
+            }
+            return toV8(buffer, creationContext, isolate);
+        }
     case IDBKey::DateType:
         return v8::Date::New(context, key->date()).ToLocalChecked();
     case IDBKey::ArrayType:
@@ -185,7 +192,12 @@ static IDBKey* createIDBKeyFromValue(v8::Isolate* isolate, v8::Local<v8::Value>
         }
         if (value->IsArrayBufferView()) {
             DOMArrayBufferView* view = V8ArrayBufferView::toImpl(value.As<v8::Object>());
-            if (view->buffer()->isNeutered()) {
+            RefPtr<DOMArrayBuffer> buffer = view->bufferOrNull();
+            if (!buffer) {

[jsbell, 2015/10/16 22:12:24]

The usage here is that script has passed a view in and we're getting at the
underlying buffer. Is it a supported scenario with this CL that script is now
able to hold on to views with null buffers? That seems odd - ISTM that the
intent is that a RangeError would be thrown before such a broken view could ever
be created.

(i.e. I'd expect this to be an ASSERT instead)

[Justin Novosad, 2015/10/19 16:42:52]

Acknowledged.

+                exceptionState.throwRangeError("Out of Momory.");

[binji, 2015/10/16 22:12:39]

sp: memory

[Justin Novosad, 2015/10/19 16:42:52]

Acknowledged.

+                return nullptr;
+            }
+            if (buffer->isNeutered()) {
                 exceptionState.throwTypeError("The viewed ArrayBuffer is neutered.");
                 return nullptr;
             }