Reimplement [PutForwards] per spec

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

Index: third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
diff --git a/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl b/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
index a1be921da37bd29b7b66ee99cf8cd345b8798c89..cddda356ad6650cc3590e1dfb2fd9e9884b39a41 100644
--- a/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
+++ b/third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl
@@ -293,14 +293,11 @@ v8::Local<v8::Value> v8Value, const v8::FunctionCallbackInfo<v8::Value>& info
     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 %}
@@ -315,9 +312,8 @@ v8::Local<v8::Value> v8Value, const v8::FunctionCallbackInfo<v8::Value>& info
   {{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 %}
@@ -337,6 +333,17 @@ v8::Local<v8::Value> v8Value, const v8::FunctionCallbackInfo<v8::Value>& info
 #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.
@@ -397,6 +404,7 @@ v8::Local<v8::Value> v8Value, const v8::FunctionCallbackInfo<v8::Value>& info
   // 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 %}