Bug 75532 - [v8] Null pointer exception if a typed array constructor set to a primitive value.
Summary: [v8] Null pointer exception if a typed array constructor set to a primitive v...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ulan Degenbaev
URL:
Keywords:
Depends on: 68890
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-04 01:55 PST by Ulan Degenbaev
Modified: 2012-01-05 12:41 PST (History)
4 users (show)

See Also:


Attachments
Make sure that V8ArrayBufferViewCustomScript.js does not throw exception. (4.42 KB, patch)
2012-01-04 02:35 PST, Ulan Degenbaev
no flags Details | Formatted Diff | Diff
More robust guard (4.44 KB, patch)
2012-01-04 06:24 PST, Ulan Degenbaev
no flags Details | Formatted Diff | Diff
Add TryCatch scope (5.19 KB, patch)
2012-01-05 02:34 PST, Ulan Degenbaev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ulan Degenbaev 2012-01-04 01:55:49 PST
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.
Comment 1 Ulan Degenbaev 2012-01-04 02:35:16 PST
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.
Comment 2 Ulan Degenbaev 2012-01-04 06:24:15 PST
Created attachment 121106 [details]
More robust guard

Avoid using hasOwnProperty since it might be also redefined.
Comment 3 Adam Barth 2012-01-04 07:48:50 PST
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.
Comment 4 Ulan Degenbaev 2012-01-04 08:32:09 PST
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.
Comment 5 Adam Barth 2012-01-04 09:06:45 PST
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?
Comment 6 Kenneth Russell 2012-01-04 10:24:17 PST
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?
Comment 7 Kenneth Russell 2012-01-04 11:33:44 PST
(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.
Comment 8 Ulan Degenbaev 2012-01-05 02:34:53 PST
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.
Comment 9 Kenneth Russell 2012-01-05 10:29:38 PST
Comment on attachment 121246 [details]
Add TryCatch scope

Thanks, looks great.
Comment 10 Adam Barth 2012-01-05 11:27:01 PST
Ok.  Thanks.
Comment 11 WebKit Review Bot 2012-01-05 12:41:32 PST
Comment on attachment 121246 [details]
Add TryCatch scope

Clearing flags on attachment: 121246

Committed r104196: <http://trac.webkit.org/changeset/104196>
Comment 12 WebKit Review Bot 2012-01-05 12:41:36 PST
All reviewed patches have been landed.  Closing bug.