document.location bindings fix

Source/bindings/templates/attributes.cpp

 {##############################################################################}
 {% macro attribute_getter(attribute, world_suffix) %}
 {% filter conditional(attribute.conditional_string) %}
 static void {{attribute.name}}AttributeGetter{{world_suffix}}(
 {%- if attribute.is_expose_js_accessors %}
 const v8::FunctionCallbackInfo<v8::Value>& info
 {%- else %}
 const v8::PropertyCallbackInfo<v8::Value>& info
 {%- endif %})
 {
@@ skipping 214 matching lines @@
     {# Type checking for interface types (if interface not implemented, throw
        TypeError), per http://www.w3.org/TR/WebIDL/#es-interface #}
     if (!isUndefinedOrNull(jsValue) && !V8{{attribute.idl_type}}::hasInstance(jsValue, info.GetIsolate())) {
         exceptionState.throwTypeError("The provided value is not of type '{{attribute.idl_type}}'.");
         exceptionState.throwIfNeeded();
         return;
     }
     {% endif %}
     {% if attribute.put_forwards %}
     {{cpp_class}}* proxyImp = {{v8_class}}::toNative(info.Holder());
-    {{attribute.idl_type}}* imp = WTF::getPtr(proxyImp->{{attribute.name}}());
+    RefPtr<{{attribute.idl_type}}> imp = WTF::getPtr(proxyImp->{{attribute.name}}());
     if (!imp)
         return;
     {% elif not attribute.is_static %}
     {{cpp_class}}* imp = {{v8_class}}::toNative(info.Holder());
     {% endif %}{# imp #}
     {# FIXME: move ASSERT(imp) here. #}
     {% if attribute.idl_type == 'EventHandler' and interface_name == 'Window' %}
     if (!imp->document())
         return;
     {% endif %}
     {% if attribute.idl_type != 'EventHandler' %}
     {{attribute.v8_value_to_local_cpp_value}};

[jochen (gone - plz use gerrit), 2014/03/13 23:26:49]

alternatively, we could just move this up, no?

[Nils Barth (inactive), 2014/03/20 07:20:29]

How would this help?
(Honest question; might be a good change.)

I.e., would it be good to do the local variable assignment
*before* we get impl?

AFAICT the problem was in the assignment to |imp| (renamed to |impl|), right?

{{v8_value_to_local_cpp_value}} assigns to a local variable
whose type depends on the attribute (in this case
the target attribute of the [PutForwards]).
In this case Document.location (type Location)
forwards to Location.href (type DOMString)
...so the line it generates is:
V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, cppValue, jsValue);

Would putting this before the |impl| setting catch errors earlier?
(e.g., on invalid/empty input)

[jochen (gone - plz use gerrit), 2014/03/20 07:42:13]

this executes arbitrary javascript. Local variables assigned before aren't
guaranteed to be valid anymore afterwards.

[Nils Barth (inactive), 2014/03/24 05:06:09]

Ok, so rearranging the generated code wouldn't help, right?

     {% elif not is_node %}{# EventHandler hack #}
     {# FIXME: duplicate of below; remove #}
     {% if attribute.is_implemented_by and not attribute.is_static %}
     ASSERT(imp);
     {% endif %}
     moveEventListenerToNewWrapper(info.Holder(), {{attribute.event_handler_getter_expression}}, jsValue, {{v8_class}}::eventListenerCacheIndex, info.GetIsolate());
     {% endif %}
     {% if attribute.enum_validation_expression %}
     {# Setter ignores invalid enum values: http://www.w3.org/TR/WebIDL/#idl-enums #}
     String string = cppValue;
@@ skipping 57 matching lines @@
     {% endif %}
     {% if attribute.has_custom_setter %}
     {{v8_class}}::{{attribute.name}}AttributeSetterCustom(jsValue, info);
     {% else %}
     {{cpp_class}}V8Internal::{{attribute.name}}AttributeSetter{{world_suffix}}(jsValue, info);
     {% endif %}
     TRACE_EVENT_SET_SAMPLING_STATE("V8", "V8Execution");
 }
 {% endfilter %}
 {% endmacro %}