[proxies] use [[GetPrototypeOf]] trap in for-in

[proxies] use [[GetPrototypeOf]] trap in for-in key accumulation

With the recent spec change removing the [[Enumerate]] internal method, we now
have to walk the complete prototype chain. This implies that we call the
[[GetPrototypeOf]] trap on proxies.

As a secondary change we now trigger the [[GetOwnProperty]] trap for the for-in
filter step to see whether the properties are still enumerable. Before we did this
in the key-accumulation phase. This way we slightly reduce the number of traps
invoked. Whilst this is not ideal, it comes closer to the Spec's example
implementation.

BUG=v8:1543, v8:4768
LOG=n

Committed: https://crrev.com/2efc1381313248352630b05f6d48064badfe7671
Cr-Commit-Position: refs/heads/master@{#35017}

[Camillo Bruni, 2016-03-01 12:48:42]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 12:48:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748923003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748923003/1

[Camillo Bruni, 2016-03-01 12:49:07]

cbruni@chromium.org changed reviewers:
+ neis@chromium.org

[Camillo Bruni, 2016-03-01 12:49:07]

PTAL

[commit-bot: I haz the power, 2016-03-01 13:08:22]

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

[commit-bot: I haz the power, 2016-03-01 13:08:23]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/16279)

[neis, 2016-03-01 13:40:31]

Hi Camillo,
besides what we discussed offline, I think we should have more tests about this
stuff.  Below are just a two suggestions.

https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
File test/mjsunit/harmony/proxies-for.js (right):

https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-for.js:141: assertEquals(["0"], keys(o));
Also do this test when o is empty, for instance.

https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-for.js:168: assertEquals(["1","2"],
Object.getOwnPropertyNames(p));
Also call getOwnPropertySymbols here.

https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
File test/mjsunit/harmony/proxies-prototype-target-stackoverflow.js (right):

https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-prototype-target-stackoverflow.js:14: }
Not sure if this was temporary debugging code or not.

[Camillo Bruni, 2016-03-01 19:04:58]

I think we have a bigger issue at hand. 
With the new spec change, it seem that any kind of reasonable optimization has
to be disable the moment we have proxies around.
We are forced to call [[GetOwnProperty]] to determine whether a property is
enumerable and we almost certainly have to walk the prototype chain including
[[GetPrototypeOf]] calls.
This implies that in our for-in filter step we can no longer simply call [[Has]]
on the receiver and stop at the proxy.
A side-effect is that we will call the [[GetPrototypeOf]] trap twice (once
during for-in-enumerate and once during for-in-filter), which is probably fine
according to the spec, but kind of makes proxies less useful.

Additionally there is now limited use to the [[Has]] trap on the proxy
(essentially only the in-operater will use it).
Which makes me think why aren't there explicit traps for [[IsEnumerable]],
[[IsConfigurable]].. and [[GetOwnProperty]] assembles the values from these
traps.

https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
File test/mjsunit/harmony/proxies-for.js (right):

https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-for.js:141: assertEquals(["0"], keys(o));
On 2016/03/01 at 13:40:31, neis wrote:
> Also do this test when o is empty, for instance.

done.

https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-for.js:168: assertEquals(["1","2"],
Object.getOwnPropertyNames(p));
On 2016/03/01 at 13:40:31, neis wrote:
> Also call getOwnPropertySymbols here.

done.

https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
File test/mjsunit/harmony/proxies-prototype-target-stackoverflow.js (right):

https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-prototype-target-stackoverflow.js:14: }
On 2016/03/01 at 13:40:31, neis wrote:
> Not sure if this was temporary debugging code or not.

is debugging code :)

[Dan Ehrenberg, 2016-03-01 19:35:22]

