IDL bindings: Avoid extended attributes leaking onto merged interface.

IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

BUG=603782, 604292
Committed: https://crrev.com/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43
Cr-Commit-Position: refs/heads/master@{#388409}

[Matt Giuca, 2016-04-15 05:02:20]

mgiuca@chromium.org changed reviewers:
+ bashi@chromium.org

[Matt Giuca, 2016-04-15 05:02:20]

See bug report for details.

To see that the new tests catch this bug, revert the change to
interface_dependency_resolver.py and run
third_party/WebKit/Tools/Scripts/run-bindings-tests:

FAIL: V8TestInterface2Partial.cpp
---
/usr/local/google/home/mgiuca/chrome/src/third_party/WebKit/Source/bindings/tests/results/modules/V8TestInterface2Partial.cpp	2016-04-15
14:49:17.536982297 +1000
+++ /tmp/tmpEbGkJV/modules/V8TestInterface2Partial.cpp	2016-04-15
15:00:59.589963738 +1000
@@ -82,7 +82,9 @@
     v8::Local<v8::ObjectTemplate> prototypeTemplate =
interfaceTemplate->PrototypeTemplate();
     ALLOW_UNUSED_LOCAL(prototypeTemplate);
     // Register DOM constants, attributes and operations.
-    V8DOMConfiguration::installMethods(isolate, instanceTemplate,
prototypeTemplate, interfaceTemplate, signature, V8TestInterface2Methods,
WTF_ARRAY_LENGTH(V8TestInterface2Methods));
+    if (RuntimeEnabledFeatures::interface2PartialFeatureNameEnabled()) {
+        V8DOMConfiguration::installMethods(isolate, instanceTemplate,
prototypeTemplate, interfaceTemplate, signature, V8TestInterface2Methods,
WTF_ARRAY_LENGTH(V8TestInterface2Methods));
+    } // if (RuntimeEnabledFeatures::interface2PartialFeatureNameEnabled())
 
     if (RuntimeEnabledFeatures::interface2PartialFeatureNameEnabled()) {
         const V8DOMConfiguration::MethodConfiguration
voidMethodPartial1MethodConfiguration = {"voidMethodPartial1",
TestInterface2PartialV8Internal::voidMethodPartial1MethodCallback, 0, 1,
v8::None, V8DOMConfiguration::ExposedToAllScripts,
V8DOMConfiguration::OnPrototype};

[Matt Giuca, 2016-04-15 05:02:44]

Description was changed from

==========
IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

BUG=603782
==========

to

==========
IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

BUG=603782
==========

[bashi, 2016-04-15 05:50:54]

bashi@chromium.org changed reviewers:
+ tasak@google.com

[bashi, 2016-04-15 05:50:55]

I'm not familiar with partial interface handling. tasak-san, could you review
this?

https://codereview.chromium.org/1884423002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/interface_dependency_resolver.py
(right):

https://codereview.chromium.org/1884423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/interface_dependency_resolver.py:266:
value = dependency_interface.extended_attributes[key]
value = dependency_interface.extended_attributes.get(key)
if not value:
  continue

https://codereview.chromium.org/1884423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/interface_dependency_resolver.py:305:
try:
if 'ImplementedAs' in dependency_interface.extended_attributes:
  del dependency_interface.extended_attributes['ImplementedAs']

[Matt Giuca, 2016-04-15 06:21:43]

https://codereview.chromium.org/1884423002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/interface_dependency_resolver.py
(right):

https://codereview.chromium.org/1884423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/interface_dependency_resolver.py:266:
value = dependency_interface.extended_attributes[key]
On 2016/04/15 05:50:54, bashi1 wrote:
> value = dependency_interface.extended_attributes.get(key)
> if not value:
>   continue

Done. (Though I think the exception checking style would be considered more
Pythonic than get-and-test; I don't feel strongly about it in this case.)

https://codereview.chromium.org/1884423002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/interface_dependency_resolver.py:305:
try:
On 2016/04/15 05:50:54, bashi1 wrote:
> if 'ImplementedAs' in dependency_interface.extended_attributes:
>   del dependency_interface.extended_attributes['ImplementedAs']

That way involves looking up the value in the dictionary twice. Found a much
simpler way which is to just change the 'get' into a 'pop'. If it isn't in the
dict, it just returns the default and doesn't throw any error.

[Matt Giuca, 2016-04-18 06:14:36]

Description was changed from

==========
IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

BUG=603782
==========

to

==========
IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

Specifically, this adds webkitTemporaryStorage and
webkitPersistentStorage to WorkerNavigator (which are specified in the
IDL but weren't working due to this bug).

BUG=603782,604292
==========

[Matt Giuca, 2016-04-18 06:17:30]

mgiuca@chromium.org changed reviewers:
+ tkent@chromium.org

[Matt Giuca, 2016-04-18 06:17:30]

+tkent: Randomly chosen from API_OWNERS file. Can you comment on the changes to
LayoutTests (as discussed in https://crbug.com/604292) -- specifically, should
webkitTemporaryStorage and webkitPersistentStorage be added to WorkerNavigator?
(They previously weren't exposed due to this bug. By fixing the bug they will be
exposed, but is that desirable?)

[tkent, 2016-04-18 07:22:50]

Does storage team have an agreement on this?
If no users requested to expose them on workers, I don't think we need to expose
them now.
If you'd like to expose them, intent-to-ship is necessary.

[Matt Giuca, 2016-04-18 07:32:59]

On 2016/04/18 07:22:50, tkent wrote:
> Does storage team have an agreement on this?
> If no users requested to expose them on workers, I don't think we need to
expose
> them now.
> If you'd like to expose them, intent-to-ship is necessary.

I've added alecflett@chromium.org on the bug (who wrote the original
implementation) and did a bit of historical research there. Maybe he can comment
on the current state. It sounds like the best course of action is just to remove
these APIs entirely from WorkerNavigator, rather than launching them. Do you
agree?

[jsbell, 2016-04-18 17:04:10]

On 2016/04/18 07:32:59, Matt Giuca wrote:
> On 2016/04/18 07:22:50, tkent wrote:
> > Does storage team have an agreement on this?
> > If no users requested to expose them on workers, I don't think we need to
> expose
> > them now.
> > If you'd like to expose them, intent-to-ship is necessary.
> 
> I've added mailto:alecflett@chromium.org on the bug (who wrote the original
> implementation) and did a bit of historical research there. Maybe he can
comment
> on the current state. It sounds like the best course of action is just to
remove
> these APIs entirely from WorkerNavigator, rather than launching them. Do you
> agree?

alecflett@ is no longer active on Chromium.

Agreed that we should remove from WorkerNavigator.

(I just sent out an I2S for a more modern, non-prefixed version of a Quota API,
so definitely want to avoid exposing these. I was surprised to see that we had
these in Worker via webexposed... and I was unaware they were broken. Explains
why they aren't getting used!)

[jsbell, 2016-04-18 17:08:41]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2016-04-18 17:08:42]

nits

https://codereview.chromium.org/1884423002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/tests/idls/modules/TestInterface2Partial.idl
(right):

https://codereview.chromium.org/1884423002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/idls/modules/TestInterface2Partial.idl:2:
* Copyright (C) 2016 Google Inc. All rights reserved.
New files should use the newer license header:

https://www.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/1884423002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/tests/idls/modules/TestInterface2Partial2.idl
(right):

https://codereview.chromium.org/1884423002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/idls/modules/TestInterface2Partial2.idl:2:
* Copyright (C) 2016 Google Inc. All rights reserved.
Ditto.

[Matt Giuca, 2016-04-19 01:06:40]

Thanks for the tips. I've created a separate CL to remove those APIs
(https://codereview.chromium.org/1896043002) and have reverted the test
expectations for this CL.

https://codereview.chromium.org/1884423002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/tests/idls/modules/TestInterface2Partial.idl
(right):

https://codereview.chromium.org/1884423002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/idls/modules/TestInterface2Partial.idl:2:
* Copyright (C) 2016 Google Inc. All rights reserved.
On 2016/04/18 17:08:42, jsbell wrote:
> New files should use the newer license header:
> 
> https://www.chromium.org/blink/coding-style#TOC-License

Done.

https://codereview.chromium.org/1884423002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/tests/idls/modules/TestInterface2Partial2.idl
(right):

https://codereview.chromium.org/1884423002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/idls/modules/TestInterface2Partial2.idl:2:
* Copyright (C) 2016 Google Inc. All rights reserved.
On 2016/04/18 17:08:42, jsbell wrote:
> Ditto.

Done.

[bashi, 2016-04-19 03:52:51]

(It seems that tasak@ is busy so let me try to review this)

Bindings lgtm with comments.

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py (right):

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py:72:
'TestPartialInterface4.idl',
Not related to your CL, but it seems that we don't have
'TestPartialInterface4.idl'.

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py:73:
'TestInterface2Partial.idl',
Could you follow the convention? TestPartialInterface4.idl would be suitable
(Since we don't have TestPartialInterface4.idl).

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py:74:
'TestInterface2Partial2.idl',
TestPartialInterface5.idl

[haraken, 2016-04-19 04:18:54]

LGTM with bashi@'s comments addressed.

[Matt Giuca, 2016-04-19 04:30:52]

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py (right):

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py:72:
'TestPartialInterface4.idl',
On 2016/04/19 03:52:51, bashi1 wrote:
> Not related to your CL, but it seems that we don't have
> 'TestPartialInterface4.idl'.

Acknowledged.

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py:73:
'TestInterface2Partial.idl',
On 2016/04/19 03:52:51, bashi1 wrote:
> Could you follow the convention? TestPartialInterface4.idl would be suitable
> (Since we don't have TestPartialInterface4.idl).

I thought about that but it seemed funny to have TPI1--3 extending TI1 and
TPI4--5 extending TI2. What if someone wants to add a new TPI to TI1 later? Do
they call it TPI6 and have everything out of order? Or do they call it TPI4 and
rename my TPIs to 5 and 6? Seems much better to have a separate partial
interface numbering scheme for each test interface, which is why I started
TestInterface2PartialN.idl scheme.

If you still think I should call it TPI4--5, let me know and I will change it.

[Matt Giuca, 2016-04-19 04:31:30]

Description was changed from

==========
IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

Specifically, this adds webkitTemporaryStorage and
webkitPersistentStorage to WorkerNavigator (which are specified in the
IDL but weren't working due to this bug).

BUG=603782,604292
==========

to

==========
IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

BUG=603782,604292
==========

[bashi, 2016-04-19 04:50:27]

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py (right):

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py:73:
'TestInterface2Partial.idl',
On 2016/04/19 04:30:51, Matt Giuca wrote:
> On 2016/04/19 03:52:51, bashi1 wrote:
> > Could you follow the convention? TestPartialInterface4.idl would be suitable
> > (Since we don't have TestPartialInterface4.idl).
> 
> I thought about that but it seemed funny to have TPI1--3 extending TI1 and
> TPI4--5 extending TI2. What if someone wants to add a new TPI to TI1 later? Do
> they call it TPI6 and have everything out of order? Or do they call it TPI4
and
> rename my TPIs to 5 and 6? Seems much better to have a separate partial
> interface numbering scheme for each test interface, which is why I started
> TestInterface2PartialN.idl scheme.
Makes sense. Probably we should rename "TestPartialInterfaceX.idl" to
"TestInterfacePartialX.idl". Anyway this doesn't relate to your CL so PS#7 lgtm.

> 
> If you still think I should call it TPI4--5, let me know and I will change it.

[Matt Giuca, 2016-04-19 05:08:18]

Great, thanks.

Now blocking on https://codereview.chromium.org/1896043002.

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py (right):

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py:72:
'TestPartialInterface4.idl',
On 2016/04/19 04:30:51, Matt Giuca wrote:
> On 2016/04/19 03:52:51, bashi1 wrote:
> > Not related to your CL, but it seems that we don't have
> > 'TestPartialInterface4.idl'.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1884423002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/bindings/main.py:73:
'TestInterface2Partial.idl',
On 2016/04/19 04:50:27, bashi1 wrote:
> On 2016/04/19 04:30:51, Matt Giuca wrote:
> > On 2016/04/19 03:52:51, bashi1 wrote:
> > > Could you follow the convention? TestPartialInterface4.idl would be
suitable
> > > (Since we don't have TestPartialInterface4.idl).
> > 
> > I thought about that but it seemed funny to have TPI1--3 extending TI1 and
> > TPI4--5 extending TI2. What if someone wants to add a new TPI to TI1 later?
Do
> > they call it TPI6 and have everything out of order? Or do they call it TPI4
> and
> > rename my TPIs to 5 and 6? Seems much better to have a separate partial
> > interface numbering scheme for each test interface, which is why I started
> > TestInterface2PartialN.idl scheme.
> Makes sense. Probably we should rename "TestPartialInterfaceX.idl" to
> "TestInterfacePartialX.idl". Anyway this doesn't relate to your CL so PS#7
lgtm.
> 
> > 
> > If you still think I should call it TPI4--5, let me know and I will change
it.
> 

Acknowledged.

[Matt Giuca, 2016-04-19 23:52:28]

mgiuca@chromium.org changed reviewers:
+ haraken@chromium.org
- jsbell@chromium.org, tasak@google.com, tkent@chromium.org

[Matt Giuca, 2016-04-19 23:53:14]

Updated reviewer list (moved some reviewers to CC; tkent no longer required
because the Storage API has been removed).

[Matt Giuca, 2016-04-19 23:53:21]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-04-19 23:53:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/1884423002/#ps140001
(title: "Remove TestPartialInterface4.idl (which doesn't exist) from list.")

[commit-bot: I haz the power, 2016-04-19 23:53:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884423002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884423002/140001

[commit-bot: I haz the power, 2016-04-20 02:49:05]

Description was changed from

==========
IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

BUG=603782,604292
==========

to

==========
IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

BUG=603782,604292
==========

[commit-bot: I haz the power, 2016-04-20 02:49:06]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-22 19:20:13]

Description was changed from

==========
IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

BUG=603782,604292
==========

to

==========
IDL bindings: Avoid extended attributes leaking onto merged interface.

Some extended attributes from dependency interface definitions are
transferred onto the definition's attributes before being merged.
However, the merged interfaces still retain the extended attributes from
the definition that comes first alphabetically, which can result in
extended attributes from one partial interface "leaking" onto all of the
others. The transferred extended attributes are now deleted from the
dependency interface so that they do not leak onto the merged interface.

Fixes a bug where adding a new RuntimeEnabled condition in a partial
interface can apply the condition to all members of the final interface.

BUG=603782,604292

Committed: https://crrev.com/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43
Cr-Commit-Position: refs/heads/master@{#388409}
==========

[commit-bot: I haz the power, 2016-04-22 19:20:14]

Patchset 8 (id:??) landed as
https://crrev.com/4ea3a097a861c6b3ce5ef7ac9b3fd33c3ed92c43
Cr-Commit-Position: refs/heads/master@{#388409}