Fix out-of-memory crashes related to ArrayBuffer allocation

Fix out-of-memory crashes related to ArrayBuffer allocation

Context: In the past, ArrayBuffer creation would return a null pointer
when memory allocation failed. The problem with that approach was that
not all callsites (and there are many) handle null pointers gracefully,
or check for null pointers at all. So this approach was leading to
potential security vulnerabilities. Right now, failure to allocate an
ArrayBuffer will crash the process, which is secure, but is responsible
for a significant number of crashes. As of Chrome 46, 2% of renderer
crashes on desktop are in WTF::ArrayBufferContents::allocateMemory.
This change should bring that number very close to zero.

Crash mitigation strategy:

1) In accordance with the ECMAScript specification, failure to allocate
the memory block that backs an ArrayBuffer should result in a RangeError
exception being thrown. Many API that internally create ArrayBuffers
are currently crashing instead of throwing an exception. Spec work is
ongoing on a cases by case basis to have the specs for the APIs changed
to state that exceptions thrown by the ArrayBuffer allocation step are
rethrown. These new exceptions, even if they are not caught and result
in script halting, are a much better UX than a process crash.
For example, an ill-behaved ad in an iframe will not interfere the
host page when there is an unhandled OOM condition.

2) In places that are not inside the scope of a script execution
context, we cannot throw exceptions, so we fail silently instead of
crashing if doing so is possible and makes sense.

3) Allocation of trivially small fixed size buffers are not resolved
by this change and will still crash the renderer. If it is not
possible to allocate very small objects, the process is probably doomed
anyways.

4) This patch leaves many non-trivial call sites in an unresolved state,
meaning that they will crash on allocation failure. The sites of these
potential crashes now have comments leading to crbug.com/536816,
and in some cases suggestions on possible resolution approaches.
The expectations is that these cases will be addressed as needed, based
on incoming crash reports.

Intent to ship thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pZ9kld0LehA

BUG=537903, 536816, 535136, 532337
R=binji@chromium.org

[Justin Novosad, 2015-10-16 21:31:51]

Work in progress.  Still working on tests and spec edits

@binji: Could you provide some early feedback on changes in Source/wtf and
Source/core/dom?

[jsbell, 2015-10-16 22:12:23]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

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

Exciting! The Indexed DB/binary key usage is going to be very odd here.
Fortunately it's not a shipping feature yet (in any browser), so carry on but
stay in touch!

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:104:
isolate->ThrowException(v8::Exception::RangeError(v8::String::NewFromUtf8(isolate,
"Out of memory. Failed to allocate ArrayBuffer.")));
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)

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:196: if
(!buffer) {
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)

[binji, 2015-10-16 22:12:38]

binji@chromium.org changed reviewers:
- jsbell@chromium.org

[binji, 2015-10-16 22:12:40]

