[bindings] add support for integer-indexed @@iterator

[bindings] add support for integer-indexed @@iterator

Adds an @@iterator to interfaces containing an indexed
property getter and an integer-typed "length" attribute,
per WebIDL section 4.5.9 http://heycam.github.io/webidl/#es-iterators

This change adds for-of iteration support, using the same behaviour
as the builtin ECMAScript Array, so long as the behaviour is not
overridden by using an alternative method of declaring an iterable
interface.

Supercedes https://codereview.chromium.org/1383093002/

BUG=538558
LOG=N
R=jsbell@chromium.org, phillipj@opera.com, jl@opera.com, jochen@chromium.org

Committed: https://crrev.com/1da1f0b3d7ab1ff625779e09c55c75b452888f7b
Cr-Commit-Position: refs/heads/master@{#355541}

[caitp (gmail), 2015-10-04 23:10:47]

PTAL, more @@iterator support for the DOM.

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

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:351:
prototypeTemplate->Set(v8::Symbol::GetIterator(isolate),
v8::Array::GetValuesIterator(isolate), v8::DontEnum);
This doesn't use the V8DOMConfiguration helpers, because there isn't any
mechanism to set a symbol-keyed data property right now. It wouldn't be too much
work to add that, if that style is preferred?

Anyways, the gist of the functionality is here --- if the interface is found to
have an integer-typed "length" attribute, and a getter which takes a single
"unsigned long" argument, AND no other iterator type is declared, then the
default ArrayValues iterator from V8 is used.

The current version of this patch is blocked on
https://codereview.chromium.org/1378403004/ which exposes (all) of the builtin
Array iterator functions to the API. But, it could be rewritten to not need
that, and of course that CL can be shrunk a bit to not grow the native context
quite as much.

[Jens Widell, 2015-10-05 05:57:12]

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:313: if
attr.idl_type.is_integer_type:
Shouldn't we also check that the name is 'length' here?

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:326: if 'getter'
in op.specials and len(op.arguments) == 1 and op.arguments[0].idl_type ==
'unsigned long':
Checking len(op.arguments) is a bit optional here; getters can't really have
more than one argument. I think it would be fine for this code here to assume
that a getter has exactly one argument.

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_interface.py:392: if
(interface.iterable or interface.maplike or interface.setlike or 'Iterable' in
extended_attributes):
Minor nit: in Python you typically don't have parentheses here. (Only if you
break the expression up over several lines or something like that.)

[Yuki, 2015-10-05 07:08:26]

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

[Yuki, 2015-10-05 07:08:26]

