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

Source/bindings/scripts/v8_types.py

 # Copyright (C) 2013 Google Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 # copyright notice, this list of conditions and the following disclaimer
@@ skipping 483 matching lines @@
 
 def v8_conversion_is_trivial(idl_type):
     # The conversion is a simple expression that returns the converted value and
     # cannot raise an exception.
     return (idl_type.base_type in TRIVIAL_CONVERSIONS or
             idl_type.is_wrapper_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 ''
 
     # Array or sequence types
     native_array_element_type = idl_type.native_array_element_type
     if native_array_element_type:
         return v8_value_to_cpp_value_array_or_sequence(native_array_element_type, v8_value, index)
 
     # Simple types
     idl_type = idl_type.preprocessed_type
@@ skipping 10 matching lines @@
         arguments = v8_value
 
     if base_idl_type in V8_VALUE_TO_CPP_VALUE:
         cpp_expression_format = V8_VALUE_TO_CPP_VALUE[base_idl_type]
     elif idl_type.is_typed_array_element_type:
         cpp_expression_format = (
             '{v8_value}->Is{idl_type}() ? '
             '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})')
 
     return cpp_expression_format.format(arguments=arguments, idl_type=base_idl_type, v8_value=v8_value, isolate=isolate)
 
 
 def v8_value_to_cpp_value_array_or_sequence(native_array_element_type, v8_value, index, isolate='info.GetIsolate()'):
     # Index is None for setters, index (starting at 0) for method arguments,
     # and is used to provide a human-readable exception message
     if index is None:
         index = 0  # special case, meaning "setter"
     else:
         index += 1  # human-readable index
     if (native_array_element_type.is_interface_type and
         native_array_element_type.name != 'Dictionary'):
         this_cpp_type = None
         ref_ptr_type = cpp_ptr_type('RefPtr', 'Member', native_array_element_type.gc_type)
         expression_format = '(to{ref_ptr_type}NativeArray<{native_array_element_type}, V8{native_array_element_type}>({v8_value}, {index}, {isolate}, exceptionState))'
         add_includes_for_type(native_array_element_type)
     else:
         ref_ptr_type = None
         this_cpp_type = native_array_element_type.cpp_type
         expression_format = 'toImplArray<{cpp_type}>({v8_value}, {index}, {isolate}, exceptionState)'
     expression = expression_format.format(native_array_element_type=native_array_element_type.name, cpp_type=this_cpp_type, index=index, ref_ptr_type=ref_ptr_type, v8_value=v8_value, isolate=isolate)
     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.
     if idl_type.is_union_type:
         return '/* no V8 -> C++ conversion for IDL union type: %s */' % idl_type.name
 
     this_cpp_type = idl_type.cpp_type_args(extended_attributes=extended_attributes, raw_type=True)
     idl_type = idl_type.preprocessed_type
 
     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.
 
         args = [variable_name, cpp_value]
 
         if idl_type.v8_conversion_needs_exception_state:
             macro = 'TONATIVE_DEFAULT_EXCEPTIONSTATE' if used_in_private_script else 'TONATIVE_VOID_EXCEPTIONSTATE'
         elif return_promise:
             macro = 'TOSTRING_VOID_EXCEPTIONSTATE'
@@ skipping 277 matching lines @@
 
 
 def is_explicit_nullable(idl_type):
     # Nullable type that isn't implicit nullable (see above.) For such types,
     # we use Nullable<T> or similar explicit ways to represent a null value.
     return idl_type.is_nullable and not idl_type.is_implicit_nullable
 
 IdlTypeBase.is_implicit_nullable = property(is_implicit_nullable)
 IdlUnionType.is_implicit_nullable = False
 IdlTypeBase.is_explicit_nullable = property(is_explicit_nullable)