Reimplement [PutForwards] per spec

Reimplement [PutForwards] per spec

An attribute setter for an attribute X with [PutForwards=Y] should mostly
just do

  Set(Get(this, X), Y, value)

With the previous implementation, we instead essentially inlined both the
getting of X and setting of Y into the setter. This is unnecessary (both
will be implemented correctly separately) and also incorrect, since both
the getter for X and setter for Y could be overridden by a script.

BUG=683310
Review-Url: https://codereview.chromium.org/2733763003
Cr-Commit-Position: refs/heads/master@{#470864}
Committed: https://chromium.googlesource.com/chromium/src/+/0fda810e000bd150860e33350b4a589d4acda1f9

[Jens Widell, 2017-03-06 14:05:27]

jl@opera.com changed reviewers:
+ haraken@chromium.org

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

PTAL

This patch is a bit preliminary; see my comments.

https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:296: {% set
check_security_for_receiver = attribute.is_check_security_for_receiver and not
attribute.is_data_type_property %}
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.

https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:345:
target.As<v8::Object>()->Set(info.GetIsolate()->GetEnteredContext(),
v8String(info.GetIsolate(), "{{attribute.target_attribute_name}}"),
v8Value).IsNothing();
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:27]

haraken@chromium.org changed reviewers:
+ jochen@chromium.org, yukishiino@chromium.org

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

https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:296: {% set
check_security_for_receiver = attribute.is_check_security_for_receiver and not
attribute.is_data_type_property %}
On 2017/03/06 14:05:28, Jens Widell wrote:
> 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.

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

https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:345:
target.As<v8::Object>()->Set(info.GetIsolate()->GetEnteredContext(),
v8String(info.GetIsolate(), "{{attribute.target_attribute_name}}"),
v8Value).IsNothing();
On 2017/03/06 14:05:28, Jens Widell wrote:
> 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.

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.

[Jens Widell, 2017-03-07 09:36:41]

On 2017/03/06 18:57:28, haraken wrote:

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

Such was my intent, yes. But the security check was never emitted (for a
[PutForwards] attribute) in the first place, so how my changed code works a bit
unclear. I will try adding a test case that has a security check, just to make
sure.


>
https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:345:
> target.As<v8::Object>()->Set(info.GetIsolate()->GetEnteredContext(),
> v8String(info.GetIsolate(), "{{attribute.target_attribute_name}}"),
> v8Value).IsNothing();
> On 2017/03/06 14:05:28, Jens Widell wrote:
> > 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.
> 
> 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.)

I'm not sure; this is not really a cross-origin use-case. Location is accessible
cross-origin, but this issue occurs equally for same-origin accesses. In fact,
I'm pretty sure this issue cannot occur in a cross-origin use-case, in the sense
that the correct URL to resolve with and the wrong URL to resolve with cannot
end up being different origins. Only the location object assigned to can be
cross-origin, but its current value would never be considered as the URL to
resolve with.

> I'd prefer using GetCurrentContext() and accepting the wrong behavior until
> Chromium implements window.location correctly though.

I don't think that's a viable approach. I think it would cause unacceptable
regressions. We'd be trading one misbehaving corner-case (the [PutForwards]
issue) for a misbehaving main use-case, that I'd assume at least some set of web
sites actually depend on.

Better then to drop this patch, if we can't come up with an agreeable solution
for the context issue.

And just to be clear: I'm definitely leaning towards that using
`GetEnteredContext()` like I'm doing here is simply the wrong approach. IOW, I'm
not advocating it being the correct way of doing things. :-)

> 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.

I think frame 1 would be the correct. It's always the original context we're
after here. IOW, if we call an event listener for window.onload, we'd call the
event listener function with that frame's context (or maybe the context that
registered the listener?), and the URL conntected to that original context
should be used for resolving relative URLs, no matter how it calls functions
defined in other contexts. Unless "calls" via setTimeout(), dispatchEvent() or
similar, I guess.

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

https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:345:
target.As<v8::Object>()->Set(info.GetIsolate()->GetEnteredContext(),
v8String(info.GetIsolate(), "{{attribute.target_attribute_name}}"),
v8Value).IsNothing();
On 2017/03/06 at 18:57:28, haraken wrote:
> On 2017/03/06 14:05:28, Jens Widell wrote:
> > 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.
> 
> 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.

GetEnteredContext is also wrong during microtask execution

[Jens Widell, 2017-03-07 11:49:07]

On 2017/03/07 11:45:06, jochen wrote:
>
https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):
> 
>
https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:345:
> target.As<v8::Object>()->Set(info.GetIsolate()->GetEnteredContext(),
> v8String(info.GetIsolate(), "{{attribute.target_attribute_name}}"),
> v8Value).IsNothing();
> On 2017/03/06 at 18:57:28, haraken wrote:
> > On 2017/03/06 14:05:28, Jens Widell wrote:
> > > 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.
> > 
> > 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.
> 
> GetEnteredContext is also wrong during microtask execution

Care to elaborate a bit on how microtask execution would affect this? I
unfortunately know next to nothing about microtask execution, so just trying to
expand my understanding.

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