+bashi@ just in case.

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

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:350: if
(RuntimeEnabledFeatures::iterableCollectionsEnabled()) {
Could you use {% filter runtime_enabled(...) %}?

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:351:
prototypeTemplate->Set(v8::Symbol::GetIterator(isolate),
v8::Array::GetValuesIterator(isolate), v8::DontEnum);
On 2015/10/04 23:10:47, caitp wrote:
> This doesn't use the V8DOMConfiguration helpers, because there isn't any
> mechanism to set a symbol-keyed data property right now. It wouldn't be too
much
> work to add that, if that style is preferred?
> 
> Anyways, the gist of the functionality is here --- if the interface is found
to
> have an integer-typed "length" attribute, and a getter which takes a single
> "unsigned long" argument, AND no other iterator type is declared, then the
> default ArrayValues iterator from V8 is used.
> 
> The current version of this patch is blocked on
> https://codereview.chromium.org/1378403004/ which exposes (all) of the builtin
> Array iterator functions to the API. But, it could be rewritten to not need
> that, and of course that CL can be shrunk a bit to not grow the native context
> quite as much.

Could you follow the way of the following?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Plus,
http://heycam.github.io/webidl/#es-iterators
says you need to define the iterator on the instance object, not on the
prototype object, in the case of [PrimaryGlobal] and [Global].  This applies to
Window interface.

I'm now introducing {{is_global}} in WIP CL http://crrev.com/1386843002 , but it
may take time.  Feel free to define your own {{is_global}}.
https://codereview.chromium.org/1386843002/diff/1/third_party/WebKit/Source/b...

[caitp (gmail), 2015-10-05 11:30:23]

Fixed up to put @@iterator on the instance for [Global] and [PrimaryGlobal]

I'd like to use V8DOMConfiguration for consistency, but I don't want to
implement this using C++-implemented accessors, because the accessors aren't
really needed for this one since the full functionality is already implemented
in JS on V8's side.

If you'd prefer that I add support to V8DOMConfiguration for installing
Symbol-keyed JS-valued data properties, I can do that.

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/idl_definitions.py (right):

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:313: if
attr.idl_type.is_integer_type:
On 2015/10/05 05:57:11, Jens Widell wrote:
> Shouldn't we also check that the name is 'length' here?

Yes, good catch. Done

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/idl_definitions.py:326: if 'getter'
in op.specials and len(op.arguments) == 1 and op.arguments[0].idl_type ==
'unsigned long':
On 2015/10/05 05:57:11, Jens Widell wrote:
> Checking len(op.arguments) is a bit optional here; getters can't really have
> more than one argument. I think it would be fine for this code here to assume
> that a getter has exactly one argument.

Done

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

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:351:
prototypeTemplate->Set(v8::Symbol::GetIterator(isolate),
v8::Array::GetValuesIterator(isolate), v8::DontEnum);
On 2015/10/05 07:08:26, Yuki wrote:
> Could you follow the way of the following?
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I don't think there is any value in implementing the method in C++ in this case.
Maybe later on, once there is a way in C++ to construct a V8 Array Iterator. But
in this case, all that is needed is a non-enumerable symbol keyed data property,
which comes from V8.

So to do that, I'd have to change V8DOMConfiguration to add a method for adding
symbol-keyed data properties. I'm not sure how worthwhile that is. Is that what
you're asking for here?

On 2015/10/05 07:08:26, Yuki wrote:
> Plus,
> http://heycam.github.io/webidl/#es-iterators
> says you need to define the iterator on the instance object, not on the
> prototype object, in the case of [PrimaryGlobal] and [Global].  This applies
to
> Window interface.
> 
> I'm now introducing {{is_global}} in WIP CL http://crrev.com/1386843002 , but
it
> may take time.  Feel free to define your own {{is_global}}.
>
https://codereview.chromium.org/1386843002/diff/1/third_party/WebKit/Source/b...

That's a good point, done

[Yuki, 2015-10-05 11:43:22]

lgtm.

Please wait for jl@'s lgtm.

[Jens Widell, 2015-10-05 12:35:04]

LGTM

We probably don't need to add three new test IDL files (and thus six new result
files.) There are plenty existing test IDL files that have indexed getters, just
not a 'length' property, and not [Global]/[PrimaryGlobal] (of which we strictly
speaking only need to test one, I think.)

We can add 'length' properties to any and all existing interfaces (less bloat
than whole new files for sure) and adding [Global] and/or [PrimaryGlobal] to one
or two might be a good thing in itself (it doesn't seem like we have any tests
for them right now.)

[caitp (gmail), 2015-10-05 14:13:59]

On 2015/10/05 12:35:04, Jens Widell wrote:
> LGTM
> 
> We probably don't need to add three new test IDL files (and thus six new
result
> files.) There are plenty existing test IDL files that have indexed getters,
just
> not a 'length' property, and not [Global]/[PrimaryGlobal] (of which we
strictly
> speaking only need to test one, I think.)
> 
> We can add 'length' properties to any and all existing interfaces (less bloat
> than whole new files for sure) and adding [Global] and/or [PrimaryGlobal] to
one
> or two might be a good thing in itself (it doesn't seem like we have any tests
> for them right now.)

