[v8] Null pointer exception if a typed array constructor set to a primitive value.

• Component: WebGL
• Status: RESOLVED FIXED

[Ulan Degenbaev]

Running the following script generates a null pointer exception:
<script>
  window.Uint8Array = 0;
  new Float64Array(function () {});
</script>

For more details, see http://code.google.com/p/v8/issues/detail?id=1877.

[Ulan Degenbaev]

Created attachment 121089 [details]
Make sure that V8ArrayBufferViewCustomScript.js does not throw exception.

Added guards in V8ArrayBufferViewCustomScript.js so that we do not try to optimize the "set" method if the typed array constructor was redefined.

[Ulan Degenbaev]

Created attachment 121106 [details]
More robust guard

Avoid using hasOwnProperty since it might be also redefined.

[Adam Barth]

Comment on attachment 121106 [details]
More robust guard

View in context: https://bugs.webkit.org/attachment.cgi?id=121106&action=review

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustomScript.js:27
>  var optimizeSetMethod = function(type)

I'm not really familiar with this file.  Is there someone you had in mind to review this patch?  If not, I can study the code and try to review it.

[Ulan Degenbaev]

Thanks Adam, I was hoping that Ken could review the patch since he reviewed my previous patches and is familiar with the code.

(In reply to comment #3)
> (From update of attachment 121106 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121106&action=review
> 
> > Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustomScript.js:27
> >  var optimizeSetMethod = function(type)
> 
> I'm not really familiar with this file.  Is there someone you had in mind to review this patch?  If not, I can study the code and try to review it.

[Adam Barth]

In general writing this kind of patch script for JavaScript is impossible to get right.  In your patch, for example, the new version of set will trigger a getter on the global object for Array (e.g., window.__defineGetter__("Array", function() {...}); )

Is there no way to avoid trying to patch the context with JavaScript?

[Kenneth Russell]

Comment on attachment 121106 [details]
More robust guard

Thanks for the quick fix. The revised code looks fine, but for better robustness I think the following two lines should also be added above the call to "script->Run()" in installFastSet (V8ArrayBufferViewCustom.cpp):

  v8::TryCatch tryCatch;
  tryCatch.SetVerbose(true);

Would those two lines have also worked around the problem?

[Kenneth Russell]

(In reply to comment #5)
> In general writing this kind of patch script for JavaScript is impossible to get right.  In your patch, for example, the new version of set will trigger a getter on the global object for Array (e.g., window.__defineGetter__("Array", function() {...}); )
> 
> Is there no way to avoid trying to patch the context with JavaScript?

I don't know whether it might be possible to implement more of this patching logic using the V8 API; however, having the JavaScript wrapper for the set() method is essential for performance.

[Ulan Degenbaev]

Created attachment 121246 [details]
Add TryCatch scope

Thank you for the suggestion, Ken. Those two lines even without guards would indeed fix the issue.

Adam, I had a solution that avoided patching, but it was several factors slower then the current solution because V8 couldn't inline it.

[Kenneth Russell]

Comment on attachment 121246 [details]
Add TryCatch scope

Thanks, looks great.

[Adam Barth]

Ok.  Thanks.

[WebKit Review Bot]

Comment on attachment 121246 [details]
Add TryCatch scope

Clearing flags on attachment: 121246

Committed r104196: <http://trac.webkit.org/changeset/104196>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.