https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2733763003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:345:
target.As<v8::Object>()->Set(info.GetIsolate()->GetEnteredContext(),
v8String(info.GetIsolate(), "{{attribute.target_attribute_name}}"),
v8Value).IsNothing();
On 2017/03/07 11:45:06, jochen wrote:
> On 2017/03/06 at 18:57:28, haraken wrote:
> > On 2017/03/06 14:05:28, Jens Widell wrote:
> > > 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.
> > 
> > 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.
> 
> GetEnteredContext is also wrong during microtask execution

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?

[jochen (gone - plz use gerrit), 2017-03-07 12:21:52]

On 2017/03/07 at 11:49:07, jl wrote:
> > GetEnteredContext is also wrong during microtask execution
> 
> Care to elaborate a bit on how microtask execution would affect this? I
unfortunately know next to nothing about microtask execution, so just trying to
expand my understanding.


Assume that an iframe triggers dispatching of an event that the main frame
listens for. At this point, the iframe's context is entered. The event handler
of the main frame could create a microtask (resolve a promise). At this point,
the main frame is entered, but when it exists, we do a microtask checkpoint at
the end of the event dispatch.

Now microtasks don't enter their context which means that the iframe context is
the entered context again for the microtasks.

[jochen (gone - plz use gerrit), 2017-03-07 12:22:06]

trying to fix this here: https://codereview.chromium.org/2738533003

[Jens Widell, 2017-03-07 12:27:43]

On 2017/03/07 at 12:15:22, haraken wrote:

> 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?