On 2016/03/01 at 19:04:58, cbruni wrote:
> I think we have a bigger issue at hand. 
> With the new spec change, it seem that any kind of reasonable optimization has
to be disable the moment we have proxies around.
> We are forced to call [[GetOwnProperty]] to determine whether a property is
enumerable and we almost certainly have to walk the prototype chain including
[[GetPrototypeOf]] calls.
> This implies that in our for-in filter step we can no longer simply call
[[Has]] on the receiver and stop at the proxy.
> A side-effect is that we will call the [[GetPrototypeOf]] trap twice (once
during for-in-enumerate and once during for-in-filter), which is probably fine
according to the spec, but kind of makes proxies less useful.
> 
> Additionally there is now limited use to the [[Has]] trap on the proxy
(essentially only the in-operater will use it).
> Which makes me think why aren't there explicit traps for [[IsEnumerable]],
[[IsConfigurable]].. and [[GetOwnProperty]] assembles the values from these
traps.
> 
>
https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
> File test/mjsunit/harmony/proxies-for.js (right):
> 
>
https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
> test/mjsunit/harmony/proxies-for.js:141: assertEquals(["0"], keys(o));
> On 2016/03/01 at 13:40:31, neis wrote:
> > Also do this test when o is empty, for instance.
> 
> done.
> 
>
https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
> test/mjsunit/harmony/proxies-for.js:168: assertEquals(["1","2"],
Object.getOwnPropertyNames(p));
> On 2016/03/01 at 13:40:31, neis wrote:
> > Also call getOwnPropertySymbols here.
> 
> done.
> 
>
https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
> File test/mjsunit/harmony/proxies-prototype-target-stackoverflow.js (right):
> 
>
https://codereview.chromium.org/1748923003/diff/1/test/mjsunit/harmony/proxie...
> test/mjsunit/harmony/proxies-prototype-target-stackoverflow.js:14: }
> On 2016/03/01 at 13:40:31, neis wrote:
> > Not sure if this was temporary debugging code or not.
> 
> is debugging code :)

The new spec is actually really vague. That sample code is non-normative. I
don't think it's too late for feedback to the committee here.

[Camillo Bruni, 2016-03-01 20:58:16]

On 2016/03/01 at 19:35:22, littledan wrote:
> 
> The new spec is actually really vague. That sample code is non-normative. I
don't think it's too late for feedback to the committee here.

Maybe that would be a good call, for my taste it is too fuzzy.
I know that it was willingly kept this way to allow for optimizations, but
currently I don't see how we can implement it much differently than what I just
did in this CL.

[Dan Ehrenberg, 2016-03-01 23:53:01]

On 2016/03/01 at 20:58:16, cbruni wrote:
> On 2016/03/01 at 19:35:22, littledan wrote:
> > 
> > The new spec is actually really vague. That sample code is non-normative. I
don't think it's too late for feedback to the committee here.
> 
> Maybe that would be a good call, for my taste it is too fuzzy.
> I know that it was willingly kept this way to allow for optimizations, but
currently I don't see how we can implement it much differently than what I just
did in this CL.

Oh, I guess there's this sentence:

"Property attributes of the target object must be obtained by calling its
[[GetOwnProperty]] internal method."

But, how else would we do it, given that there's no isEnumerable trap? It
doesn't seem like it *prohibits* calling the has trap, given how vague
everything is. There's an outstanding unresolved issue where IE checks
enumerability as it goes through the loop, and other browsers don't; the
majority behavior basically corresponds to calling GetOwnProperty in the first
step and Has later, so it would seem to be a reasonable definition. Given the
vagueness, I think it's fine for us to experiment with what's optimizable and
report back to the committee.

[rossberg, 2016-03-02 14:29:14]

+1 to what Dan said. We intentionally asked to make the specification vague
enough to not break existing implementations. In particular, it does not specify
_when_ enumerability is checked, and so allows doing that for all properties
_before_ starting to yield.

[neis, 2016-03-04 09:29:30]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-04 09:29:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748923003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748923003/80001

[commit-bot: I haz the power, 2016-03-04 09:46:24]

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

[commit-bot: I haz the power, 2016-03-04 09:46:24]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)

[Camillo Bruni, 2016-03-04 13:48:28]

Description was changed from

==========
[proxies] use [[GetPrototypeOf]] trap in for-in

With the recent spec change removing the [[Enumerate]] internal method, we now
have to walk the complete prototype chain. This implies that we call the
[[GetPrototypeOf]] trap on proxies, otherwise the target's prototype keys
would not show up in a for-in loop.

BUG=
==========

to

==========
[proxies] use [[GetPrototypeOf]] trap in for-in key accumulation

With the recent spec change removing the [[Enumerate]] internal method, we now
have to walk the complete prototype chain. This implies that we call the
[[GetPrototypeOf]] trap on proxies, otherwise the target's prototype keys
would not show up in a for-in loop.

BUG=
==========

[Camillo Bruni, 2016-03-11 13:30:41]

Description was changed from