This is looking good, mostly just nits.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:197:
exceptionState.throwRangeError("Out of Momory.");
sp: memory

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/DOMArrayBuffer.h (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/DOMArrayBuffer.h:39: static
PassRefPtr<DOMArrayBuffer> create(WTF::ArrayBufferContents& contents)
it almost feels like these functions that don't allocate should have a different
name. Not sure if that's overkill though

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/DOMTypedArray.h (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/DOMTypedArray.h:70: RefPtr<WTF::ArrayBuffer>
buffer = WTF::ArrayBuffer::deprecatedCreateOrCrash(length, 1);
what is the 1 here? Ah, element size.

Seems weird that length is byteLength, not number of elements. Maybe the naming
is just unclear.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/ArrayBufferBuilder.h (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/ArrayBufferBuilder.h:42: // by isValid() before
using an instance.
hm, seems like a potentially dangerous interface. Where is this used?

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/ArrayBufferContents.cpp:158:
allocateMemoryOrNull(sizeInBytes, initPolicy, data);
since this is an enum, probably should assert that the value is what you
expected.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/ArrayBufferContents.cpp:182: other.m_sizeInBytes =
0;
shouldn't be necessary (it is asserted to be zero at the top of the function)

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/ArrayBufferContents.h:96: static void
fakeOutOfMemoryForNextArrayBufferAllocation()
I don't really know the coding standard for Blink, but should this include
forTesting somewhere or is fake enough?

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/Float32Array.h (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Float32Array.h:74: return
TypedArrayBase<float>::createOrNull<Float32Array>(array, length);
should be deprecatedCreateOrCrash?

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

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-10-16 22:16:27]

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:104:
isolate->ThrowException(v8::Exception::RangeError(v8::String::NewFromUtf8(isolate,
"Out of memory. Failed to allocate ArrayBuffer.")));
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]

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:104:
isolate->ThrowException(v8::Exception::RangeError(v8::String::NewFromUtf8(isolate,
"Out of memory. Failed to allocate ArrayBuffer.")));
On 2015/10/16 22:16:26, jsbell wrote:
> 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.

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.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:196: if
(!buffer) {
On 2015/10/16 22:12:24, jsbell wrote:
> 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)

Acknowledged.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:197:
exceptionState.throwRangeError("Out of Momory.");
On 2015/10/16 22:12:39, binji wrote:
> sp: memory

Acknowledged.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/DOMArrayBuffer.h (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/DOMArrayBuffer.h:39: static
PassRefPtr<DOMArrayBuffer> create(WTF::ArrayBufferContents& contents)
On 2015/10/16 22:12:39, binji wrote:
> it almost feels like these functions that don't allocate should have a
different
> name. Not sure if that's overkill though

They still construct a DOMArrayBuffer object. So "create" is still appropriate
IMHO. In theory this method can still yield an OOM crash, though very unlikely.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/DOMTypedArray.h (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/DOMTypedArray.h:70: RefPtr<WTF::ArrayBuffer>
buffer = WTF::ArrayBuffer::deprecatedCreateOrCrash(length, 1);
On 2015/10/16 22:12:39, binji wrote:
> what is the 1 here? Ah, element size.
> 
> Seems weird that length is byteLength, not number of elements. Maybe the
naming
> is just unclear.

Acknowledged.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/ArrayBufferBuilder.h (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/ArrayBufferBuilder.h:42: // by isValid() before
using an instance.
On 2015/10/16 22:12:39, binji wrote:
> hm, seems like a potentially dangerous interface. Where is this used?

Potentially, but did you notice the 'isValid()' method below? This class was
designed with the possibility of an alloc failure in mind. I see isValid being
called in many places. I am going to add some asserts to make sure we never
access m_buffer when !isValid.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/ArrayBufferContents.cpp:158:
allocateMemoryOrNull(sizeInBytes, initPolicy, data);
On 2015/10/16 22:12:39, binji wrote:
> since this is an enum, probably should assert that the value is what you
> expected.

Acknowledged.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/ArrayBufferContents.cpp:182: other.m_sizeInBytes =
0;
On 2015/10/16 22:12:39, binji wrote:
> shouldn't be necessary (it is asserted to be zero at the top of the function)
I disagree. The purpose of this function is to overwrite 'other', which includes
reallocatin other.m_data.  This bit of code is for handling the allocation
failure.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/ArrayBufferContents.h:96: static void
fakeOutOfMemoryForNextArrayBufferAllocation()
On 2015/10/16 22:12:39, binji wrote:
> I don't really know the coding standard for Blink, but should this include
> forTesting somewhere or is fake enough?

Acknowledged.

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/Float32Array.h (right):

https://codereview.chromium.org/1414553002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Float32Array.h:74: return
TypedArrayBase<float>::createOrNull<Float32Array>(array, length);
On 2015/10/16 22:12:39, binji wrote:
> should be deprecatedCreateOrCrash?

Acknowledged.

[jsbell, 2015-10-20 22:25:50]

Thanks, and apologies for more questions than answers. Kudos for taking on
retrofitting OOM handling.

I'm still really worried about bufferless views making it back to script, e.g.
via SerializedScriptValue. I assume that's not something we want to have happen?

Maybe rather than tackling this monolithically, consider landing just #1 first
(i.e. still crash on anything but direct allocations from script), and then
explore the other allocation paths and consequences one by one? 

I don't know what the crash rates look like for the various cases, so maybe #1
on its own just isn't worth doing.

Again, thank you for taking this on, and don't let me dissuade you.

https://codereview.chromium.org/1414553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/1414553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1562:
PassRefPtr<DOMArrayBuffer>
SerializedScriptValueReader::doReadArrayBufferOrNull()
Since this already returned nullptr in some cases, do we need to change the
name?

https://codereview.chromium.org/1414553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1578:
return false;
I'm embarrassed to say I don't know how we handle failure (false) here
downstream in all cases. Previously this would only happen if the data was
corrupted e.g. due to a bug in serialization, so crash-fast might be the best
approach.

We're trying to avoid that with this exploration, so more investigation needed
here. (Another TODO to investigate?)

[tyoshino (SeeGerritForStatus), 2015-10-21 05:36:11]

Took a look at XMLHttpRequest and the code path affecting it. Looks good. As
https://github.com/whatwg/xhr/issues/26 is pending, we should keep it crashing
on .response access. PS2 is aligned with that.

Thanks so much for working on this. It'll be clearer what the crash reason is on
the crash reports.

[Justin Novosad, 2015-10-28 18:23:56]

Description was changed from

==========
Fix out-of-memory crashes related to ArrayBuffer allocation

Context: In the past, ArrayBuffer creation would return a null pointer
when memory allocation failed. The problem with that approach was that
not all callsites (and there are many) handle null pointers gracefully,
or check for null pointers at all. So this approach was leading to
potential security vulnerabilities. Right now, failure to allocate an
ArrayBuffer will crash the process, which is secure, but is responsible
for a significant number of crashes. As of Chrome 46, 2% of renderer
crashes on desktop are in WTF::ArrayBufferContents::allocateMemory.
This change should bring that number very close to zero.

Crash mitigation strategy:

1) In accordance with the ECMAScript specification, failure to allocate
the memory block that backs an ArrayBuffer should result in a RangeError
exception being thrown. Many API that internally create a ArrayBuffers
are currently crashing instead of throwing an exception. Spec work is
ongoing on a cases by case basis to have the specs for the APIs changed
to state that exceptions thrown by the ArrayBuffer allocation step are
rethrown. These new exceptions, event if they are not caught and result
in script halting, are a much better UX than a process crash because.
For example, an ill-behaved ad in an iframe will not interfere the
host page when there is an unhandled OOM condition.

2) In places that are not inside the scope of a script execution
context, we cannot throw exceptions, so we fail silently instead of
crashing. For example if the buffer required for storing the message
data for a push notification cannot be allocated, we simply drop the
notification and never dispatch the event. This sounds bad, but it is
much better than to crash the service worker's entire process, which
also results in the notification being lost.

3) Allocation of trivially small fixed size buffers are not resolved
by this change and will still crash the renderer. If it is not
possible to allocate very small objects, the process is probably doomed
anyways.

Intent to ship thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pZ9kld0LehA

BUG=537903,536816,535136,532337
R=binji@chromium.org
==========

to

==========
Fix out-of-memory crashes related to ArrayBuffer allocation

Context: In the past, ArrayBuffer creation would return a null pointer
when memory allocation failed. The problem with that approach was that
not all callsites (and there are many) handle null pointers gracefully,
or check for null pointers at all. So this approach was leading to
potential security vulnerabilities. Right now, failure to allocate an
ArrayBuffer will crash the process, which is secure, but is responsible
for a significant number of crashes. As of Chrome 46, 2% of renderer
crashes on desktop are in WTF::ArrayBufferContents::allocateMemory.
This change should bring that number very close to zero.

Crash mitigation strategy:

1) In accordance with the ECMAScript specification, failure to allocate
the memory block that backs an ArrayBuffer should result in a RangeError
exception being thrown. Many API that internally create ArrayBuffers
are currently crashing instead of throwing an exception. Spec work is
ongoing on a cases by case basis to have the specs for the APIs changed
to state that exceptions thrown by the ArrayBuffer allocation step are
rethrown. These new exceptions, even if they are not caught and result
in script halting, are a much better UX than a process crash.
For example, an ill-behaved ad in an iframe will not interfere the
host page when there is an unhandled OOM condition.

2) In places that are not inside the scope of a script execution
context, we cannot throw exceptions, so we fail silently instead of
crashing if doing so is possible and makes sense.