So, what happens, AFAICT, is that e.g. v8::Object::Set() enters the context
passed to it (via some macros and ultimately CallDepthScope's constructor.) So
during the call to Set(), isolate->GetEnteredContext() will be whatever context
we passed to Set(). And Set() in this case calls
V8Location::hrefAttributeSetterCallback(), where enteredDOMWindow() is used to
get the window whose URL is used to resolve the assigned value.

[haraken, 2017-03-07 13:03:35]

On 2017/03/07 12:27:43, Jens Widell wrote:
> On 2017/03/07 at 12:15:22, haraken wrote:
> 
> > 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?
> 
> So, what happens, AFAICT, is that e.g. v8::Object::Set() enters the context
> passed to it (via some macros and ultimately CallDepthScope's constructor.) So
> during the call to Set(), isolate->GetEnteredContext() will be whatever
context
> we passed to Set(). And Set() in this case calls
> V8Location::hrefAttributeSetterCallback(), where enteredDOMWindow() is used to
> get the window whose URL is used to resolve the assigned value.

Maybe a right solution would be to explicitly enter a correct context before
forwarding the [PutForwards] attribute? In this case, the correct context will
be the window object of window.location.

Then you can just use the CurrentContext when calling Get/Set.

[Yuki, 2017-03-07 13:17:07]

On 2017/03/07 13:03:35, haraken wrote:
> On 2017/03/07 12:27:43, Jens Widell wrote:
> > On 2017/03/07 at 12:15:22, haraken wrote:
> > 
> > > 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?
> > 
> > So, what happens, AFAICT, is that e.g. v8::Object::Set() enters the context
> > passed to it (via some macros and ultimately CallDepthScope's constructor.)
So
> > during the call to Set(), isolate->GetEnteredContext() will be whatever
> context
> > we passed to Set(). And Set() in this case calls
> > V8Location::hrefAttributeSetterCallback(), where enteredDOMWindow() is used
to
> > get the window whose URL is used to resolve the assigned value.
> 
> Maybe a right solution would be to explicitly enter a correct context before
> forwarding the [PutForwards] attribute? In this case, the correct context will
> be the window object of window.location.
> 
> Then you can just use the CurrentContext when calling Get/Set.

I've not yet dived deeply, but it sounds strange to me that we explicitly enter
another context just to keep the Entry context.  Probably, we need to change V8
or to manage the Entry context in Blink.

Plus, reading Jens' explanation, even if we entered to another context, Set()
will change / override the Entry context, so it's meaningless, IIUC.

[Jens Widell, 2017-03-07 13:23:59]

On 2017/03/07 at 13:17:07, yukishiino wrote:
> On 2017/03/07 13:03:35, haraken wrote:
> > On 2017/03/07 12:27:43, Jens Widell wrote:
> > > On 2017/03/07 at 12:15:22, haraken wrote:
> > > 
> > > > 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?
> > > 
> > > So, what happens, AFAICT, is that e.g. v8::Object::Set() enters the
context
> > > passed to it (via some macros and ultimately CallDepthScope's
constructor.) So
> > > during the call to Set(), isolate->GetEnteredContext() will be whatever
> > context
> > > we passed to Set(). And Set() in this case calls
> > > V8Location::hrefAttributeSetterCallback(), where enteredDOMWindow() is
used to
> > > get the window whose URL is used to resolve the assigned value.
> > 
> > Maybe a right solution would be to explicitly enter a correct context before
> > forwarding the [PutForwards] attribute? In this case, the correct context
will
> > be the window object of window.location.
> > 
> > Then you can just use the CurrentContext when calling Get/Set.
> 
> I've not yet dived deeply, but it sounds strange to me that we explicitly
enter another context just to keep the Entry context.  Probably, we need to
change V8 or to manage the Entry context in Blink.

My feeling about this is that there's a mismatch between what the entered
context is (and how its maintained) in/by V8, and how Blink uses it. And that
maintaining the Entry context separately in Blink might be a way forward.

> Plus, reading Jens' explanation, even if we entered to another context, Set()
will change / override the Entry context, so it's meaningless, IIUC.

Yes, there's nothing that the forwarding setter can do (other than to pass in
the wanted context to Set(), as this patch currently does) to make sure the
correct context is "entered" when the target setter is called.

[Yuki, 2017-03-07 13:28:14]

On 2017/03/07 13:23:59, Jens Widell wrote:
> On 2017/03/07 at 13:17:07, yukishiino wrote:
> > On 2017/03/07 13:03:35, haraken wrote:
> > > On 2017/03/07 12:27:43, Jens Widell wrote:
> > > > On 2017/03/07 at 12:15:22, haraken wrote:
> > > > 
> > > > > 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?
> > > > 
> > > > So, what happens, AFAICT, is that e.g. v8::Object::Set() enters the
> context
> > > > passed to it (via some macros and ultimately CallDepthScope's
> constructor.) So
> > > > during the call to Set(), isolate->GetEnteredContext() will be whatever
> > > context
> > > > we passed to Set(). And Set() in this case calls
> > > > V8Location::hrefAttributeSetterCallback(), where enteredDOMWindow() is
> used to
> > > > get the window whose URL is used to resolve the assigned value.
> > > 
> > > Maybe a right solution would be to explicitly enter a correct context
before
> > > forwarding the [PutForwards] attribute? In this case, the correct context
> will
> > > be the window object of window.location.
> > > 
> > > Then you can just use the CurrentContext when calling Get/Set.
> > 
> > I've not yet dived deeply, but it sounds strange to me that we explicitly
> enter another context just to keep the Entry context.  Probably, we need to
> change V8 or to manage the Entry context in Blink.
> 
> My feeling about this is that there's a mismatch between what the entered
> context is (and how its maintained) in/by V8, and how Blink uses it. And that
> maintaining the Entry context separately in Blink might be a way forward.

Totally agreed.  > mismatch

FWIW, while I've been investigating how to implement the Incumbent realm in
Blink (not yet completed), it turned out that we need a stack to manage the
Incumbent realm in addition to v8::Isolate::GetCallingContext().

Probably, we need a similar stack for the Entry context.  If the stack is empty,
we should use GetEnteredContext().  Otherwise, we should use the context on the
top of the stack.

Or, should we ask V8 to not change the Entry context automatically?


> > Plus, reading Jens' explanation, even if we entered to another context,
Set()
> will change / override the Entry context, so it's meaningless, IIUC.
> 
> Yes, there's nothing that the forwarding setter can do (other than to pass in
> the wanted context to Set(), as this patch currently does) to make sure the
> correct context is "entered" when the target setter is called.

[haraken, 2017-03-07 13:49:37]

On 2017/03/07 13:28:14, Yuki(slow_til_mar08) wrote:
> On 2017/03/07 13:23:59, Jens Widell wrote:
> > On 2017/03/07 at 13:17:07, yukishiino wrote:
> > > On 2017/03/07 13:03:35, haraken wrote:
> > > > On 2017/03/07 12:27:43, Jens Widell wrote:
> > > > > On 2017/03/07 at 12:15:22, haraken wrote:
> > > > > 
> > > > > > 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?
> > > > > 
> > > > > So, what happens, AFAICT, is that e.g. v8::Object::Set() enters the
> > context
> > > > > passed to it (via some macros and ultimately CallDepthScope's
> > constructor.) So
> > > > > during the call to Set(), isolate->GetEnteredContext() will be
whatever
> > > > context
> > > > > we passed to Set(). And Set() in this case calls
> > > > > V8Location::hrefAttributeSetterCallback(), where enteredDOMWindow() is
> > used to
> > > > > get the window whose URL is used to resolve the assigned value.
> > > > 
> > > > Maybe a right solution would be to explicitly enter a correct context
> before
> > > > forwarding the [PutForwards] attribute? In this case, the correct
context
> > will
> > > > be the window object of window.location.
> > > > 
> > > > Then you can just use the CurrentContext when calling Get/Set.
> > > 
> > > I've not yet dived deeply, but it sounds strange to me that we explicitly
> > enter another context just to keep the Entry context.  Probably, we need to
> > change V8 or to manage the Entry context in Blink.
> > 
> > My feeling about this is that there's a mismatch between what the entered
> > context is (and how its maintained) in/by V8, and how Blink uses it. And
that
> > maintaining the Entry context separately in Blink might be a way forward.
> 
> Totally agreed.  > mismatch
> 
> FWIW, while I've been investigating how to implement the Incumbent realm in
> Blink (not yet completed), it turned out that we need a stack to manage the
> Incumbent realm in addition to v8::Isolate::GetCallingContext().
> 
> Probably, we need a similar stack for the Entry context.  If the stack is
empty,
> we should use GetEnteredContext().  Otherwise, we should use the context on
the
> top of the stack.
> 
> Or, should we ask V8 to not change the Entry context automatically?

I think we should just unconditionally pass CurrentContext to Get/Set. Then V8
won't change the entry context. That's why I proposed explicitly changing an
entry context before forwarding the put-forwards attribute so that we can pass
CurrentContext to Get/Set.

I guess that an incumbent context is not relevant here. According to the spec,
the href setter is expected to use an entry context:
https://html.spec.whatwg.org/multipage/browsers.html#dom-location-href

[Jens Widell, 2017-03-07 14:23:17]

On 2017/03/07 at 13:49:37, haraken wrote:

> I think we should just unconditionally pass CurrentContext to Get/Set. Then V8
won't change the entry context.

This is not what I'm seeing. When I pass in the current context to Set(), that
context will then be the entered context in the location.href setter.

> That's why I proposed explicitly changing an entry context before forwarding
the put-forwards attribute so that we can pass CurrentContext to Get/Set.

If I explicitly enter the entered context (using a v8::Context::Scope) before
calling Set() it works to isolate->GetCurrentContext() for the call to Set(),
but that's because it then returns the context I just entered.

If I store the current context in a local before entering the entered context,
and then use the stored current context in the call, I'm back to failing.

So explicitly entering a context in the forwarding setter doesn't seem to make
much difference. (Unless you meant something other than calling
v8::Context::Enter().)

[haraken, 2017-03-07 14:33:40]

On 2017/03/07 14:23:17, Jens Widell wrote:
> On 2017/03/07 at 13:49:37, haraken wrote:
> 
> > I think we should just unconditionally pass CurrentContext to Get/Set. Then
V8
> won't change the entry context.
> 
> This is not what I'm seeing. When I pass in the current context to Set(), that
> context will then be the entered context in the location.href setter.
> 
> > That's why I proposed explicitly changing an entry context before forwarding
> the put-forwards attribute so that we can pass CurrentContext to Get/Set.
> 
> If I explicitly enter the entered context (using a v8::Context::Scope) before
> calling Set() it works to isolate->GetCurrentContext() for the call to Set(),
> but that's because it then returns the context I just entered.

I meant this. What happens if we enter the creation context of |window| (which
will be equal to GetEnteredContext() in this particular case) before calling
Set() and call Set() with GetCurrentContext() (which should return the context
we have just entered)?

> 
> If I store the current context in a local before entering the entered context,
> and then use the stored current context in the call, I'm back to failing.
> 
> So explicitly entering a context in the forwarding setter doesn't seem to make
> much difference. (Unless you meant something other than calling
> v8::Context::Enter().)

[Jens Widell, 2017-03-07 14:40:21]

On 2017/03/07 at 14:33:40, haraken wrote:
> On 2017/03/07 14:23:17, Jens Widell wrote:
> > On 2017/03/07 at 13:49:37, haraken wrote:
> > 
> > > I think we should just unconditionally pass CurrentContext to Get/Set.
Then V8
> > won't change the entry context.
> > 
> > This is not what I'm seeing. When I pass in the current context to Set(),
that
> > context will then be the entered context in the location.href setter.
> > 
> > > That's why I proposed explicitly changing an entry context before
forwarding
> > the put-forwards attribute so that we can pass CurrentContext to Get/Set.
> > 
> > If I explicitly enter the entered context (using a v8::Context::Scope)
before
> > calling Set() it works to isolate->GetCurrentContext() for the call to
Set(),
> > but that's because it then returns the context I just entered.
> 
> I meant this. What happens if we enter the creation context of |window| (which
will be equal to GetEnteredContext() in this particular case) before calling
Set() and call Set() with GetCurrentContext() (which should return the context
we have just entered)?

This works. It's also quite similar in practice to just calling Set() with
GetEnteredContext(), since I'd be calling Set() with the same argument value in
both cases. I'd just have some seemingly pointless extra code in the forwarding
setter entering the already entered context.

[haraken, 2017-03-07 14:48:24]

On 2017/03/07 14:40:21, Jens Widell wrote:
> On 2017/03/07 at 14:33:40, haraken wrote:
> > On 2017/03/07 14:23:17, Jens Widell wrote:
> > > On 2017/03/07 at 13:49:37, haraken wrote:
> > > 
> > > > I think we should just unconditionally pass CurrentContext to Get/Set.
> Then V8
> > > won't change the entry context.
> > > 
> > > This is not what I'm seeing. When I pass in the current context to Set(),
> that
> > > context will then be the entered context in the location.href setter.
> > > 
> > > > That's why I proposed explicitly changing an entry context before
> forwarding
> > > the put-forwards attribute so that we can pass CurrentContext to Get/Set.
> > > 
> > > If I explicitly enter the entered context (using a v8::Context::Scope)
> before
> > > calling Set() it works to isolate->GetCurrentContext() for the call to
> Set(),
> > > but that's because it then returns the context I just entered.
> > 
> > I meant this. What happens if we enter the creation context of |window|
(which
> will be equal to GetEnteredContext() in this particular case) before calling
> Set() and call Set() with GetCurrentContext() (which should return the context
> we have just entered)?
> 
> This works. It's also quite similar in practice to just calling Set() with
> GetEnteredContext(), since I'd be calling Set() with the same argument value
in
> both cases. I'd just have some seemingly pointless extra code in the
forwarding
> setter entering the already entered context.

I think we need to carefully check with the spec
(https://heycam.github.io/webidl/#PutForwards), in particular, figure out what
the "forwarding" accurately means, but I guess the spec requires to switch the
current context when "forwarding" the attribute.

[Jens Widell, 2017-03-07 14:56:44]

On 2017/03/07 at 14:48:24, haraken wrote:
> On 2017/03/07 14:40:21, Jens Widell wrote:
> > On 2017/03/07 at 14:33:40, haraken wrote:
> > > On 2017/03/07 14:23:17, Jens Widell wrote:
> > > > On 2017/03/07 at 13:49:37, haraken wrote:
> > > > 
> > > > > I think we should just unconditionally pass CurrentContext to Get/Set.
> > Then V8
> > > > won't change the entry context.
> > > > 
> > > > This is not what I'm seeing. When I pass in the current context to
Set(),
> > that
> > > > context will then be the entered context in the location.href setter.
> > > > 
> > > > > That's why I proposed explicitly changing an entry context before
> > forwarding
> > > > the put-forwards attribute so that we can pass CurrentContext to
Get/Set.
> > > > 
> > > > If I explicitly enter the entered context (using a v8::Context::Scope)
> > before
> > > > calling Set() it works to isolate->GetCurrentContext() for the call to
> > Set(),
> > > > but that's because it then returns the context I just entered.
> > > 
> > > I meant this. What happens if we enter the creation context of |window|
(which
> > will be equal to GetEnteredContext() in this particular case) before calling
> > Set() and call Set() with GetCurrentContext() (which should return the
context
> > we have just entered)?
> > 
> > This works. It's also quite similar in practice to just calling Set() with
> > GetEnteredContext(), since I'd be calling Set() with the same argument value
in
> > both cases. I'd just have some seemingly pointless extra code in the
forwarding
> > setter entering the already entered context.
> 
> I think we need to carefully check with the spec
(https://heycam.github.io/webidl/#PutForwards), in particular, figure out what
the "forwarding" accurately means, but I guess the spec requires to switch the
current context when "forwarding" the attribute.

The way I read the Web IDL specification, [PutForwards] should essentially be
implemented as syntactic sugar. IOW, the expressions `window.location = X` and
`window.location.href = X` would result in the same set of Get() and Set()
operations on the same objects.

[haraken, 2017-03-07 15:01:01]

On 2017/03/07 14:56:44, Jens Widell wrote:
> On 2017/03/07 at 14:48:24, haraken wrote:
> > On 2017/03/07 14:40:21, Jens Widell wrote:
> > > On 2017/03/07 at 14:33:40, haraken wrote:
> > > > On 2017/03/07 14:23:17, Jens Widell wrote:
> > > > > On 2017/03/07 at 13:49:37, haraken wrote:
> > > > > 
> > > > > > I think we should just unconditionally pass CurrentContext to
Get/Set.
> > > Then V8
> > > > > won't change the entry context.
> > > > > 
> > > > > This is not what I'm seeing. When I pass in the current context to
> Set(),
> > > that
> > > > > context will then be the entered context in the location.href setter.
> > > > > 
> > > > > > That's why I proposed explicitly changing an entry context before
> > > forwarding
> > > > > the put-forwards attribute so that we can pass CurrentContext to
> Get/Set.
> > > > > 
> > > > > If I explicitly enter the entered context (using a v8::Context::Scope)
> > > before
> > > > > calling Set() it works to isolate->GetCurrentContext() for the call to
> > > Set(),
> > > > > but that's because it then returns the context I just entered.
> > > > 
> > > > I meant this. What happens if we enter the creation context of |window|
> (which
> > > will be equal to GetEnteredContext() in this particular case) before
calling
> > > Set() and call Set() with GetCurrentContext() (which should return the
> context
> > > we have just entered)?
> > > 
> > > This works. It's also quite similar in practice to just calling Set() with
> > > GetEnteredContext(), since I'd be calling Set() with the same argument
value
> in
> > > both cases. I'd just have some seemingly pointless extra code in the
> forwarding
> > > setter entering the already entered context.
> > 
> > I think we need to carefully check with the spec
> (https://heycam.github.io/webidl/#PutForwards), in particular, figure out what
> the "forwarding" accurately means, but I guess the spec requires to switch the
> current context when "forwarding" the attribute.
> 
> The way I read the Web IDL specification, [PutForwards] should essentially be
> implemented as syntactic sugar. IOW, the expressions `window.location = X` and
> `window.location.href = X` would result in the same set of Get() and Set()
> operations on the same objects.

When V8 calls window.location, V8 sets the current context to window's relevant
realm (IIUC).

When V8 calls window.location.href, V8 sets the current context to
window.location's relevant realm (IIUC).

This means that if we want to forward window.location to window.location.href,
we need to switch the current context accordingly, right?

[Jens Widell, 2017-03-07 15:16:56]

On 2017/03/07 at 15:01:01, haraken wrote:

> When V8 calls window.location, V8 sets the current context to window's
relevant realm (IIUC).
> When V8 calls window.location.href, V8 sets the current context to
window.location's relevant realm (IIUC).

No, I'm pretty sure the current context in all cases is the context in which the
code that contains the assignment was compiled. So if I have an inline script in
document A that defines a function f() that assigns to some other window's
.location, and then I call f() from an inline script in document B, then the
setter is called with A's context as current context. B's context would be the
entered context. The manipulated window's context is not involved at all.

> This means that if we want to forward window.location to window.location.href,
we need to switch the current context accordingly, right?

I'd say we need to *not* switch the entered context. The current context is not
used to determine how to resolve URLs. I'd rather just pass along whatever the
current context is from the forwarding setter to the target setter, but the
problem is that when I do that, the entered context is switched to that context.

Or, to put it this way. In my example above, B's URL should be used for URL
resolution.

If f() assigns directly to window.location.href, its setter is called with
GetCurrentContext()==A and GetEnteredContext()==B. We used the entered context
for resolution, current context for some other stuff, and all is well.

If f() assigns to window.location instead, the forwarding setter is called with
the same context as above, but when it then (indirectly) calls the
Location.href, unavoidably GetCurrentContext()==GetEnteredContext(). So we seem
to be left with the choice of which context we want to pass on. We can't achieve
exactly the same behavior as when window.location.href is assigned directly.
(Not when only modifying the forwarding setter, at least.)

[Yuki, 2017-03-08 09:56:20]

On 2017/03/07 15:16:56, Jens Widell wrote:
> On 2017/03/07 at 15:01:01, haraken wrote:
> 
> > When V8 calls window.location, V8 sets the current context to window's
> relevant realm (IIUC).
> > When V8 calls window.location.href, V8 sets the current context to
> window.location's relevant realm (IIUC).
> 
> No, I'm pretty sure the current context in all cases is the context in which
the
> code that contains the assignment was compiled. So if I have an inline script
in
> document A that defines a function f() that assigns to some other window's
> .location, and then I call f() from an inline script in document B, then the
> setter is called with A's context as current context. B's context would be the
> entered context. The manipulated window's context is not involved at all.
> 
> > This means that if we want to forward window.location to
window.location.href,
> we need to switch the current context accordingly, right?
> 
> I'd say we need to *not* switch the entered context. The current context is
not
> used to determine how to resolve URLs. I'd rather just pass along whatever the
> current context is from the forwarding setter to the target setter, but the
> problem is that when I do that, the entered context is switched to that
context.
> 
> Or, to put it this way. In my example above, B's URL should be used for URL
> resolution.
> 
> If f() assigns directly to window.location.href, its setter is called with
> GetCurrentContext()==A and GetEnteredContext()==B. We used the entered context
> for resolution, current context for some other stuff, and all is well.
> 
> If f() assigns to window.location instead, the forwarding setter is called
with
> the same context as above, but when it then (indirectly) calls the
> Location.href, unavoidably GetCurrentContext()==GetEnteredContext(). So we
seem
> to be left with the choice of which context we want to pass on. We can't
achieve
> exactly the same behavior as when window.location.href is assigned directly.
> (Not when only modifying the forwarding setter, at least.)

Jens is right.  It seems that all V8 APIs that take a v8::Context argument
and return a Maybe update the entered context.  PREPARE_FOR_EXECUTION_GENERIC
(hence PREPARE_FOR_EXECUTION_WITH_CONTEXT) updates the entered context.

I think we have at least three options.

1) Use non-Maybe/non-Context version of APIs (they're V8_DEPRECATE_SOON,
though).
In this case, Set(Local<Value> key, Local<Value> value) seems not changing
the entered context.

2) Implement the "Entry realm" on our side in Blink.

3) Ask V8 not to change the entered context.
jochen@, could this be an option?

Option 2) should be the most straightforward, but it will be some amount of
work.
We could use Option 1) as a short-term hack.