==========
[proxies] use [[GetPrototypeOf]] trap in for-in key accumulation

With the recent spec change removing the [[Enumerate]] internal method, we now
have to walk the complete prototype chain. This implies that we call the
[[GetPrototypeOf]] trap on proxies, otherwise the target's prototype keys
would not show up in a for-in loop.

BUG=
==========

to

==========
[proxies] use [[GetPrototypeOf]] trap in for-in key accumulation

With the recent spec change removing the [[Enumerate]] internal method, we now
have to walk the complete prototype chain. This implies that we call the
[[GetPrototypeOf]] trap on proxies.

As a secondary change we now trigger the [[GetOwnProperty]] trap for the for-in
filter step to see whether the properties are still enumerable. Before we did
this
in the key-accumulation phase. This way we slightly reduce the number of traps
invoked. Whilst this is not ideal, it comes closer to the Spec's example
implementation.

BUG=v8:1543, v8:4768
LOG=n
==========

[Camillo Bruni, 2016-03-16 11:50:19]

cbruni@chromium.org changed reviewers:
+ jkummerow@chromium.org

[neis, 2016-03-16 13:05:54]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-16 13:06:06]

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

[commit-bot: I haz the power, 2016-03-16 13:08:56]

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

[commit-bot: I haz the power, 2016-03-16 13:08:57]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/2999)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[Camillo Bruni, 2016-03-16 15:45:39]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-16 15:45:46]

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

[commit-bot: I haz the power, 2016-03-16 16:08:11]

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

[commit-bot: I haz the power, 2016-03-16 16:08:12]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/3024)
  v8_linux64_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)

[Camillo Bruni, 2016-03-17 20:24:47]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-17 20:24:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748923003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748923003/160001

[commit-bot: I haz the power, 2016-03-17 20:48:51]

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

[commit-bot: I haz the power, 2016-03-17 20:48:53]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-03-17 21:12:05]

PTAL again... what a struggle

[Jakob Kummerow, 2016-03-18 14:12:00]

LGTM, just nits.

https://codereview.chromium.org/1748923003/diff/160001/src/keys.h
File src/keys.h (right):

https://codereview.chromium.org/1748923003/diff/160001/src/keys.h#newcode92
src/keys.h:92: // prototype chain and forwords the complex/slow cases to the
normal
drive-by nit: s/forwords/forwards/

https://codereview.chromium.org/1748923003/diff/160001/src/runtime/runtime-fo...
File src/runtime/runtime-forin.cc (right):