3) Allocation of trivially small fixed size buffers are not resolved
by this change and will still crash the renderer. If it is not
possible to allocate very small objects, the process is probably doomed
anyways.

4) This patch leaves many non-trivial call sites in an unresolved state,
meaning that they will crash on allocation failure. Site of
these potential
crash have comments leading to crbug.com/536816, and in some cases
suggestions on possible resolution approaches.  The expectations is
that these cases will be addressed as needed based on incoming crash
reports.

Intent to ship thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pZ9kld0LehA

BUG=537903,536816,535136,532337
R=binji@chromium.org
==========

[Justin Novosad, 2015-10-28 18:24:57]

Description was changed from

==========
Fix out-of-memory crashes related to ArrayBuffer allocation

Context: In the past, ArrayBuffer creation would return a null pointer
when memory allocation failed. The problem with that approach was that
not all callsites (and there are many) handle null pointers gracefully,
or check for null pointers at all. So this approach was leading to
potential security vulnerabilities. Right now, failure to allocate an
ArrayBuffer will crash the process, which is secure, but is responsible
for a significant number of crashes. As of Chrome 46, 2% of renderer
crashes on desktop are in WTF::ArrayBufferContents::allocateMemory.
This change should bring that number very close to zero.

Crash mitigation strategy:

1) In accordance with the ECMAScript specification, failure to allocate
the memory block that backs an ArrayBuffer should result in a RangeError
exception being thrown. Many API that internally create ArrayBuffers
are currently crashing instead of throwing an exception. Spec work is
ongoing on a cases by case basis to have the specs for the APIs changed
to state that exceptions thrown by the ArrayBuffer allocation step are
rethrown. These new exceptions, even if they are not caught and result
in script halting, are a much better UX than a process crash.
For example, an ill-behaved ad in an iframe will not interfere the
host page when there is an unhandled OOM condition.