[Yuki, 2017-03-13 08:51:29]

Can we move forward with
non-Maybe/non-Context version of
Set(Local<Value> key, Local<Value> value)
with a TODO(yukishiino) for the time being?

I'll implement the Entry realm separately.

What do you guys think?

[Jens Widell, 2017-03-13 10:41:29]

On 2017/03/13 08:51:29, Yuki wrote:
> Can we move forward with
> non-Maybe/non-Context version of
> Set(Local<Value> key, Local<Value> value)
> with a TODO(yukishiino) for the time being?
> 
> I'll implement the Entry realm separately.
> 
> What do you guys think?

That's not a valid workaround, I think. The non-context versions of Get/Set just
call the context versions with some context. The context they use is the current
context of an isolate extracted from the this-object. So in this case that will
be the same as just using isolate->GetCurrentContext().

I think we might as well postpone landing this fix (changed to use
GetCurrentContext()) until we've implemented Entry realm and thus using
GetCurrentContext() works.

At the end of the day, fixing this [PutForwards] corner case doesn't appear very
important. I just picked this issue as an almost trivial first issue to work on
after being absent for a while. Didn't that work out great? :-)

[Yuki, 2017-03-13 12:27:10]

On 2017/03/13 10:41:29, Jens Widell wrote:
> On 2017/03/13 08:51:29, Yuki wrote:
> > Can we move forward with
> > non-Maybe/non-Context version of
> > Set(Local<Value> key, Local<Value> value)
> > with a TODO(yukishiino) for the time being?
> > 
> > I'll implement the Entry realm separately.
> > 
> > What do you guys think?
> 
> That's not a valid workaround, I think. The non-context versions of Get/Set
just
> call the context versions with some context. The context they use is the
current
> context of an isolate extracted from the this-object. So in this case that
will
> be the same as just using isolate->GetCurrentContext().
> 
> I think we might as well postpone landing this fix (changed to use
> GetCurrentContext()) until we've implemented Entry realm and thus using
> GetCurrentContext() works.
> 
> At the end of the day, fixing this [PutForwards] corner case doesn't appear
very
> important. I just picked this issue as an almost trivial first issue to work
on
> after being absent for a while. Didn't that work out great? :-)

