bindings: Adds check for attributes on prototype chains in CSSStyleDeclaration.

bindings: Adds check for attributes on prototype chains in CSSStyleDeclaration.

CSSStyleDeclarationCustom is missing an existence check against attributes on
prototype chains.  This CL adds the missing check.

BUG=470671
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193771

[Yuki, 2015-04-07 08:19:11]

yukishiino@chromium.org changed reviewers:
+ bashi@chromium.org, dcarney@chromium.org

[Yuki, 2015-04-07 08:19:12]

Could you guys review this CL?

[haraken, 2015-04-07 08:41:54]

Looks like the tests are failing :(

[bashi, 2015-04-07 08:44:56]

https://codereview.chromium.org/1061243002/diff/1/Source/bindings/core/v8/cus...
File Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp (right):

https://codereview.chromium.org/1061243002/diff/1/Source/bindings/core/v8/cus...
Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp:210: if
(v8Call(info.Holder()->GetRealNamedPropertyInPrototypeChain(context,
nameString), returnValue, tryCatch)) {
I read the following comment on GetRealNamedPropertyInPrototypeChain() as "V8
API could return an empty maybe handle without exception". @dcarney, is this
correct?

"If result.IsEmpty() no real property was located in the prototype chain."

v8Call() assumes that there must be an exception if result.IsEmpty() is true, so
we can't use v8Call() here. Maybe just use ToLocal()?

[Yuki, 2015-04-07 09:29:00]

https://codereview.chromium.org/1061243002/diff/1/Source/bindings/core/v8/cus...
File Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp (right):

https://codereview.chromium.org/1061243002/diff/1/Source/bindings/core/v8/cus...
Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp:210: if
(v8Call(info.Holder()->GetRealNamedPropertyInPrototypeChain(context,
nameString), returnValue, tryCatch)) {
On 2015/04/07 08:44:56, bashi1 wrote:
> I read the following comment on GetRealNamedPropertyInPrototypeChain() as "V8
> API could return an empty maybe handle without exception". @dcarney, is this
> correct?
> 
> "If result.IsEmpty() no real property was located in the prototype chain."
> 
> v8Call() assumes that there must be an exception if result.IsEmpty() is true,
so
> we can't use v8Call() here. Maybe just use ToLocal()?

I think that it is an intended behavior that
GetRealNamedPropertyInPrototypeChain returns an empty handle if no property
found.  The old version has the same spec.  Having said that, I agree that this
is confusing.

Fixed to not use v8Call.

[haraken, 2015-04-07 09:32:18]

On 2015/04/07 09:29:00, Yuki wrote:
>
https://codereview.chromium.org/1061243002/diff/1/Source/bindings/core/v8/cus...
> File Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp (right):
> 
>
https://codereview.chromium.org/1061243002/diff/1/Source/bindings/core/v8/cus...
> Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp:210: if
> (v8Call(info.Holder()->GetRealNamedPropertyInPrototypeChain(context,
> nameString), returnValue, tryCatch)) {
> On 2015/04/07 08:44:56, bashi1 wrote:
> > I read the following comment on GetRealNamedPropertyInPrototypeChain() as
"V8
> > API could return an empty maybe handle without exception". @dcarney, is this
> > correct?
> > 
> > "If result.IsEmpty() no real property was located in the prototype chain."
> > 
> > v8Call() assumes that there must be an exception if result.IsEmpty() is
true,
> so
> > we can't use v8Call() here. Maybe just use ToLocal()?
> 
> I think that it is an intended behavior that
> GetRealNamedPropertyInPrototypeChain returns an empty handle if no property
> found.  The old version has the same spec.  Having said that, I agree that
this
> is confusing.
> 
> Fixed to not use v8Call.

I guess we might want to fix the V8 API. The contract is that a V8 API returns
an empty handle if and only if V8 throws an exception.

Dan?

[dcarney, 2015-04-07 18:58:39]

On 2015/04/07 09:32:18, haraken wrote:
> On 2015/04/07 09:29:00, Yuki wrote:
> >
>
https://codereview.chromium.org/1061243002/diff/1/Source/bindings/core/v8/cus...
> > File Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1061243002/diff/1/Source/bindings/core/v8/cus...
> > Source/bindings/core/v8/custom/V8CSSStyleDeclarationCustom.cpp:210: if
> > (v8Call(info.Holder()->GetRealNamedPropertyInPrototypeChain(context,
> > nameString), returnValue, tryCatch)) {
> > On 2015/04/07 08:44:56, bashi1 wrote:
> > > I read the following comment on GetRealNamedPropertyInPrototypeChain() as
> "V8
> > > API could return an empty maybe handle without exception". @dcarney, is
this
> > > correct?
> > > 
> > > "If result.IsEmpty() no real property was located in the prototype chain."
> > > 
> > > v8Call() assumes that there must be an exception if result.IsEmpty() is
> true,
> > so
> > > we can't use v8Call() here. Maybe just use ToLocal()?
> > 
> > I think that it is an intended behavior that
> > GetRealNamedPropertyInPrototypeChain returns an empty handle if no property
> > found.  The old version has the same spec.  Having said that, I agree that
> this
> > is confusing.
> > 
> > Fixed to not use v8Call.
> 
> I guess we might want to fix the V8 API. The contract is that a V8 API returns
> an empty handle if and only if V8 throws an exception.
> 
> Dan?

I'll change the api.  in the meantime, don't do this, but mark this callback in
the bindings generator as kNonMasking and remove the check to
HasRealNamedCallbackProperty in the custom code

[Yuki, 2015-04-09 10:55:54]

On 2015/04/07 18:58:39, dcarney wrote:
> I'll change the api.  in the meantime, don't do this, but mark this callback
in
> the bindings generator as kNonMasking and remove the check to
> HasRealNamedCallbackProperty in the custom code

Done.  Could you guys take another look?

[dcarney, 2015-04-09 11:03:09]

https://codereview.chromium.org/1061243002/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1061243002/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:396: {% if interface_name !=
'Window' %}
i think there are nsobject callbacks which you will break here.  please use
interface_name == 'CSS...'

[bashi, 2015-04-10 00:15:32]

https://codereview.chromium.org/1061243002/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1061243002/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:396: {% if interface_name !=
'Window' %}
On 2015/04/09 11:03:09, dcarney wrote:
> i think there are nsobject callbacks which you will break here.  please use
> interface_name == 'CSS...'

It would be a bit difficult to understand why we have this branch when we read
this later. Could we do either:
- add a comment
- introduce a variable with descriptive name
?

[dcarney, 2015-04-10 07:21:39]

On 2015/04/10 00:15:32, bashi1 wrote:
>
https://codereview.chromium.org/1061243002/diff/40001/Source/bindings/templat...
> File Source/bindings/templates/interface_base.cpp (right):
> 
>
https://codereview.chromium.org/1061243002/diff/40001/Source/bindings/templat...
> Source/bindings/templates/interface_base.cpp:396: {% if interface_name !=
> 'Window' %}
> On 2015/04/09 11:03:09, dcarney wrote:
> > i think there are nsobject callbacks which you will break here.  please use
> > interface_name == 'CSS...'
> 
> It would be a bit difficult to understand why we have this branch when we read
> this later. Could we do either:
> - add a comment
> - introduce a variable with descriptive name
> ?

whichever you like.  i will want to do the same to Window after you land, but
eveyrthing else won't have it

[Yuki, 2015-04-10 14:06:49]

As not few tests are failing, I'll investigate what I should do more.  However,
I'm inclined to apply kNonMasking to all interfaces.  Let me work on failing
tests next week.

https://codereview.chromium.org/1061243002/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1061243002/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:396: {% if interface_name !=
'Window' %}
On 2015/04/10 00:15:32, bashi1 wrote:
> On 2015/04/09 11:03:09, dcarney wrote:
> > i think there are nsobject callbacks which you will break here.  please use
> > interface_name == 'CSS...'
> 
> It would be a bit difficult to understand why we have this branch when we read
> this later. Could we do either:
> - add a comment
> - introduce a variable with descriptive name
> ?

For NPObject, we don't use any auto-generated binding code, all the
implementations are hand-written.  So I think this is okay.

And as you wrote, since we shouldn't use HasRealNamedProperty or
GetRealNamedPropertyInPrototypeChain, then not only CSSStyleDeclaration but also
other interfaces should use kNonMasking flag, I think.  Having said that,
Window's implementation is a little bit special, so I'd like to exclude Window
in this CL and will address Window's attributes separately.

[Yuki, 2015-04-14 06:37:24]

Could you guys take another look?

It turned out that HTMLObjectElement relies on that it's being
[OverrideBuiltins] (HTMLObjectElement was not calling HasRealNamedProperty,
etc.), so I added [OverrideBuiltins] explicitly.

Except for Window, we use kNonMasking flag for all cases including custom
binding code.

[haraken, 2015-04-14 06:50:00]

On 2015/04/14 06:37:24, Yuki wrote:
> Could you guys take another look?
> 
> It turned out that HTMLObjectElement relies on that it's being
> [OverrideBuiltins] (HTMLObjectElement was not calling HasRealNamedProperty,
> etc.), so I added [OverrideBuiltins] explicitly.
> 
> Except for Window, we use kNonMasking flag for all cases including custom
> binding code.

What is the behavior of Firefox? Does Firefox also handle HTMLObjectElement as
if it doesn't have [OverrideBuiltins]?

[dcarney, 2015-04-14 06:51:30]

On 2015/04/14 06:37:24, Yuki wrote:
> Could you guys take another look?
> 
> It turned out that HTMLObjectElement relies on that it's being
> [OverrideBuiltins] (HTMLObjectElement was not calling HasRealNamedProperty,
> etc.), so I added [OverrideBuiltins] explicitly.
> 
> Except for Window, we use kNonMasking flag for all cases including custom
> binding code.

lgtm.  i'll change window over once this lands. i've got a cl ready

[Yuki, 2015-04-14 07:13:04]

On 2015/04/14 06:50:00, haraken wrote:
> On 2015/04/14 06:37:24, Yuki wrote:
> > Could you guys take another look?
> > 
> > It turned out that HTMLObjectElement relies on that it's being
> > [OverrideBuiltins] (HTMLObjectElement was not calling HasRealNamedProperty,
> > etc.), so I added [OverrideBuiltins] explicitly.
> > 
> > Except for Window, we use kNonMasking flag for all cases including custom
> > binding code.
> 
> What is the behavior of Firefox? Does Firefox also handle HTMLObjectElement as
> if it doesn't have [OverrideBuiltins]?

It seems that Firefox doesn't have [OverrideBuiltins].  However, first of all,
HTMLObjectElement is supposed to NOT have named property.  We have a TODO
comment to get rid of named property.

core/html/HTMLObjectElement.idl:
 // TODO(philipj): These getters and setters are not in the spec.

HTMLAppletElement and HTMLEmbedElement are the same.  All these three interfaces
are supposed to not have named/indexed property, but we actually have, and there
are TODO/FIXME comments.

[haraken, 2015-04-14 08:10:35]

On 2015/04/14 07:13:04, Yuki wrote:
> On 2015/04/14 06:50:00, haraken wrote:
> > On 2015/04/14 06:37:24, Yuki wrote:
> > > Could you guys take another look?
> > > 
> > > It turned out that HTMLObjectElement relies on that it's being
> > > [OverrideBuiltins] (HTMLObjectElement was not calling
HasRealNamedProperty,
> > > etc.), so I added [OverrideBuiltins] explicitly.
> > > 
> > > Except for Window, we use kNonMasking flag for all cases including custom
> > > binding code.
> > 
> > What is the behavior of Firefox? Does Firefox also handle HTMLObjectElement
as
> > if it doesn't have [OverrideBuiltins]?
> 
> It seems that Firefox doesn't have [OverrideBuiltins].  However, first of all,
> HTMLObjectElement is supposed to NOT have named property.  We have a TODO
> comment to get rid of named property.
> 
> core/html/HTMLObjectElement.idl:
>  // TODO(philipj): These getters and setters are not in the spec.
> 
> HTMLAppletElement and HTMLEmbedElement are the same.  All these three
interfaces
> are supposed to not have named/indexed property, but we actually have, and
there
> are TODO/FIXME comments.

Thanks for the confirmation. At the very least, this CL keeps the current
behavior, so is OK. LGTM.

[bashi, 2015-04-15 03:35:34]

LGTM

[Yuki, 2015-04-15 09:35:29]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2015-04-15 09:35:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcarney@chromium.org, bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/1061243002/#ps120001
(title: "Added [OverrideBuiltins] to HTML{Applet,Embed}Element in addition to
HTMLObejctElement.")

[commit-bot: I haz the power, 2015-04-15 09:35:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1061243002/120001

[commit-bot: I haz the power, 2015-04-15 09:56:44]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193771

[dcarney, 2015-04-16 11:49:37]

On 2015/04/15 09:56:44, I haz the power (commit-bot) wrote:
> Committed patchset #7 (id:120001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=193771

note: the fix for window is https://codereview.chromium.org/1089833003/
(currently without the test expectation changes)

[Yuki, 2015-04-16 12:00:35]

On 2015/04/16 11:49:37, dcarney wrote:
> On 2015/04/15 09:56:44, I haz the power (commit-bot) wrote:
> > Committed patchset #7 (id:120001) as
> > https://src.chromium.org/viewvc/blink?view=rev&revision=193771
> 
> note: the fix for window is https://codereview.chromium.org/1089833003/
> (currently without the test expectation changes)

Thanks for letting us know, and it's good to see that a simple solution works.