2) In places that are not inside the scope of a script execution
context, we cannot throw exceptions, so we fail silently instead of
crashing if doing so is possible and makes sense.

3) Allocation of trivially small fixed size buffers are not resolved
by this change and will still crash the renderer. If it is not
possible to allocate very small objects, the process is probably doomed
anyways.

4) This patch leaves many non-trivial call sites in an unresolved state,
meaning that they will crash on allocation failure. Site of
these potential
crash have comments leading to crbug.com/536816, and in some cases
suggestions on possible resolution approaches.  The expectations is
that these cases will be addressed as needed based on incoming crash
reports.

Intent to ship thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pZ9kld0LehA

BUG=537903,536816,535136,532337
R=binji@chromium.org
==========

to

==========
Fix out-of-memory crashes related to ArrayBuffer allocation

Context: In the past, ArrayBuffer creation would return a null pointer
when memory allocation failed. The problem with that approach was that
not all callsites (and there are many) handle null pointers gracefully,
or check for null pointers at all. So this approach was leading to
potential security vulnerabilities. Right now, failure to allocate an
ArrayBuffer will crash the process, which is secure, but is responsible
for a significant number of crashes. As of Chrome 46, 2% of renderer
crashes on desktop are in WTF::ArrayBufferContents::allocateMemory.
This change should bring that number very close to zero.

Crash mitigation strategy:

1) In accordance with the ECMAScript specification, failure to allocate
the memory block that backs an ArrayBuffer should result in a RangeError
exception being thrown. Many API that internally create ArrayBuffers
are currently crashing instead of throwing an exception. Spec work is
ongoing on a cases by case basis to have the specs for the APIs changed
to state that exceptions thrown by the ArrayBuffer allocation step are
rethrown. These new exceptions, even if they are not caught and result
in script halting, are a much better UX than a process crash.
For example, an ill-behaved ad in an iframe will not interfere the
host page when there is an unhandled OOM condition.

2) In places that are not inside the scope of a script execution
context, we cannot throw exceptions, so we fail silently instead of
crashing if doing so is possible and makes sense.

3) Allocation of trivially small fixed size buffers are not resolved
by this change and will still crash the renderer. If it is not
possible to allocate very small objects, the process is probably doomed
anyways.

4) This patch leaves many non-trivial call sites in an unresolved state,
meaning that they will crash on allocation failure. Site of these potential
crashes have comments leading to crbug.com/536816, and in some cases
suggestions on possible resolution approaches.  The expectations is
that these cases will be addressed as needed based on incoming crash
reports.

Intent to ship thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pZ9kld0LehA

BUG=537903,536816,535136,532337
R=binji@chromium.org
==========

[Justin Novosad, 2015-10-28 18:26:28]

Description was changed from

==========
Fix out-of-memory crashes related to ArrayBuffer allocation

Context: In the past, ArrayBuffer creation would return a null pointer
when memory allocation failed. The problem with that approach was that
not all callsites (and there are many) handle null pointers gracefully,
or check for null pointers at all. So this approach was leading to
potential security vulnerabilities. Right now, failure to allocate an
ArrayBuffer will crash the process, which is secure, but is responsible
for a significant number of crashes. As of Chrome 46, 2% of renderer
crashes on desktop are in WTF::ArrayBufferContents::allocateMemory.
This change should bring that number very close to zero.

Crash mitigation strategy:

1) In accordance with the ECMAScript specification, failure to allocate
the memory block that backs an ArrayBuffer should result in a RangeError
exception being thrown. Many API that internally create ArrayBuffers
are currently crashing instead of throwing an exception. Spec work is
ongoing on a cases by case basis to have the specs for the APIs changed
to state that exceptions thrown by the ArrayBuffer allocation step are
rethrown. These new exceptions, even if they are not caught and result
in script halting, are a much better UX than a process crash.
For example, an ill-behaved ad in an iframe will not interfere the
host page when there is an unhandled OOM condition.

2) In places that are not inside the scope of a script execution
context, we cannot throw exceptions, so we fail silently instead of
crashing if doing so is possible and makes sense.

3) Allocation of trivially small fixed size buffers are not resolved
by this change and will still crash the renderer. If it is not
possible to allocate very small objects, the process is probably doomed
anyways.

4) This patch leaves many non-trivial call sites in an unresolved state,
meaning that they will crash on allocation failure. Site of these potential
crashes have comments leading to crbug.com/536816, and in some cases
suggestions on possible resolution approaches.  The expectations is
that these cases will be addressed as needed based on incoming crash
reports.

Intent to ship thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pZ9kld0LehA

BUG=537903,536816,535136,532337
R=binji@chromium.org
==========

to

==========
Fix out-of-memory crashes related to ArrayBuffer allocation

Context: In the past, ArrayBuffer creation would return a null pointer
when memory allocation failed. The problem with that approach was that
not all callsites (and there are many) handle null pointers gracefully,
or check for null pointers at all. So this approach was leading to
potential security vulnerabilities. Right now, failure to allocate an
ArrayBuffer will crash the process, which is secure, but is responsible
for a significant number of crashes. As of Chrome 46, 2% of renderer
crashes on desktop are in WTF::ArrayBufferContents::allocateMemory.
This change should bring that number very close to zero.

Crash mitigation strategy:

1) In accordance with the ECMAScript specification, failure to allocate
the memory block that backs an ArrayBuffer should result in a RangeError
exception being thrown. Many API that internally create ArrayBuffers
are currently crashing instead of throwing an exception. Spec work is
ongoing on a cases by case basis to have the specs for the APIs changed
to state that exceptions thrown by the ArrayBuffer allocation step are
rethrown. These new exceptions, even if they are not caught and result
in script halting, are a much better UX than a process crash.
For example, an ill-behaved ad in an iframe will not interfere the
host page when there is an unhandled OOM condition.

2) In places that are not inside the scope of a script execution
context, we cannot throw exceptions, so we fail silently instead of
crashing if doing so is possible and makes sense.

3) Allocation of trivially small fixed size buffers are not resolved
by this change and will still crash the renderer. If it is not
possible to allocate very small objects, the process is probably doomed
anyways.

4) This patch leaves many non-trivial call sites in an unresolved state,
meaning that they will crash on allocation failure. The sites of these
potential crashes now have comments leading to crbug.com/536816,
and in some cases suggestions on possible resolution approaches.
The expectations is that these cases will be addressed as needed, based
on incoming crash reports.

Intent to ship thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pZ9kld0LehA

BUG=537903,536816,535136,532337
R=binji@chromium.org
==========

[Justin Novosad, 2015-10-29 16:07:16]

junov@chromium.org changed reviewers:
+ haraken@chromium.org, senorblanco@google.com

[Justin Novosad, 2015-10-29 16:07:17]

+haraken for Source/bindings/OWNER and for Source/modules/OWNER
(I don't think it is necessary to bother owners for all the individual modules
that have trivial changes)
+senorblanco for Source/modules/canvas2d

New Patch.

I revert many of the behavior changes from previous CL so that they can be
carefully considered one at a time in follow-up CLs.  This patch is now mostly
about propagating the new more explicit function names that replace 'create':
createOrNull and deprecatedCreateOrCrash.  I also added a lot of comments around
the calls site that still need resolution.

In places where the code was clearly already ready to deal with null pointers, I
went ahead and replaced create with createOrNull (which is behavior that
create() had in the past, so it makes sens that some in some place the code is
already ready for this).

This CL does fix the canvas image data APIs by throwing a RangeError when
allocation fails because this particular use case has reached a clear consensus
on blink-dev already.

[haraken, 2015-10-29 16:24:34]

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1583:
NonThrowableExceptionState exceptionState;

Why can't we use a normal ExecptionState?

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1625: //
FIXME(crbug.com/536816): Instead of the following assert, we should consider
doing:

FIXME => TODO(junov)

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:355: // FIXME: Use
AllocateMemoryOrNull. Requires verification that all

FIXME => TODO

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:364: // FIXME: Use
AllocateMemoryOrNull. Requires verification that all

Ditto.

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp
(right):

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/V8BindingForModules.cpp:103: //
FIXME(crbug.com/536816): Find a more graceful way to handle allocation

Ditto.

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1625:
exceptionState.throwRangeError("Out of memory.");

Maybe do we want to invent a better error message for this?

[Stephen White, 2015-10-29 16:29:51]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2015-10-29 16:29:52]

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/crypto/subtle/digest-arraybuffer-oom.html
(right):

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/crypto/subtle/digest-arraybuffer-oom.html:23:
internals.fakeOutOfMemoryForNextArrayBufferAllocation();
Nice!

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:184: //
FIXME: Is there a way to transfer 'buffer' without making a copy?
Log a bug for this?

[Stephen White, 2015-10-29 16:30:35]

modules/canvas2d lgtm, but will leave the rest for haraken

[Justin Novosad, 2015-10-29 18:26:02]

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1583:
NonThrowableExceptionState exceptionState;
On 2015/10/29 16:24:34, haraken wrote:
> 
> Why can't we use a normal ExecptionState?

Added an explanatory comment.

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:184: //
FIXME: Is there a way to transfer 'buffer' without making a copy?
On 2015/10/29 16:29:52, Stephen White wrote:
> Log a bug for this?

