Re: [bindings] add support for integer-indexed @@iterator (issue 1381413003 by caitpotter88@gmail.com)

[j...@opera.com]

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

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/bindings/scripts/idl_definitions.py#newcode313
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/bindings/scripts/idl_definitions.py#newcode326
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/bindings/scripts/v8_interface.py
File third_party/WebKit/Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/bindings/scripts/v8_interface.py#newcode392
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.)

https://codereview.chromium.org/1381413003/

[yukis...@chromium.org]

+bashi@ just in case.


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

https://codereview.chromium.org/1381413003/diff/1/third_party/WebKit/Source/bindings/templates/interface_base.cpp#newcode350
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/bindings/templates/interface_base.cpp#newcode351
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/Source/bindings/templates/interface_base.cpp&l=431

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/bindings/scripts/v8_interface.py

https://codereview.chromium.org/1381413003/

[yukis...@chromium.org]

lgtm.

Please wait for jl@'s lgtm.

https://codereview.chromium.org/1381413003/

[j...@opera.com]

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

https://codereview.chromium.org/1381413003/

[joc...@chromium.org]

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

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

https://codereview.chromium.org/1381413003/

[joc...@chromium.org]

On 2015/10/05 at 15:24:51, caitpotter88 wrote:

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


https://codereview.chromium.org/1381413003/diff/20001/third_party/WebKit/public/web/WebRuntimeFeatures.h#newcode90
> 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.

https://codereview.chromium.org/1381413003/

[joc...@chromium.org]

lgtm



https://codereview.chromium.org/1381413003/

[har...@chromium.org]

LGTM


https://codereview.chromium.org/1381413003/

[commit-bot@chromium.org via codereview.chromium.org]

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


https://codereview.chromium.org/1381413003/

[commit-bot@chromium.org via codereview.chromium.org]

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_chromeos_rel_ng/builds/113353)

https://codereview.chromium.org/1381413003/

[joc...@chromium.org]

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?

https://codereview.chromium.org/1381413003/

[joc...@chromium.org]

On 2015/10/08 at 17:16:30, caitpotter88 wrote:

> 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/1drkxseLASL91hJQW4VjURa3ALRdGQj0kFtm6rZqsz0I/edit?usp=sharing

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

https://codereview.chromium.org/1381413003/

[joc...@chromium.org]

On 2015/10/08 at 18:31:28, caitpotter88 wrote:

> 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

https://codereview.chromium.org/1381413003/

[joc...@chromium.org]

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

+verwaest

https://codereview.chromium.org/1381413003/

[verw...@chromium.org]

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

https://codereview.chromium.org/1381413003/diff/120001/third_party/WebKit/Source/bindings/templates/interface_base.cpp#newcode354
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).

https://codereview.chromium.org/1381413003/

[joc...@chromium.org]

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

https://codereview.chromium.org/1381413003/

[joc...@chromium.org]

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.


https://codereview.chromium.org/1381413003/

[commit-bot@chromium.org via codereview.chromium.org]

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


https://codereview.chromium.org/1381413003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #7 (id:140001)

https://codereview.chromium.org/1381413003/

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/1381413003/

[mache...@chromium.org]

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

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.

https://codereview.chromium.org/1381413003/