Reimplement [PutForwards] per spec

third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl

 {% from 'utilities.cpp.tmpl' import declare_enum_validation_variable, v8_value_to_local_cpp_value %}
 
 {##############################################################################}
 {% macro attribute_getter(attribute, world_suffix) %}
 static void {{attribute.name}}AttributeGetter{{world_suffix}}(
 {%- if attribute.is_data_type_property %}
 const v8::PropertyCallbackInfo<v8::Value>& info
 {%- else %}
 const v8::FunctionCallbackInfo<v8::Value>& info
 {%- endif %}) {
@@ skipping 275 matching lines @@
   ExceptionState exceptionState(info.GetIsolate(), ExceptionState::SetterContext, "{{interface_name}}", "{{attribute.name}}");
   {%- endset %}
 
   {% if attribute.is_lenient_this %}
   // [LenientThis]
   // Make sure that info.Holder() really points to an instance if [LenientThis].
   if (!{{v8_class}}::hasInstance(info.Holder(), info.GetIsolate()))
     return; // Return silently because of [LenientThis].
   {% endif %}
 
-  {% if not attribute.is_static and not attribute.is_replaceable %}
+  {% set check_security_for_receiver = attribute.is_check_security_for_receiver and not attribute.is_data_type_property %}

[Jens Widell, 2017/03/06 14:05:28]

The reason I changed things a bit around the security check was so that the
check would still be generated for [PutForwards] attributes, if it was before my
change. Which, I might add, it never was, so this might be quite pointless. Not
even the `Window.location` setter had a security check, because its
`attribute.is_data_type_property` was true.

[haraken, 2017/03/06 18:57:28]

Just to confirm: This CL is not changing the condition where the security check
is generated, right?

+
+  {% if not attribute.is_static and not attribute.is_replaceable and
+        (not attribute.is_put_forwards or check_security_for_receiver) %}
   v8::Local<v8::Object> holder = info.Holder();
-  {% if attribute.is_put_forwards %}
-  {{cpp_class}}* proxyImpl = {{v8_class}}::toImpl(holder);
-  {{attribute.cpp_type}} impl = WTF::getPtr(proxyImpl->{{attribute.name}}());
-  if (!impl)
-    return;
-  {% else %}
   {% set local_dom_window_only = interface_name == 'Window' and not attribute.has_cross_origin_setter %}
   {% if local_dom_window_only %}
   {% if attribute.is_check_security_for_receiver %}
   {{cpp_class}}* uncheckedImpl = {{v8_class}}::toImpl(holder);
   {% else %}
   // Same-origin attributes setters are never exposed via the cross-origin
   // interceptors. Since same-origin access requires a LocalDOMWindow, it is
   // safe to downcast here.
   LocalDOMWindow* impl = toLocalDOMWindow({{v8_class}}::toImpl(holder));
   {% endif %}{# attribute.is_check_security_for_receiver #}
   {% else %}
   {{cpp_class}}* impl = {{v8_class}}::toImpl(holder);
   {% endif %}{# local_dom_window_only #}
   {% endif %}
-  {% endif %}
 
-  {% if attribute.is_check_security_for_receiver and not attribute.is_data_type_property %}
+  {% if check_security_for_receiver %}
   // Perform a security check for the receiver object.
   {{define_exception_state}}
   {% if local_dom_window_only %}
   if (!BindingSecurity::shouldAllowAccessTo(currentDOMWindow(info.GetIsolate()), uncheckedImpl, exceptionState)) {
   {% else %}
   if (!BindingSecurity::shouldAllowAccessTo(currentDOMWindow(info.GetIsolate()), impl, exceptionState)) {
   {% endif %}{# local_dom_window_only #}
     v8SetReturnValue(info, v8Value);
     return;
   }
   {% if local_dom_window_only %}
   LocalDOMWindow* impl = toLocalDOMWindow(uncheckedImpl);
   {% endif %}{# local_dom_window_only #}
   {% endif %}
 
   {% if attribute.is_check_security_for_return_value %}
 #error Attribute setter with the security check for the return value is not supported.  Since the return value is the given value to be set, it\'s meaningless to perform the security check for the return value.
   {% endif %}
 
+  {% if attribute.is_put_forwards %}
+  {{define_exception_state}}
+  v8::Local<v8::Value> target;
+  if (!info.Holder()->Get(info.GetIsolate()->GetEnteredContext(), v8String(info.GetIsolate(), "{{attribute.name}}")).ToLocal(&target))
+    return;
+  if (!target->IsObject()) {
+    exceptionState.throwTypeError("The attribute value is not an object");
+    return;
+  }
+  target.As<v8::Object>()->Set(info.GetIsolate()->GetEnteredContext(), v8String(info.GetIsolate(), "{{attribute.target_attribute_name}}"), v8Value).IsNothing();

[Jens Widell, 2017/03/06 14:05:28]

Note on use of `GetEnteredContext()` here: using `GetCurrentContext()`, which
was my first choice, causes incorrect behavior when resolving the URL assigned
to `window.location`, if one frame (A) calls a function in another frame (B) to
perform the assignment. In this case, A's URL should be used to resolve the
assigned value. If I use `GetCurrentContext()` here, B's URL is used.

LayoutTests/fast/dom/Window/window-property-clearing.html breaks when getting
this wrong. (As would, I assume, half the Internet.)

I believe there's a lingering compat bug in Chromium here. The same incorrect
URL resolving behavior can be triggered by making an assignment to
`window.location` (or `location.href` directly) by way of e.g. an object's
`toString()` function via a conversion performed by the bindings layer:

  function navigate1(relative_url) {
    some_window.location = relative_url;
  }

  function navigate2(relative_url) {
    document.title = {
      toString: function () {
        some_window.location = relative_url;
        return "dummy title";
      }
    };
  }

These two functions should always cause the same navigation, but `navigate2`
will always resolve relative the URL of the document in which it was defined,
whereas `navigate1` correctly resolves relative the URL of the document from
which it was called.

Chrome and Firefox behaves different in this scenario.

[haraken, 2017/03/06 18:57:28]

Yeah, I think this is because Chromium has not yet implemented the "cross-origin
appropriate representation" correctly. (It looks like the spec changed the term;
I haven't yet caught up the latest spec.)

We have a couple of hacks in V8DOMWrapper.cpp to "adjust" a context from which
an exception is thrown for window.location
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...),
but it's not sufficient to handle all cases. I think this is what you're hitting
here.

yukishiino@ and jochen@ would know better about this.

I'd prefer using GetCurrentContext() and accepting the wrong behavior until
Chromium implements window.location correctly though. GetEnteredContext()
wouldn't give you a right context if you start the JS execution from frame 1,
call frame 2, call frame 3 and then run window.location. Then
GetEnteredContext() returns frame 1 although what you need is frame 2.

[jochen (gone - plz use gerrit), 2017/03/07 11:45:06]

GetEnteredContext is also wrong during microtask execution

[haraken, 2017/03/07 12:15:22]

Jens: I'm confused. The context parameter in Get()/Set() should be used only to
specify a context from which an exception is thrown. Why does it affect the
assignment behavior of window.location?

+  {% else %}
   {% if attribute.is_custom_element_callbacks or
         (attribute.is_reflect and not (attribute.idl_type == 'DOMString' and is_node)) %}
   // Skip on compact node DOMString getters.
   V0CustomElementProcessingStack::CallbackDeliveryScope deliveryScope;
   {% endif %}
 
   {% if attribute.has_setter_exception_state %}
   {{define_exception_state}}
   {% endif %}
 
@@ skipping 40 matching lines @@
   {% if attribute.is_replaceable %}
   v8::Local<v8::String> propertyName = v8AtomicString(info.GetIsolate(), "{{attribute.name}}");
   {% endif %}
   {{attribute.cpp_setter}};
 
   {% if attribute.cached_attribute_validation_method %}
   // [CachedAttribute]
   // Invalidate the cached value.
   V8HiddenValue::deleteHiddenValue(ScriptState::forFunctionObject(info), holder, v8AtomicString(info.GetIsolate(), "{{attribute.name}}"));
   {% endif %}
+  {% endif %}{# is_put_forwards #}
 }
 {% endfilter %}{# format_remove_duplicates #}
 {% endmacro %}
 
 
 {##############################################################################}
 {% macro attribute_setter_callback(attribute, world_suffix) %}
 void {{v8_class_or_partial}}::{{attribute.name}}AttributeSetterCallback{{world_suffix}}(
 {%- if attribute.is_data_type_property %}
 v8::Local<v8::Name>, v8::Local<v8::Value> v8Value, const v8::PropertyCallbackInfo<void>& info
@@ skipping 101 matching lines @@
 {% filter exposed(attribute.exposed_test) %}
 {% filter secure_context(attribute.secure_context_test) %}
 {% filter runtime_enabled(attribute.runtime_enabled_feature_name) %}
 const V8DOMConfiguration::AccessorConfiguration accessorConfiguration = {{attribute_configuration(attribute)}};
 V8DOMConfiguration::installAccessor(isolate, world, v8::Local<v8::Object>(), prototypeObject, interfaceObject, signature, accessorConfiguration);
 {% endfilter %}{# runtime_enabled #}
 {% endfilter %}{# secure_context #}
 {% endfilter %}{# exposed #}
 {% endfor %}
 {% endmacro %}