[webkit-reviews] review canceled: [Bug 106308] [Chromium] WebGL typed array constructor crashes on exception : [Attachment 182243] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has canceled Kenneth Russell
<kbr at google.com>'s request for review:
Bug 106308: [Chromium] WebGL typed array constructor crashes on exception
https://bugs.webkit.org/show_bug.cgi?id=106308

Attachment 182243: Patch
https://bugs.webkit.org/attachment.cgi?id=182243&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=182243&action=review


>> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:176
>> +		// Exception thrown during fetch of length property.
> 
> Another way to detect an exception is:
> 
>   v8::TryCatch block;
>   v8::Local<v8::Value> val = srcArray->Get(v8::String::NewSymbol("length"));
>   if (block->hadException())
>     return v8Null();
> 
> But as far as I see other code, it looks like we are likely to check
IsEmpty() rather than using v8::TryCatch. So your code looks fine.

Yes, I saw the TryCatch class in the V8 API but it seemed more direct to check
for the failure case.

>> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:177
>> +		return v8::Null();
> 
> Nit: v8::Null() => v8Null()
> 
> Are you sure that this should be not v8Undefined() but v8Null()?

Looking at other similar code it should be v8Undefined. The return value is
ignored however since the exception is thrown to the calling code. I'll upload
a new patch.