Fix memory leak on serializing neutered ArrayBuffer.

Fix memory leak on serializing neutered ArrayBuffer.

If the serialization of an ArrayBufferView's ArrayBuffer fails, unwind
the state stack completely before reporting an error to avoid leaking
a serializer state object.

To accommodate, split out the state stack overflow checking from the
dispatching on value types which was both performed by the main
serialization method. Now handled by

    Serializer::doSerialize()
    Serializer::doSerializeImpl()

respectively, with the former calling upon the latter if the stack
check passes.

R=dslomov@chromium.org
BUG=291240
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161343

[sof, 2013-11-02 16:36:21]

Please have a look.

[Dmitry Lomov (no reviews), 2013-11-04 10:15:34]

https://codereview.chromium.org/56973002/diff/1/Source/bindings/v8/Serialized...
File Source/bindings/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/56973002/diff/1/Source/bindings/v8/Serialized...
Source/bindings/v8/SerializedScriptValue.cpp:1142: if (StateBase* stateOut =
doSerialize(underlyingBuffer, 0)) {
style nit: I am a simple programmer and variable declarations inside if
conditions frighten and confuse me. 
Please move the declaration outside if statement (I know other similar
statements in this file follow this style, but this is just WebKit legacy - feel
free to change the other occurences too too)

https://codereview.chromium.org/56973002/diff/1/Source/bindings/v8/Serialized...
Source/bindings/v8/SerializedScriptValue.cpp:1144: stateOut = pop(stateOut);
Hmm I do not like this popping out loop (It repeats what happens in
handleError). If I understand correctly, the correct fix here would be to pass
'next' into doSerialize (instead of 0 second parameter) and then just return the
result if if it is non-null.

[sof, 2013-11-04 10:21:14]

https://codereview.chromium.org/56973002/diff/1/Source/bindings/v8/Serialized...
File Source/bindings/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/56973002/diff/1/Source/bindings/v8/Serialized...
Source/bindings/v8/SerializedScriptValue.cpp:1142: if (StateBase* stateOut =
doSerialize(underlyingBuffer, 0)) {
On 2013/11/04 10:15:35, Dmitry Lomov (chromium) wrote:
> style nit: I am a simple programmer and variable declarations inside if
> conditions frighten and confuse me. 
> Please move the declaration outside if statement (I know other similar
> statements in this file follow this style, but this is just WebKit legacy -
feel
> free to change the other occurences too too)

alright. One of the likeable syntactic features of c++, imo.

https://codereview.chromium.org/56973002/diff/1/Source/bindings/v8/Serialized...
Source/bindings/v8/SerializedScriptValue.cpp:1144: stateOut = pop(stateOut);
On 2013/11/04 10:15:35, Dmitry Lomov (chromium) wrote:
> Hmm I do not like this popping out loop (It repeats what happens in
> handleError). If I understand correctly, the correct fix here would be to pass
> 'next' into doSerialize (instead of 0 second parameter) and then just return
the
> result if if it is non-null.

Hmm, and we won't run into the depth check at the front of doSerialize() by
propagating the stack (represented by 'next')?

[Dmitry Lomov (no reviews), 2013-11-04 11:05:47]

On 2013/11/04 10:21:14, sof wrote:
>
https://codereview.chromium.org/56973002/diff/1/Source/bindings/v8/Serialized...
> File Source/bindings/v8/SerializedScriptValue.cpp (right):
> 
>
https://codereview.chromium.org/56973002/diff/1/Source/bindings/v8/Serialized...
> Source/bindings/v8/SerializedScriptValue.cpp:1142: if (StateBase* stateOut =
> doSerialize(underlyingBuffer, 0)) {
> On 2013/11/04 10:15:35, Dmitry Lomov (chromium) wrote:
> > style nit: I am a simple programmer and variable declarations inside if
> > conditions frighten and confuse me. 
> > Please move the declaration outside if statement (I know other similar
> > statements in this file follow this style, but this is just WebKit legacy -
> feel
> > free to change the other occurences too too)
> 
> alright. One of the likeable syntactic features of c++, imo.

I do not want to get into a style war here, but clang (which we run with
warnings-as-errors for chromium, but, admittedly, not for blink yet) will
require double parenthezation here:
  if ((StateBase* state = ...)) ...


> 
>
https://codereview.chromium.org/56973002/diff/1/Source/bindings/v8/Serialized...
> Source/bindings/v8/SerializedScriptValue.cpp:1144: stateOut = pop(stateOut);
> On 2013/11/04 10:15:35, Dmitry Lomov (chromium) wrote:
> > Hmm I do not like this popping out loop (It repeats what happens in
> > handleError). If I understand correctly, the correct fix here would be to
pass
> > 'next' into doSerialize (instead of 0 second parameter) and then just return
> the
> > result if if it is non-null.
> 
> Hmm, and we won't run into the depth check at the front of doSerialize() by
> propagating the stack (represented by 'next')?

Yes you are right: we are avoiding the depth check here because we know that
ArrayBuffer is a leaf object and will always require constant system stack for
serialization.
I suggest we make this assumption (more) explicit:
- Factor out everything besides depth check from doSerialize into
doSerializeImpl.
- Introduce a new method doSerializeArrayBuffer that calls into doSerializeImpl.

[sof, 2013-11-04 17:10:45]

On 2013/11/04 11:05:47, Dmitry Lomov (chromium) wrote:
> On 2013/11/04 10:21:14, sof wrote:
> >
>
https://codereview.chromium.org/56973002/diff/1/Source/bindings/v8/Serialized...
> > File Source/bindings/v8/SerializedScriptValue.cpp (right):
> > 
> >
>
https://codereview.chromium.org/56973002/diff/1/Source/bindings/v8/Serialized...
> > Source/bindings/v8/SerializedScriptValue.cpp:1142: if (StateBase* stateOut =
> > doSerialize(underlyingBuffer, 0)) {
> > On 2013/11/04 10:15:35, Dmitry Lomov (chromium) wrote:
> > > style nit: I am a simple programmer and variable declarations inside if
> > > conditions frighten and confuse me. 
> > > Please move the declaration outside if statement (I know other similar
> > > statements in this file follow this style, but this is just WebKit legacy
-
> > feel
> > > free to change the other occurences too too)
> > 
> > alright. One of the likeable syntactic features of c++, imo.
> 
> I do not want to get into a style war here, but clang (which we run with
> warnings-as-errors for chromium, but, admittedly, not for blink yet) will
> require double parenthezation here:
>   if ((StateBase* state = ...)) ...
> 

Thanks for filling me in on the style that's in effect; I didn't know from
the context here.

[Dmitry Lomov (no reviews), 2013-11-05 10:56:09]

LGTM with a comment on comment

https://codereview.chromium.org/56973002/diff/90001/Source/bindings/v8/Serial...
File Source/bindings/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/56973002/diff/90001/Source/bindings/v8/Serial...
Source/bindings/v8/SerializedScriptValue.cpp:1156: // (but might fail and unwind
our current stack.)
Keep the statement "the call to doSerializeArrayBuffer should neither cause the
system stack to overflow nor should it have potential to reach this
ArrayBufferView again".

[sof, 2013-11-05 14:13:44]

On 2013/11/05 10:56:09, Dmitry Lomov (chromium) wrote:
> LGTM with a comment on comment
> 
>
https://codereview.chromium.org/56973002/diff/90001/Source/bindings/v8/Serial...
> File Source/bindings/v8/SerializedScriptValue.cpp (right):
> 
>
https://codereview.chromium.org/56973002/diff/90001/Source/bindings/v8/Serial...
> Source/bindings/v8/SerializedScriptValue.cpp:1156: // (but might fail and
unwind
> our current stack.)
> Keep the statement "the call to doSerializeArrayBuffer should neither cause
the
> system stack to overflow nor should it have potential to reach this
> ArrayBufferView again".

Addressed; thanks very much for the review. Guess I need approval from a
bindings/ owner before being able to land.

[jochen (gone - plz use gerrit), 2013-11-05 14:41:00]

rubberstamp lgtm

[commit-bot: I haz the power, 2013-11-05 14:49:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/56973002/140001

[commit-bot: I haz the power, 2013-11-05 15:42:41]

Change committed as 161343