Done.

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/1414553002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:1625:
exceptionState.throwRangeError("Out of memory.");
On 2015/10/29 16:24:34, haraken wrote:
> 
> Maybe do we want to invent a better error message for this?

Acknowledged.

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

Finished looking at modules/ and bindings/. Will take a look at core/ tomorrow
:)

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:254:
ASSERT(static_cast<const uint8_t*>(arrayBufferView.bufferBaseOrNull()->data()) +
arrayBufferView.byteOffset() ==

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

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1594:
RELEASE_ASSERT(arrayBuffer); // This is essentially an out of memory crash

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.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:355: // FIXME: Use
AllocateMemoryOrNull. Requires verification that all

Blink now uses TODO. Update all FIXMEs in this CL to TODO(junov).

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/crypto/CryptoResultImpl.cpp:187: // the
promise with a RangeError exception when crewation returns null.

creation

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/crypto/NormalizeAlgorithm.cpp:331: array =
DOMUint8Array::deprecatedCreateOrCrash(0, 0);

Shouldn't this be deprecatedCreateOrCrash(nullptr, 1)?

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Body.cpp:127:
resolver->resolve(DOMArrayBuffer::deprecatedCreateOrCrash(0u, 1));

0u => nullptr

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:230: // is
ascertained that silently droping data is acceptable when the

dropping

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:218:
RELEASE_ASSERT(buffer); // crbug.com/536816

Another idea would be to create bufferOrCrash(). Then you don't need to write
RELEASE_ASSERT in the caller site.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:318:
// determing an accepatble alternative to crashing when buffer

determining an acceptable

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2086: //
FIXME: Should we consider throwing a RangeError exception instead of crashing
when array alloc fails.

allocation

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/DOMWebSocket.cpp:646: // failures.
Should the event be droped? Should we dispatch an event

dropped

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/websockets/DOMWebSocketTest.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/DOMWebSocketTest.cpp:676:
RELEASE_ASSERT(buffer); // Out of momry?

memory

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/DOMWebSocketTest.cpp:685:
RELEASE_ASSERT(buffer); // Out of momry?

memory

[Justin Novosad, 2015-10-29 19:10:10]

junov@chromium.org changed reviewers:
+ wfh@chromium.org

[Justin Novosad, 2015-10-29 19:10:11]

+cc jochen (you asked for it on blink-dev)
+wfh for security review (you volunteered on crbug.com/532337)

[Justin Novosad, 2015-11-05 00:17:52]

New patch.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:254:
ASSERT(static_cast<const uint8_t*>(arrayBufferView.bufferBaseOrNull()->data()) +
arrayBufferView.byteOffset() ==
On 2015/10/29 18:58:37, haraken wrote:
> 
> arrayBufferView.bufferBaseOrNull() can return nullptr. Is this OK to access
> data()?
> 

Done.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:1594:
RELEASE_ASSERT(arrayBuffer); // This is essentially an out of memory crash
On 2015/10/29 18:58:37, haraken wrote:
> 
> 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.

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.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:355: // FIXME: Use
AllocateMemoryOrNull. Requires verification that all
On 2015/10/29 18:58:37, haraken wrote:
> 
> Blink now uses TODO. Update all FIXMEs in this CL to TODO(junov).

Done.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/crypto/CryptoResultImpl.cpp:187: // the
promise with a RangeError exception when crewation returns null.
On 2015/10/29 18:58:37, haraken wrote:
> 
> creation

Done.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/crypto/NormalizeAlgorithm.cpp:331: array =
DOMUint8Array::deprecatedCreateOrCrash(0, 0);
On 2015/10/29 18:58:37, haraken wrote:
> 
> Shouldn't this be deprecatedCreateOrCrash(nullptr, 1)?

Done.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:230: // is
ascertained that silently droping data is acceptable when the
On 2015/10/29 18:58:37, haraken wrote:
> 
> dropping

Done.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:218:
RELEASE_ASSERT(buffer); // crbug.com/536816
On 2015/10/29 18:58:37, haraken wrote:
> 
> Another idea would be to create bufferOrCrash(). Then you don't need to write
> RELEASE_ASSERT in the caller site.

This bit of code was reverted. Getting a buffer out of a bufferview does not
fail.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:318:
// determing an accepatble alternative to crashing when buffer
On 2015/10/29 18:58:37, haraken wrote:
> 
> determining an acceptable

Done.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp
(right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGL2RenderingContextBase.cpp:2086: //
FIXME: Should we consider throwing a RangeError exception instead of crashing
when array alloc fails.
On 2015/10/29 18:58:37, haraken wrote:
> 
> allocation

Done.

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/websockets/DOMWebSocket.cpp (right):

https://codereview.chromium.org/1414553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/websockets/DOMWebSocket.cpp:646: // failures.
Should the event be droped? Should we dispatch an event
On 2015/10/29 18:58:37, haraken wrote:
> 
> dropped

Done.

[Will Harris, 2015-11-11 23:08:53]

Hi. Thanks for doing this!

From a security perspective I think this is very much along the right track.
From what I can tell you have changed all the callers to either a) call the
version that still crashes or b) make sure we handle the nullptr correctly. I
started going through all the call sites in this CL (there are lots of them!)
and haven't found any that don't fit into one of these two buckets, but I wonder
if there is a doc or list of all the sites you have touched just to make it
easier to review or fit in my head?

[Justin Novosad, 2015-11-12 16:15:19]

On 2015/11/11 23:08:53, Will Harris wrote:
> Hi. Thanks for doing this!
> I wonder
> if there is a doc or list of all the sites you have touched just to make it
> easier to review or fit in my head?

My strategy so far has been to document in the code (CL has more comments that
code I think)
I made sure to add a comments with the bug number in it for all the call sites
that still require attention. So once this patch is committed, we will be able
to track with a code search for 536816
I did not think to do anything of the sort for code sites that I consider to be
in their final state, but I guess we can track down calls to createOrNull using
code search xref feature.

I could easily put an inventory into a doc or spreadsheet. If that helps.

FWIW, I do not plan on fixing all the sites myself. Instead I am leaving behind
docs that people may find when investigating crashes. These docs will lead
people this this bug (and to me). Also, we will be able to classify the
different crash sites by frequency and prioritize their resolution
strategically. A lot of them require significant work, including amendments to
standards specifications for features I am unfamiliar with.

[Will Harris, 2015-11-13 18:05:39]

a sheet with all the sites you found would be great, it could also act as a
place we can check off each of them as they are done - if it's not too much
trouble? :)