About the tests, the existing TestInterface* tests (which would be the best
interfaces to extend this way) all implement iterators using other forms of
iterator declarations, so they override the default integer-indexed iterators.
Either way there are probably going to be at least 6 files added in order to get
proper coverage

[jochen (gone - plz use gerrit), 2015-10-05 15:23:22]

https://codereview.chromium.org/1381413003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/1381413003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebRuntimeFeatures.h:90: BLINK_EXPORT static void
enableIterableCollections(bool);
why do you need this?

[caitp (gmail), 2015-10-05 15:24:51]

https://codereview.chromium.org/1381413003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/1381413003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebRuntimeFeatures.h:90: BLINK_EXPORT static void
enableIterableCollections(bool);
On 2015/10/05 15:23:22, jochen wrote:
> why do you need this?

I figured I'd be asked to put this behind a flag. If people think it doesn't
need to live behind a flag for a while, then I suppose it's not needed

[jochen (gone - plz use gerrit), 2015-10-05 15:32:12]

On 2015/10/05 at 15:24:51, caitpotter88 wrote:
>
https://codereview.chromium.org/1381413003/diff/20001/third_party/WebKit/publ...
> File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):
> 
>
https://codereview.chromium.org/1381413003/diff/20001/third_party/WebKit/publ...
> third_party/WebKit/public/web/WebRuntimeFeatures.h:90: BLINK_EXPORT static
void enableIterableCollections(bool);
> On 2015/10/05 15:23:22, jochen wrote:
> > why do you need this?
> 
> I figured I'd be asked to put this behind a flag. If people think it doesn't
need to live behind a flag for a while, then I suppose it's not needed

the change in the RuntimeEnabledFeatures.in already guards it by a flag. you'd
only need a public api if you want to be able to enable it on some platforms but
not others.

[caitp (gmail), 2015-10-05 15:39:09]

On 2015/10/05 15:32:12, jochen wrote:
> On 2015/10/05 at 15:24:51, caitpotter88 wrote:
> >
>
https://codereview.chromium.org/1381413003/diff/20001/third_party/WebKit/publ...
> > File third_party/WebKit/public/web/WebRuntimeFeatures.h (right):
> > 
> >
>
https://codereview.chromium.org/1381413003/diff/20001/third_party/WebKit/publ...
> > third_party/WebKit/public/web/WebRuntimeFeatures.h:90: BLINK_EXPORT static
> void enableIterableCollections(bool);
> > On 2015/10/05 15:23:22, jochen wrote:
> > > why do you need this?
> > 
> > I figured I'd be asked to put this behind a flag. If people think it doesn't
> need to live behind a flag for a while, then I suppose it's not needed
> 
> the change in the RuntimeEnabledFeatures.in already guards it by a flag. you'd
> only need a public api if you want to be able to enable it on some platforms
but
> not others.

I see, done

[caitp (gmail), 2015-10-06 12:08:49]

Patchset #5 (id:80001) has been deleted

[jochen (gone - plz use gerrit), 2015-10-07 09:34:34]

lgtm

[caitp (gmail), 2015-10-07 11:45:18]

On 2015/10/07 09:34:34, jochen wrote:
> lgtm

Thanks --- This can probably land after the next V8 roll if people are okay with
that

[haraken, 2015-10-07 12:09:51]

LGTM

[caitp (gmail), 2015-10-08 12:47:14]

The API changes needed were rolled, so CQing now. I may need to fix up some
layout tests after

[caitp (gmail), 2015-10-08 12:47:19]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-10-08 12:47:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org, jl@opera.com, jochen@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1381413003/#ps120001
(title: "Rebase just in case")

[commit-bot: I haz the power, 2015-10-08 12:47:32]

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

[commit-bot: I haz the power, 2015-10-08 13:27:57]

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

[commit-bot: I haz the power, 2015-10-08 13:27:58]

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

