Fix out-of-memory crashes related to ArrayBuffer allocation

third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp

Index: third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp b/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
index 3bd0390146d9043f971387b6fb751005509455e8..516ee495c8eb90ba956de9f45136e80208a645c9 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
@@ -251,7 +251,7 @@ void SerializedScriptValueWriter::writeArrayBufferView(const DOMArrayBufferView&
 {
     append(ArrayBufferViewTag);
 #if ENABLE(ASSERT)
-    ASSERT(static_cast<const uint8_t*>(arrayBufferView.bufferBase()->data()) + arrayBufferView.byteOffset() ==
+    ASSERT(static_cast<const uint8_t*>(arrayBufferView.bufferBaseOrNull()->data()) + arrayBufferView.byteOffset() ==

[haraken, 2015/10/29 18:58:37]

arrayBufferView.bufferBaseOrNull() can return nullptr. Is this OK to access
data()?

[Justin Novosad, 2015/11/05 00:17:51]

Done.

         static_cast<const uint8_t*>(arrayBufferView.baseAddress()));
 #endif
     DOMArrayBufferView::ViewType type = arrayBufferView.type();
@@ -976,10 +976,10 @@ ScriptValueSerializer::StateBase* ScriptValueSerializer::writeAndGreyArrayBuffer
     ASSERT(!object.IsEmpty());
     DOMArrayBufferView* arrayBufferView = V8ArrayBufferView::toImpl(object);
     if (!arrayBufferView)
-        return 0;
-    if (!arrayBufferView->bufferBase())
+        return nullptr;
+    if (!arrayBufferView->bufferBaseOrNull())
         return handleError(DataCloneError, "An ArrayBuffer could not be cloned.", next);
-    v8::Local<v8::Value> underlyingBuffer = toV8(arrayBufferView->bufferBase(), m_scriptState->context()->Global(), isolate());
+    v8::Local<v8::Value> underlyingBuffer = toV8(arrayBufferView->bufferBaseOrNull(), m_scriptState->context()->Global(), isolate());
     if (underlyingBuffer.IsEmpty())
         return handleError(DataCloneError, "An ArrayBuffer could not be cloned.", next);
     StateBase* stateOut = doSerializeArrayBuffer(underlyingBuffer, next);
@@ -1532,7 +1532,22 @@ bool SerializedScriptValueReader::readImageData(v8::Local<v8::Value>* value)
         return false;
     if (m_position + pixelDataLength > m_length)
         return false;
-    ImageData* imageData = ImageData::create(IntSize(width, height));
+    // TODO(junov): crbug.com/536816
+    // Here we use a NonThorwableExceptionState in order to fail silently
+    // when ImageData allocation fails. It needs to be ascertained whether
+    // the call sites that depend on value deserialization agree with
+    // re-throwing a RangeError exception from here, which is what happens
+    // when the ArrayBuffer encapsulated in the ImageData fails to be
+    // allocated, as per the ECMAScript spec:
+    // http://ecma-international.org/ecma-262/6.0/#sec-createbytedatablock
+    // Before we decide to propagate the exception down to the script
+    // execution context, all the APIs that depend on this routine would
+    // need to have specifications stating that exceptions thrown by sub
+    // routines involved in the deserialization process are re-thrown.
+    NonThrowableExceptionState exceptionState;
+    ImageData* imageData = ImageData::create(IntSize(width, height), exceptionState);
+    if (exceptionState.hadException())
+        return false;
     DOMUint8ClampedArray* pixelArray = imageData->data();
     ASSERT(pixelArray);
     ASSERT(pixelArray->length() >= pixelDataLength);
@@ -1556,7 +1571,7 @@ bool SerializedScriptValueReader::readCompositorProxy(v8::Local<v8::Value>* valu
     return !value->IsEmpty();
 }
 
-PassRefPtr<DOMArrayBuffer> SerializedScriptValueReader::doReadArrayBuffer()
+PassRefPtr<DOMArrayBuffer> SerializedScriptValueReader::doReadArrayBufferOrNull()
 {
     uint32_t byteLength;
     if (!doReadUint32(&byteLength))
@@ -1565,14 +1580,18 @@ PassRefPtr<DOMArrayBuffer> SerializedScriptValueReader::doReadArrayBuffer()
         return nullptr;
     const void* bufferStart = m_buffer + m_position;
     m_position += byteLength;
-    return DOMArrayBuffer::create(bufferStart, byteLength);
+    return DOMArrayBuffer::createOrNull(bufferStart, byteLength);
 }
 
 bool SerializedScriptValueReader::readArrayBuffer(v8::Local<v8::Value>* value)
 {
-    RefPtr<DOMArrayBuffer> arrayBuffer = doReadArrayBuffer();
-    if (!arrayBuffer)
-        return false;
+    RefPtr<DOMArrayBuffer> arrayBuffer = doReadArrayBufferOrNull();
+    // TODO(junov): crbug.com/536816 Instead of the following assert, we should consider doing:
+    //   if (!arrayBuffer)
+    //       return false;
+    // To do that, we need to make sure that call sites would react correctly
+    // in this case, with the value not having been set.
+    RELEASE_ASSERT(arrayBuffer); // This is essentially an out of memory crash

[haraken, 2015/10/29 18:58:37]

You've added a bunch of RELEASE_ASSERT(arrayBuffer) that catches OOM. Maybe
would it be better to introduce a helper never-inline function like
arrayBufferOutOfMemory()? Then we can get more consistent crash reports for
array buffer OOM.

[Justin Novosad, 2015/11/05 00:17:51]

Done. Turns out that I made a mistake about this case, and I ended up reverting
changes to call site where we are extracting an array buffer from an array
buffer view. These operation never perform any buffer allocations, so they did
not need to be touched.

     *value = toV8(arrayBuffer.release(), m_scriptState->context()->Global(), isolate());
     return !value->IsEmpty();
 }