You're right.  I feel sorry to make this CL suspended.

[haraken, 2017-03-13 12:36:20]

On 2017/03/13 12:27:10, Yuki wrote:
> On 2017/03/13 10:41:29, Jens Widell wrote:
> > On 2017/03/13 08:51:29, Yuki wrote:
> > > Can we move forward with
> > > non-Maybe/non-Context version of
> > > Set(Local<Value> key, Local<Value> value)
> > > with a TODO(yukishiino) for the time being?
> > > 
> > > I'll implement the Entry realm separately.
> > > 
> > > What do you guys think?
> > 
> > That's not a valid workaround, I think. The non-context versions of Get/Set
> just
> > call the context versions with some context. The context they use is the
> current
> > context of an isolate extracted from the this-object. So in this case that
> will
> > be the same as just using isolate->GetCurrentContext().
> > 
> > I think we might as well postpone landing this fix (changed to use
> > GetCurrentContext()) until we've implemented Entry realm and thus using
> > GetCurrentContext() works.
> > 
> > At the end of the day, fixing this [PutForwards] corner case doesn't appear
> very
> > important. I just picked this issue as an almost trivial first issue to work
> on
> > after being absent for a while. Didn't that work out great? :-)
> 
> You're right.  I feel sorry to make this CL suspended.