[caitp (gmail), 2015-10-08 14:26:41]

On 2015/10/08 12:47:14, caitp wrote:
> The API changes needed were rolled, so CQing now. I may need to fix up some
> layout tests after

So there's a bit of a problem --- Adding the @@iterator symbol to the global
instance causes leaks to be detected in
WebLeakDetectorClient::onLeakDetectionComplete().

Not adding the symbol to the global object gets tests passing, but that's not
really the right behaviour. Is there some specific cleanup that needs to happen
somewhere?

[caitp (gmail), 2015-10-08 17:09:15]

On 2015/10/08 14:26:41, caitp wrote:
> On 2015/10/08 12:47:14, caitp wrote:
> > The API changes needed were rolled, so CQing now. I may need to fix up some
> > layout tests after
> 
> So there's a bit of a problem --- Adding the @@iterator symbol to the global
> instance causes leaks to be detected in
> WebLeakDetectorClient::onLeakDetectionComplete().
> 
> Not adding the symbol to the global object gets tests passing, but that's not
> really the right behaviour. Is there some specific cleanup that needs to
happen
> somewhere?

On a more recent pull, the tests are passing locally. I guess I'll try CQing
again

[caitp (gmail), 2015-10-08 17:11:10]

On 2015/10/08 17:09:15, caitp wrote:
> On 2015/10/08 14:26:41, caitp wrote:
> > On 2015/10/08 12:47:14, caitp wrote:
> > > The API changes needed were rolled, so CQing now. I may need to fix up
some
> > > layout tests after
> > 
> > So there's a bit of a problem --- Adding the @@iterator symbol to the global
> > instance causes leaks to be detected in
> > WebLeakDetectorClient::onLeakDetectionComplete().
> > 
> > Not adding the symbol to the global object gets tests passing, but that's
not
> > really the right behaviour. Is there some specific cleanup that needs to
> happen
> > somewhere?
> 
> On a more recent pull, the tests are passing locally. I guess I'll try CQing
> again