[jsbell, 2015-11-13 18:15:05]

+1 to a checklist. We can assign owners, etc.

I'd still prefer an initial CL that has the plumbing and mechanically changes
all the call sites to createOrCrash (so no behavior change), followed by more
scoped CLs that change specific modules (etc) one by one. That would be much
easier to review and if we see regressions we can do more scoped reverts while
we re-evaluate.

(Maybe that's the plan and this CL is just a work in progress?)

[jochen (gone - plz use gerrit), 2015-11-24 16:37:11]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-11-24 16:37:12]

what about
https://code.google.com/p/chromium/codesearch#chromium/src/gin/array_buffer.c...
?

[Justin Novosad, 2015-11-24 17:38:10]

On 2015/11/24 16:37:12, jochen wrote:
> what about
>
https://code.google.com/p/chromium/codesearch#chromium/src/gin/array_buffer.c...
> ?

What is that?

[jochen (gone - plz use gerrit), 2015-11-24 17:40:05]

On 2015/11/24 at 17:38:10, junov wrote:
> On 2015/11/24 16:37:12, jochen wrote:
> > what about
> >
https://code.google.com/p/chromium/codesearch#chromium/src/gin/array_buffer.c...
> > ?
> 
> What is that?

it's another array buffer allocator we use, e.g., for the PDF plugin, or the
proxy auto-config