https://codereview.chromium.org/1748923003/diff/160001/src/runtime/runtime-fo...
src/runtime/runtime-forin.cc:39: // This is a slight modifcation of
JSReceiver::HasProperty, dealing with
nit: missng 'i' ;-)

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/for-in-op...
File test/mjsunit/for-in-opt.js (left):

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/for-in-op...
test/mjsunit/for-in-opt.js:94: var handler2 = {
That's weird. This careful setup of handler2/proxy2/o2 looks a lot like they
should be used somewhere. Maybe in the f4(o, p) call in line 114? But then the
expectations in check_f4 wouldn't make sense. Hmmm... Fine, drop it.

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/for-in-op...
File test/mjsunit/for-in-opt.js (right):

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/for-in-op...
test/mjsunit/for-in-opt.js:134: if (x) %ScheduleBreak()
nit: why drop the semicolons?

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/harmony/p...
File test/mjsunit/harmony/proxies-for.js (right):

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-for.js:139: ownKeys() { return []; },
whaaa, fancy ES6 syntax eh?

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-for.js:160: // The [[Has]] trap has no influence on
which are enumerable properties are
nit: s/which are/which/

[neis, 2016-03-18 14:30:02]

https://codereview.chromium.org/1748923003/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1748923003/diff/160001/src/objects.cc#newcode...
src/objects.cc:8538: if (!iter.AdvanceFollowingProxiesIgnoringAccessChecks()) {
Can you explain the "IgnoringAccessChecks" part of this?

[Camillo Bruni, 2016-03-18 17:46:34]

https://codereview.chromium.org/1748923003/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1748923003/diff/160001/src/objects.cc#newcode...
src/objects.cc:8538: if (!iter.AdvanceFollowingProxiesIgnoringAccessChecks()) {
On 2016/03/18 at 14:30:02, neis wrote:
> Can you explain the "IgnoringAccessChecks" part of this?

Will add a comment:

Since we now have to walk up the proto-chain through the proxyies I used
AdvanceFollowingProxies which had the downside that it would stop the lookup
once we had no longer access to the current object.
However, in the case of keys, you have to switch to the weird ALL_CAN_READ case
where you only see the default properties of the GlobalObject (that's my fuzzy
recollection... don't remember where the spec for this is).

https://codereview.chromium.org/1748923003/diff/160001/src/runtime/runtime-fo...
File src/runtime/runtime-forin.cc (right):

https://codereview.chromium.org/1748923003/diff/160001/src/runtime/runtime-fo...
src/runtime/runtime-forin.cc:39: // This is a slight modifcation of
JSReceiver::HasProperty, dealing with
On 2016/03/18 at 14:12:00, Jakob wrote:
> nit: missng 'i' ;-)

shorter is better, no ;)

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/for-in-op...
File test/mjsunit/for-in-opt.js (left):

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/for-in-op...
test/mjsunit/for-in-opt.js:94: var handler2 = {
On 2016/03/18 at 14:12:00, Jakob wrote:
> That's weird. This careful setup of handler2/proxy2/o2 looks a lot like they
should be used somewhere. Maybe in the f4(o, p) call in line 114? But then the
expectations in check_f4 wouldn't make sense. Hmmm... Fine, drop it.

yeah, this confused me more that it should have...

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/for-in-op...
File test/mjsunit/for-in-opt.js (right):

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/for-in-op...
test/mjsunit/for-in-opt.js:134: if (x) %ScheduleBreak()
On 2016/03/18 at 14:12:00, Jakob wrote:
> nit: why drop the semicolons?

random... will re-add ;)

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/harmony/p...
File test/mjsunit/harmony/proxies-for.js (right):

https://codereview.chromium.org/1748923003/diff/160001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-for.js:139: ownKeys() { return []; },
On 2016/03/18 at 14:12:00, Jakob wrote:
> whaaa, fancy ES6 syntax eh?

if we suffered so much implementing we better make use of it!

[Camillo Bruni, 2016-03-22 18:02:25]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-22 18:02:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748923003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748923003/180001

[commit-bot: I haz the power, 2016-03-22 18:03:43]

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

[commit-bot: I haz the power, 2016-03-22 18:03:44]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/3340)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[Camillo Bruni, 2016-03-23 08:00:29]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-03-23 08:00:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1748923003/#ps200001
(title: "merge with master")

[Camillo Bruni, 2016-03-23 08:00:30]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-03-23 08:00:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1748923003/#ps200001
(title: "merge with master")

[commit-bot: I haz the power, 2016-03-23 08:00:38]

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

[commit-bot: I haz the power, 2016-03-23 08:26:45]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-23 08:37:12]

Description was changed from

==========
[proxies] use [[GetPrototypeOf]] trap in for-in key accumulation

With the recent spec change removing the [[Enumerate]] internal method, we now
have to walk the complete prototype chain. This implies that we call the
[[GetPrototypeOf]] trap on proxies.

As a secondary change we now trigger the [[GetOwnProperty]] trap for the for-in
filter step to see whether the properties are still enumerable. Before we did
this
in the key-accumulation phase. This way we slightly reduce the number of traps
invoked. Whilst this is not ideal, it comes closer to the Spec's example
implementation.

BUG=v8:1543, v8:4768
LOG=n
==========

to

==========
[proxies] use [[GetPrototypeOf]] trap in for-in key accumulation

With the recent spec change removing the [[Enumerate]] internal method, we now
have to walk the complete prototype chain. This implies that we call the
[[GetPrototypeOf]] trap on proxies.

As a secondary change we now trigger the [[GetOwnProperty]] trap for the for-in
filter step to see whether the properties are still enumerable. Before we did
this
in the key-accumulation phase. This way we slightly reduce the number of traps
invoked. Whilst this is not ideal, it comes closer to the Spec's example
implementation.

BUG=v8:1543, v8:4768
LOG=n

Committed: https://crrev.com/2efc1381313248352630b05f6d48064badfe7671
Cr-Commit-Position: refs/heads/master@{#35017}
==========

[commit-bot: I haz the power, 2016-03-23 08:37:14]

Patchset 11 (id:??) landed as
https://crrev.com/2efc1381313248352630b05f6d48064badfe7671
Cr-Commit-Position: refs/heads/master@{#35017}