or... maybe not? :(

[jochen (gone - plz use gerrit), 2015-10-08 17:12:00]

On 2015/10/08 at 14:26:41, caitpotter88 wrote:
> On 2015/10/08 12:47:14, caitp wrote:
> > The API changes needed were rolled, so CQing now. I may need to fix up some
> > layout tests after
> 
> So there's a bit of a problem --- Adding the @@iterator symbol to the global
instance causes leaks to be detected in
WebLeakDetectorClient::onLeakDetectionComplete().
> 
> Not adding the symbol to the global object gets tests passing, but that's not
really the right behaviour. Is there some specific cleanup that needs to happen
somewhere?

can you figure out what keeps the global object alive if you pass the symbol?

[caitp (gmail), 2015-10-08 17:16:30]

On 2015/10/08 17:12:00, jochen wrote:
> On 2015/10/08 at 14:26:41, caitpotter88 wrote:
> > On 2015/10/08 12:47:14, caitp wrote:
> > > The API changes needed were rolled, so CQing now. I may need to fix up
some
> > > layout tests after
> > 
> > So there's a bit of a problem --- Adding the @@iterator symbol to the global
> instance causes leaks to be detected in
> WebLeakDetectorClient::onLeakDetectionComplete().
> > 
> > Not adding the symbol to the global object gets tests passing, but that's
not
> really the right behaviour. Is there some specific cleanup that needs to
happen
> somewhere?
> 
> can you figure out what keeps the global object alive if you pass the symbol?

I'm not sure what it is, it looks like the ScriptState and all per-context data
are dead, but something is keeping it alive. I've tried to reproduce this in a
plain webpage using --enable-leak-detection, but haven't had any luck with this
yet.

[jochen (gone - plz use gerrit), 2015-10-08 17:22:11]

On 2015/10/08 at 17:16:30, caitpotter88 wrote:
> On 2015/10/08 17:12:00, jochen wrote:
> > On 2015/10/08 at 14:26:41, caitpotter88 wrote:
> > > On 2015/10/08 12:47:14, caitp wrote:
> > > > The API changes needed were rolled, so CQing now. I may need to fix up
some
> > > > layout tests after
> > > 
> > > So there's a bit of a problem --- Adding the @@iterator symbol to the
global
> > instance causes leaks to be detected in
> > WebLeakDetectorClient::onLeakDetectionComplete().
> > > 
> > > Not adding the symbol to the global object gets tests passing, but that's
not
> > really the right behaviour. Is there some specific cleanup that needs to
happen
> > somewhere?
> > 
> > can you figure out what keeps the global object alive if you pass the
symbol?
> 
> I'm not sure what it is, it looks like the ScriptState and all per-context
data are dead, but something is keeping it alive. I've tried to reproduce this
in a plain webpage using --enable-leak-detection, but haven't had any luck with
this yet.

you need to have the entire layout test infrastructure in place for this option
to work. it's probably easiest to reproduce with run-webkit-tests
--enable-leak-detection

I wrote a doc with an example debugging session:
https://docs.google.com/document/d/1drkxseLASL91hJQW4VjURa3ALRdGQj0kFtm6rZqsz...

it might be easier to stare at the CL for a while and trying to figure out
what's going on..

[caitp (gmail), 2015-10-08 18:31:28]

On 2015/10/08 17:22:11, jochen wrote:
> On 2015/10/08 at 17:16:30, caitpotter88 wrote:
> > On 2015/10/08 17:12:00, jochen wrote:
> > > On 2015/10/08 at 14:26:41, caitpotter88 wrote:
> > > > On 2015/10/08 12:47:14, caitp wrote:
> > > > > The API changes needed were rolled, so CQing now. I may need to fix up
> some
> > > > > layout tests after
> > > > 
> > > > So there's a bit of a problem --- Adding the @@iterator symbol to the
> global
> > > instance causes leaks to be detected in
> > > WebLeakDetectorClient::onLeakDetectionComplete().
> > > > 
> > > > Not adding the symbol to the global object gets tests passing, but
that's
> not
> > > really the right behaviour. Is there some specific cleanup that needs to
> happen
> > > somewhere?
> > > 
> > > can you figure out what keeps the global object alive if you pass the
> symbol?
> > 
> > I'm not sure what it is, it looks like the ScriptState and all per-context
> data are dead, but something is keeping it alive. I've tried to reproduce this
> in a plain webpage using --enable-leak-detection, but haven't had any luck
with
> this yet.
> 
> you need to have the entire layout test infrastructure in place for this
option
> to work. it's probably easiest to reproduce with run-webkit-tests
> --enable-leak-detection
> 
> I wrote a doc with an example debugging session:
>
https://docs.google.com/document/d/1drkxseLASL91hJQW4VjURa3ALRdGQj0kFtm6rZqsz...
> 
> it might be easier to stare at the CL for a while and trying to figure out
> what's going on..

I'm not sure what is hanging on to the reference to that property, but it
affects all layout tests without exception --- only if it's present on the
global object.

Since on the V8 side, the function is implemented in JS, it may have a context
allocated reference to the global variable (somewhere up the chain), which
produces a cycle. Although this is really the builtins object and not the real
global, so that still doesn't make a ton of sense.

Maybe it's simpler to just not expose @@iterator on the global object, and wait
until it's possible to install this property on the template weakly? Since the
global object should end up as a Persistent<> anyways, that seems like it should
be doable.

[jochen (gone - plz use gerrit), 2015-10-08 18:39:37]

On 2015/10/08 at 18:31:28, caitpotter88 wrote:
> On 2015/10/08 17:22:11, jochen wrote:
> > On 2015/10/08 at 17:16:30, caitpotter88 wrote:
> > > On 2015/10/08 17:12:00, jochen wrote:
> > > > On 2015/10/08 at 14:26:41, caitpotter88 wrote:
> > > > > On 2015/10/08 12:47:14, caitp wrote:
> > > > > > The API changes needed were rolled, so CQing now. I may need to fix
up
> > some
> > > > > > layout tests after
> > > > > 
> > > > > So there's a bit of a problem --- Adding the @@iterator symbol to the
> > global
> > > > instance causes leaks to be detected in
> > > > WebLeakDetectorClient::onLeakDetectionComplete().
> > > > > 
> > > > > Not adding the symbol to the global object gets tests passing, but
that's
> > not
> > > > really the right behaviour. Is there some specific cleanup that needs to
> > happen
> > > > somewhere?
> > > > 
> > > > can you figure out what keeps the global object alive if you pass the
> > symbol?
> > > 
> > > I'm not sure what it is, it looks like the ScriptState and all per-context
> > data are dead, but something is keeping it alive. I've tried to reproduce
this
> > in a plain webpage using --enable-leak-detection, but haven't had any luck
with
> > this yet.
> > 
> > you need to have the entire layout test infrastructure in place for this
option
> > to work. it's probably easiest to reproduce with run-webkit-tests
> > --enable-leak-detection
> > 
> > I wrote a doc with an example debugging session:
> >
https://docs.google.com/document/d/1drkxseLASL91hJQW4VjURa3ALRdGQj0kFtm6rZqsz...
> > 
> > it might be easier to stare at the CL for a while and trying to figure out
> > what's going on..
> 
> I'm not sure what is hanging on to the reference to that property, but it
affects all layout tests without exception --- only if it's present on the
global object.
> 
> Since on the V8 side, the function is implemented in JS, it may have a context
allocated reference to the global variable (somewhere up the chain), which
produces a cycle. Although this is really the builtins object and not the real
global, so that still doesn't make a ton of sense.
> 
> Maybe it's simpler to just not expose @@iterator on the global object, and
wait until it's possible to install this property on the template weakly? Since
the global object should end up as a Persistent<> anyways, that seems like it
should be doable.

Hum, the function certainly has a reference to its creation context. But that
needs to be the same context the global object belongs to, otherwise we'd have
an XSS bug

[caitp (gmail), 2015-10-09 16:15:38]

On 2015/10/08 18:39:37, jochen wrote:
> On 2015/10/08 at 18:31:28, caitpotter88 wrote:
> > On 2015/10/08 17:22:11, jochen wrote:
> > > On 2015/10/08 at 17:16:30, caitpotter88 wrote:
> > > > On 2015/10/08 17:12:00, jochen wrote:
> > > > > On 2015/10/08 at 14:26:41, caitpotter88 wrote:
> > > > > > On 2015/10/08 12:47:14, caitp wrote:
> > > > > > > The API changes needed were rolled, so CQing now. I may need to
fix
> up
> > > some
> > > > > > > layout tests after
> > > > > > 
> > > > > > So there's a bit of a problem --- Adding the @@iterator symbol to
the
> > > global
> > > > > instance causes leaks to be detected in
> > > > > WebLeakDetectorClient::onLeakDetectionComplete().
> > > > > > 
> > > > > > Not adding the symbol to the global object gets tests passing, but
> that's
> > > not
> > > > > really the right behaviour. Is there some specific cleanup that needs
to
> > > happen
> > > > > somewhere?
> > > > > 
> > > > > can you figure out what keeps the global object alive if you pass the
> > > symbol?
> > > > 
> > > > I'm not sure what it is, it looks like the ScriptState and all
per-context
> > > data are dead, but something is keeping it alive. I've tried to reproduce
> this
> > > in a plain webpage using --enable-leak-detection, but haven't had any luck
> with
> > > this yet.
> > > 
> > > you need to have the entire layout test infrastructure in place for this
> option
> > > to work. it's probably easiest to reproduce with run-webkit-tests
> > > --enable-leak-detection
> > > 
> > > I wrote a doc with an example debugging session:
> > >
>
https://docs.google.com/document/d/1drkxseLASL91hJQW4VjURa3ALRdGQj0kFtm6rZqsz...
> > > 
> > > it might be easier to stare at the CL for a while and trying to figure out
> > > what's going on..
> > 
> > I'm not sure what is hanging on to the reference to that property, but it
> affects all layout tests without exception --- only if it's present on the
> global object.
> > 
> > Since on the V8 side, the function is implemented in JS, it may have a
context
> allocated reference to the global variable (somewhere up the chain), which
> produces a cycle. Although this is really the builtins object and not the real
> global, so that still doesn't make a ton of sense.
> > 
> > Maybe it's simpler to just not expose @@iterator on the global object, and
> wait until it's possible to install this property on the template weakly?
Since
> the global object should end up as a Persistent<> anyways, that seems like it
> should be doable.
> 
> Hum, the function certainly has a reference to its creation context. But that
> needs to be the same context the global object belongs to, otherwise we'd have
> an XSS bug

Using a (hacked to build) version of your reference retainer tracing CL, and
running the test, I get the following:
https://gist.github.com/caitp/c167cb29390eccec36a2

I'm not 100% sure I understand all of the output, but it looks like the native
context is kept alive, because values is installed on the global instance
template, which is an "eternal object" and is kept alive indefinitely?

The simplest thing for now is to just leave the method off of the global
interfaces, and I could try to get v8/src/heap owners to approve a CL which lets
native context fields live on Templates without leaking.Does that sound like the
wrong way around it?

[jochen (gone - plz use gerrit), 2015-10-12 11:25:28]

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

[jochen (gone - plz use gerrit), 2015-10-12 11:25:29]

the getter / setter function on a global object really should be created in that
global object's context.

+verwaest

[Toon Verwaest, 2015-10-14 10:59:42]

https://codereview.chromium.org/1381413003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1381413003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:354:
instanceTemplate->Set(v8::Symbol::GetIterator(isolate),
v8::Array::GetValuesIterator(isolate), v8::DontEnum);
Instance templates that are supposed to be instantiated in multiple native
contexts cannot have non-primitive members. Otherwise there's a cross-context
leak between the context that the values are taken from, and the context they
are injected into. In this case the .constructor (Function) from the context
active during template creation leaks, creating a full UXSS between that context
and any context that gets global objects afterwards. (Which provides a channel
between all contexts and origins).

[caitp (gmail), 2015-10-14 12:32:03]

https://codereview.chromium.org/1381413003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1381413003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:354:
instanceTemplate->Set(v8::Symbol::GetIterator(isolate),
v8::Array::GetValuesIterator(isolate), v8::DontEnum);
On 2015/10/14 10:59:42, Toon Verwaest wrote:
> Instance templates that are supposed to be instantiated in multiple native
> contexts cannot have non-primitive members. Otherwise there's a cross-context
> leak between the context that the values are taken from, and the context they
> are injected into. In this case the .constructor (Function) from the context
> active during template creation leaks, creating a full UXSS between that
context
> and any context that gets global objects afterwards. (Which provides a channel
> between all contexts and origins).

It sounds like the non-global interfaces could also have this problem, I'm not
sure I understand why they don't.

What would be an alternative strategy that people could live with?

[caitp (gmail), 2015-10-16 10:55:09]

On 2015/10/14 12:32:03, caitp wrote:
>
https://codereview.chromium.org/1381413003/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):
> 
>
https://codereview.chromium.org/1381413003/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/templates/interface_base.cpp:354:
> instanceTemplate->Set(v8::Symbol::GetIterator(isolate),
> v8::Array::GetValuesIterator(isolate), v8::DontEnum);
> On 2015/10/14 10:59:42, Toon Verwaest wrote:
> > Instance templates that are supposed to be instantiated in multiple native
> > contexts cannot have non-primitive members. Otherwise there's a
cross-context
> > leak between the context that the values are taken from, and the context
they
> > are injected into. In this case the .constructor (Function) from the context
> > active during template creation leaks, creating a full UXSS between that
> context
> > and any context that gets global objects afterwards. (Which provides a
channel
> > between all contexts and origins).
> 
> It sounds like the non-global interfaces could also have this problem, I'm not
> sure I understand why they don't.
> 
> What would be an alternative strategy that people could live with?

An API like https://codereview.chromium.org/1409593002/ makes this possible. It
seems that the iterator accessors that were landed earlier need a change, to not
use the current context. Or they could just be removed

[jochen (gone - plz use gerrit), 2015-10-21 15:27:10]

btw, since you apparently rebased the tracing patch for v8, would you mind
uploading it?

[caitp (gmail), 2015-10-21 16:01:34]

On 2015/10/21 15:27:10, jochen wrote:
> btw, since you apparently rebased the tracing patch for v8, would you mind
> uploading it?

I don't still have the rebased patch, but I'll try to rebuild it? It was just a
(very) careful `git apply -3` of
https://codereview.chromium.org/download/issue1187563010_1.diff

[jochen (gone - plz use gerrit), 2015-10-21 16:03:39]

On 2015/10/21 at 16:01:34, caitpotter88 wrote:
> On 2015/10/21 15:27:10, jochen wrote:
> > btw, since you apparently rebased the tracing patch for v8, would you mind
> > uploading it?
> 
> I don't still have the rebased patch, but I'll try to rebuild it? It was just
a (very) careful `git apply -3` of
https://codereview.chromium.org/download/issue1187563010_1.

Nah, don't bother if you no longer have it.

[caitp (gmail), 2015-10-22 14:10:34]

This should work now, going to try a CQ after running browsertests locally

[caitp (gmail), 2015-10-22 14:24:04]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-10-22 14:24:04]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
yukishiino@chromium.org, haraken@chromium.org, jl@opera.com
Link to the patchset: https://codereview.chromium.org/1381413003/#ps140001
(title: "Use SetIntrinsicDataProperty() in new V8 roll")

