Canvas2D Performance: fix the bottleneck of hasInstance in JS binding -- TypeChecking Interface

Source/bindings/scripts/v8_types.py

Index: Source/bindings/scripts/v8_types.py
diff --git a/Source/bindings/scripts/v8_types.py b/Source/bindings/scripts/v8_types.py
index a63a39326c34290d437e66f575a8979d8e177ec1..c5386332ab68783947cef766934f2463ce8048f6 100644
--- a/Source/bindings/scripts/v8_types.py
+++ b/Source/bindings/scripts/v8_types.py
@@ -501,7 +501,7 @@ def v8_conversion_is_trivial(idl_type):
 IdlType.v8_conversion_is_trivial = property(v8_conversion_is_trivial)
 
 
-def v8_value_to_cpp_value(idl_type, extended_attributes, v8_value, index, isolate):
+def v8_value_to_cpp_value(idl_type, extended_attributes, v8_value, type_checked, index, isolate):
     if idl_type.name == 'void':
         return ''
 
@@ -532,6 +532,9 @@ def v8_value_to_cpp_value(idl_type, extended_attributes, v8_value, index, isolat
             'V8{idl_type}::toImpl(v8::Handle<v8::{idl_type}>::Cast({v8_value})) : 0')
     elif idl_type.is_dictionary:
         cpp_expression_format = 'V8{idl_type}::toImpl({isolate}, {v8_value}, exceptionState)'
+    elif type_checked:
+        cpp_expression_format = (
+            'V8{idl_type}::toImpl(v8::Handle<v8::Object>::Cast({v8_value}))')

[Jens Widell, 2014/10/02 12:18:14]

This isn't safe if the argument is nullable or if it's optional with
[Default=Undefined]. In both cases, the V8 value can be null or undefined, in
which case v8::Handle<v8::Object>::Cast() is invalid to use.

IOW, toImplWithTypeCheck() doesn't just do hasInstance(), it also implicitly
checks if the value is an object.

Test case: <script>document.body.insertBefore(new Text, null)</script>

[yunchao, 2014/10/08 07:04:58]

Hi Jens, thanks for your review. However, nullable or undefined arguments have
been checked already 
if the method is a TypeCheck one. You can see the code in
bindings/templates/methods.cpp:117, 
right before hasInstance is called. For the test case you pointed out:

  <script>document.body.insertBefore(new Text, null)</script>

You can double check the generated code of insertBeforeMethod 
in src/out/Release/gen/blink/bindings/core/v8/V8Node.cpp

[Jens Widell, 2014/10/08 08:11:13]

Did you try my testcase? With your patch applied to current master, it crashes
for me, just as I'd expect, for the reasons I explained above.

To summarize: Yes, it's type checked already, but it's type checked as "null,
undefined or an object of the correct type." If it was null or undefined,
v8::Handle<v8::Object>::Cast() is invalid and crashes. If it was an object, then
all is good.

For arguments that aren't nullable and aren't optional with [Default=Undefined]
the type check also fails if the value is null or undefined, so for those
arguments, using v8::Handle<v8::Object>::Cast() like you do here is safe.

[yunchao, 2014/10/08 11:23:23]

You are correct, Jens. So I want to narrow down to arguments who are not
nullable. As you know, I am aim at improving the performance of Canvas2D,
arguments in drawImage API in CanvasRenderingContext2D.idl are not nullable,
they can get improvement by this change. And some other APIs may be also
improved.

[Jens Widell, 2014/10/08 11:51:26]

This still generates code that crashes in the [Default=Undefined] case.

I don't think we have any overlap between [Default=Undefined] (on interface-type
arguments) and [TypeChecking=Interface] in current IDL files, but this is
largely because [TypeChecking=Interface] isn't widely used. Anyway, we still
shouldn't knowingly add the possibility to introduce a crash bug simply by doing
valid changes to IDL files, so we should fix this.

See comment elsewhere for proposed way to do that.

     else:
         cpp_expression_format = (
             'V8{idl_type}::toImplWithTypeCheck({isolate}, {v8_value})')
@@ -560,7 +563,7 @@ def v8_value_to_cpp_value_array_or_sequence(native_array_element_type, v8_value,
     return expression
 
 
-def v8_value_to_local_cpp_value(idl_type, extended_attributes, v8_value, variable_name, index=None, declare_variable=True, isolate='info.GetIsolate()', used_in_private_script=False, return_promise=False):
+def v8_value_to_local_cpp_value(idl_type, extended_attributes, v8_value, variable_name, type_checked=False, index=None, declare_variable=True, isolate='info.GetIsolate()', used_in_private_script=False, return_promise=False):

[haraken, 2014/10/02 11:52:57]

type_checked => needs_type_check (and flip the true/false)

     """Returns an expression that converts a V8 value to a C++ value and stores it as a local value."""
 
     # FIXME: Support union type.
@@ -573,7 +576,7 @@ def v8_value_to_local_cpp_value(idl_type, extended_attributes, v8_value, variabl
     if idl_type.base_type in ('void', 'object', 'EventHandler', 'EventListener'):
         return '/* no V8 -> C++ conversion for IDL type: %s */' % idl_type.name
 
-    cpp_value = v8_value_to_cpp_value(idl_type, extended_attributes, v8_value, index, isolate)
+    cpp_value = v8_value_to_cpp_value(idl_type, extended_attributes, v8_value, type_checked, index, isolate)
     if idl_type.is_string_type or idl_type.v8_conversion_needs_exception_state:
         # Types that need error handling and use one of a group of (C++) macros
         # to take care of this.