(I welcome any contribution from you though -- feel free to dive in to the entry
realm thing if you have a bandwidth & interest :-)

[Jens Widell, 2017-05-05 13:26:50]

Patch rebased and updated to use GetCurrentContext() instead of
GetEnteredContext(), which now works, after
https://codereview.chromium.org/2862483003/.

[Yuki, 2017-05-08 09:31:19]

https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:302: {% set
check_security_for_receiver = attribute.is_check_security_for_receiver and not
attribute.is_data_type_property %}
nit: If this is not a big deal, I'd prefer not aliasing, especially when it's
used only once.

|attribute.is_check_security_for_receiver|
should be good enough to understand, and |is_data_type_property| should be gone
(in future).

https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:306:
v8::Local<v8::Object> holder = info.Holder();
Personally, I'd prefer the previous code rather than the new one.

* holder = info.Holder()
This is just an alias.  Why we didn't do this aliasing for the case of |not
attribute.is_static| is just because static attributes never use |holder|
(because it's static).
Probably |attribute.is_replaceable| wouldn't use |holder|, too.
We could do this aliasing unconditionally with using ALLOW_UNUSED.
Anyway, my preference is that I don't want to make this condition more complex.

https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:345: if
(!info.Holder()->Get(info.GetIsolate()->GetCurrentContext(),
V8String(info.GetIsolate(), "{{attribute.name}}")).ToLocal(&target))
nit: You could use |holder| instead of |info.GetHolder()|.

https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:351:
target.As<v8::Object>()->Set(info.GetIsolate()->GetCurrentContext(),
V8String(info.GetIsolate(), "{{attribute.target_attribute_name}}"),
v8Value).IsNothing();
IsNothing() doesn't make sense here.

IIUC, you should do:

  bool result;
  if (!target.Set(...).To(&result))
    return;
  if (!result) {
    // In this case, no exception should be thrown, but
    // somehow we failed to set the value.  Then, shall
    // we throw something?
    exceptionState->ThrowSomething(...);
    return;
  }

Or, if we believe that it never fails, DCHECK is okay.

https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:352: {% else %}
nit: {% else %}{# attribute.is_put_forwards %}

https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:418: {% endif
%}{# is_put_forwards #}
nit: {# attribute.is_put_forwards #}

[Jens Widell, 2017-05-10 13:03:26]

On 2017/05/08 at 09:31:19, yukishiino wrote:
>
https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):
> 
>
https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:302: {% set
check_security_for_receiver = attribute.is_check_security_for_receiver and not
attribute.is_data_type_property %}
> nit: If this is not a big deal, I'd prefer not aliasing, especially when it's
used only once.

Dropped.

> |attribute.is_check_security_for_receiver|
> should be good enough to understand, and |is_data_type_property| should be
gone (in future).
> 
>
https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:306:
v8::Local<v8::Object> holder = info.Holder();
> Personally, I'd prefer the previous code rather than the new one.
> 
> * holder = info.Holder()
> This is just an alias.  Why we didn't do this aliasing for the case of |not
attribute.is_static| is just because static attributes never use |holder|
(because it's static).
> Probably |attribute.is_replaceable| wouldn't use |holder|, too.
> We could do this aliasing unconditionally with using ALLOW_UNUSED.
> Anyway, my preference is that I don't want to make this condition more
complex.

Changed by adding an unconditional 'holder' local, with ALLOW_UNUSED_LOCAL().
The vast majority of setters already had the local.

>
https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:345: if
(!info.Holder()->Get(info.GetIsolate()->GetCurrentContext(),
V8String(info.GetIsolate(), "{{attribute.name}}")).ToLocal(&target))
> nit: You could use |holder| instead of |info.GetHolder()|.

Done.

>
https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:351:
target.As<v8::Object>()->Set(info.GetIsolate()->GetCurrentContext(),
V8String(info.GetIsolate(), "{{attribute.target_attribute_name}}"),
v8Value).IsNothing();
> IsNothing() doesn't make sense here.
> 
> IIUC, you should do:
> 
>   bool result;
>   if (!target.Set(...).To(&result))
>     return;
>   if (!result) {
>     // In this case, no exception should be thrown, but
>     // somehow we failed to set the value.  Then, shall
>     // we throw something?
>     exceptionState->ThrowSomething(...);
>     return;
>   }
> 
> Or, if we believe that it never fails, DCHECK is okay.

It can certainly fail, but we never need to handle the error in any way other
than returning. And that is optional too, since we're at the end of the
function.

But I've changed now to actually look at the result and (seemingly) return
early. Since it can't change the code flow, it's possible the compiler can
optimize it all away.

>
https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:352: {% else
%}
> nit: {% else %}{# attribute.is_put_forwards %}
> 
>
https://codereview.chromium.org/2733763003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:418: {% endif
%}{# is_put_forwards #}
> nit: {# attribute.is_put_forwards #}

Both done.

[Yuki, 2017-05-10 13:07:51]

Looks like you've not yet uploaded your latest patch?

[Jens Widell, 2017-05-10 13:09:10]

On 2017/05/10 at 13:07:51, yukishiino wrote:
> Looks like you've not yet uploaded your latest patch?

Turns out 'git cl upload' expects you to press enter after inputting a patch set
description. ;-)

[Yuki, 2017-05-10 13:28:15]

LGTM ;)

[Jens Widell, 2017-05-10 14:18:25]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2017-05-10 14:19:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-05-10 14:40:03]

https://codereview.chromium.org/2733763003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2733763003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:345: if
(!holder->Get(isolate->GetCurrentContext(), V8String(isolate,
"{{attribute.name}}")).ToLocal(&target))

Just to confirm: If the author script overrides the getter, the customized
getter can be called here. Is that okay?

https://codereview.chromium.org/2733763003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:351:
v8::Maybe<bool> result =
target.As<v8::Object>()->Set(isolate->GetCurrentContext(), V8String(isolate,
"{{attribute.target_attribute_name}}"), v8Value);

We prefer avoid using Maybe. You can use ...Set().To().

https://codereview.chromium.org/2733763003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:351:
v8::Maybe<bool> result =
target.As<v8::Object>()->Set(isolate->GetCurrentContext(), V8String(isolate,
"{{attribute.target_attribute_name}}"), v8Value);

Ditto. The customized setter can be called.

[Jens Widell, 2017-05-10 14:40:33]

The CQ bit was unchecked by jl@opera.com

[Jens Widell, 2017-05-10 14:46:15]

On 2017/05/10 at 14:40:03, haraken wrote:
>
https://codereview.chromium.org/2733763003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):
> 
>
https://codereview.chromium.org/2733763003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:345: if
(!holder->Get(isolate->GetCurrentContext(), V8String(isolate,
"{{attribute.name}}")).ToLocal(&target))
> 
> Just to confirm: If the author script overrides the getter, the customized
getter can be called here. Is that okay?

Us not calling the getter here (and setter below) is essentially the bug this is
fixing. So yes, that's okay.

Of note is that in the case of window.location forwarding to location.href, both
attributes are [Unforgeable], so in that case there can be no custom getters or
setters.

>
https://codereview.chromium.org/2733763003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:351:
v8::Maybe<bool> result =
target.As<v8::Object>()->Set(isolate->GetCurrentContext(), V8String(isolate,
"{{attribute.target_attribute_name}}"), v8Value);
> 
> We prefer avoid using Maybe. You can use ...Set().To().

Sure.

>
https://codereview.chromium.org/2733763003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:351:
v8::Maybe<bool> result =
target.As<v8::Object>()->Set(isolate->GetCurrentContext(), V8String(isolate,
"{{attribute.target_attribute_name}}"), v8Value);
> 
> Ditto. The customized setter can be called.

Yes, this is fine.

[haraken, 2017-05-10 14:56:38]

LGTM

[Jens Widell, 2017-05-10 14:56:54]

The CQ bit was checked by jl@opera.com

[Jens Widell, 2017-05-10 14:56:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2733763003/#ps60001
(title: "avoid using v8::Maybe")

[commit-bot: I haz the power, 2017-05-10 14:57:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 16:43:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 16:43:31]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jens Widell, 2017-05-11 06:35:37]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2017-05-11 06:36:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 07:40:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494484537867630,
"parent_rev": "ea1716a00c9d7ba48f98537099a1ccd750bdaa37", "commit_rev":
"0fda810e000bd150860e33350b4a589d4acda1f9"}

[commit-bot: I haz the power, 2017-05-11 07:40:14]

Description was changed from

==========
Reimplement [PutForwards] per spec

An attribute setter for an attribute X with [PutForwards=Y] should mostly
just do

  Set(Get(this, X), Y, value)

With the previous implementation, we instead essentially inlined both the
getting of X and setting of Y into the setter. This is unnecessary (both
will be implemented correctly separately) and also incorrect, since both
the getter for X and setter for Y could be overridden by a script.

BUG=683310
==========

to

==========
Reimplement [PutForwards] per spec

An attribute setter for an attribute X with [PutForwards=Y] should mostly
just do

  Set(Get(this, X), Y, value)

With the previous implementation, we instead essentially inlined both the
getting of X and setting of Y into the setter. This is unnecessary (both
will be implemented correctly separately) and also incorrect, since both
the getter for X and setter for Y could be overridden by a script.

BUG=683310

Review-Url: https://codereview.chromium.org/2733763003
Cr-Commit-Position: refs/heads/master@{#470864}
Committed:
https://chromium.googlesource.com/chromium/src/+/0fda810e000bd150860e33350b4a...
==========

[commit-bot: I haz the power, 2017-05-11 07:40:15]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/0fda810e000bd150860e33350b4a...