[commit-bot: I haz the power, 2015-10-22 14:24:35]

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

[commit-bot: I haz the power, 2015-10-22 15:22:29]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-10-22 15:23:20]

Patchset 7 (id:??) landed as
https://crrev.com/1da1f0b3d7ab1ff625779e09c55c75b452888f7b
Cr-Commit-Position: refs/heads/master@{#355541}

[Michael Achenbach, 2015-10-22 19:22:20]

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

[Michael Achenbach, 2015-10-22 19:22:21]

FYI (no action required now): This breaks the api stability checker:
http://build.chromium.org/p/chromium.fyi/builders/Linux%20V8%20API%20Stabilit...

That means, the CL depended on code very recently submitted to v8. In rare
cases, we want to be able to revert v8 for the current canary. Therefore, the
chromium code needs to stay compatible with the v8 code from the last canary.

Please leave enough time in the future between v8 CLs and dependent chromium CLs
(several days).

Please object if I'm wrong - the api checker is still in beta.

[caitp (gmail), 2015-10-22 19:24:27]

On 2015/10/22 19:22:21, Michael Achenbach wrote:
> FYI (no action required now): This breaks the api stability checker:
>
http://build.chromium.org/p/chromium.fyi/builders/Linux%20V8%20API%20Stabilit...
> 
> That means, the CL depended on code very recently submitted to v8. In rare
> cases, we want to be able to revert v8 for the current canary. Therefore, the
> chromium code needs to stay compatible with the v8 code from the last canary.
> 
> Please leave enough time in the future between v8 CLs and dependent chromium
CLs
> (several days).
> 
> Please object if I'm wrong - the api checker is still in beta.

My bad, the API change landed yesterday.