Allow global prototype to be a Proxy

Allow global prototype to be a Proxy

There was a security issue from allowing WindowProxy to have its prototype mutated in https://bugs.chromium.org/p/chromium/issues/detail?id=399951 . This has since been resolved by making `window`, `location`, and `Object.prototype.__proto__` immutable. This change was fixed in https://bugs.chromium.org/p/v8/issues/detail?id=5149 .

Reverts https://codereview.chromium.org/1529303003

R=littledan@chromium.org
R=verwaest@chromium.org

Review-Url: https://codereview.chromium.org/2671553006
Cr-Commit-Position: refs/heads/master@{#44078}
Committed: https://chromium.googlesource.com/v8/v8/+/b123ee34111f643fec6f7b662912deafd1165e3e

[caitp, 2017-02-03 22:28:44]

caitp@igalia.com changed reviewers:
+ caitp@igalia.com

[caitp, 2017-02-03 22:28:45]

Could you please add a line in the CL description stating that the reason for
deleting these lines of code, is that they're apparently obsoleted by the new
frozen/immutable prototypes thing?

[Bradley Farias, 2017-02-03 22:38:52]

Description was changed from

==========
Allow global prototype to be a Proxy

Reverts https://codereview.chromium.org/1529303003 now that
https://bugs.chromium.org/p/v8/issues/detail?id=5149 and
https://bugs.chromium.org/p/chromium/issues/detail?id=399951 are resolved

R=littledan@chromium.org
R=verwaest@chromium.org
BUG=chromium:399951
==========

to

==========
Allow global prototype to be a Proxy

There was a security issue from allowing WindowProxy to have its prototype
mutated in https://bugs.chromium.org/p/chromium/issues/detail?id=399951 . This
has since been resolved by making `window`, `location`, and
`Object.prototype.__proto__` immutable. This change was fixed in
https://bugs.chromium.org/p/v8/issues/detail?id=5149 .

Reverts https://codereview.chromium.org/1529303003

R=littledan@chromium.org
R=verwaest@chromium.org
==========

[caitp, 2017-02-03 22:41:31]

https://codereview.chromium.org/2671553006/diff/1/test/mjsunit/es6/global-pro...
File test/mjsunit/es6/global-proto-proxy.js (right):

https://codereview.chromium.org/2671553006/diff/1/test/mjsunit/es6/global-pro...
test/mjsunit/es6/global-proto-proxy.js:26: // OF THIS SOFTWARE, EVEN IF ADVISED
OF THE POSSIBILITY OF SUCH DAMAGE.
The license header can be shrunk down to just

```
// Copyright 2018 the V8 project authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
```

https://codereview.chromium.org/2671553006/diff/1/test/mjsunit/es6/global-pro...
test/mjsunit/es6/global-proto-proxy.js:54: Object.setPrototypeOf(global, proxy);
Shouldn't this throw due to https://github.com/heycam/webidl/pull/133 ? Or does
d8's global not share the immutable prototype behaviour

https://codereview.chromium.org/2671553006/diff/1/test/mjsunit/es6/global-pro...
test/mjsunit/es6/global-proto-proxy.js:56: assertEquals(true, calledGet);
Nit: assertTrue() is usually recommended over assertEquals(true, ...), though in
practice they're the same thing.

[Bradley Farias, 2017-02-03 22:50:20]

On 2017/02/03 22:41:31, caitp wrote:
>
https://codereview.chromium.org/2671553006/diff/1/test/mjsunit/es6/global-pro...
> File test/mjsunit/es6/global-proto-proxy.js (right):
> 
>
https://codereview.chromium.org/2671553006/diff/1/test/mjsunit/es6/global-pro...
> test/mjsunit/es6/global-proto-proxy.js:26: // OF THIS SOFTWARE, EVEN IF
ADVISED
> OF THE POSSIBILITY OF SUCH DAMAGE.
> The license header can be shrunk down to just
> 
> ```
> // Copyright 2018 the V8 project authors. All rights reserved.
> // Use of this source code is governed by a BSD-style license that can be
> // found in the LICENSE file.
> ```

Fixed to 2017

> 
>
https://codereview.chromium.org/2671553006/diff/1/test/mjsunit/es6/global-pro...
> test/mjsunit/es6/global-proto-proxy.js:54: Object.setPrototypeOf(global,
proxy);
> Shouldn't this throw due to https://github.com/heycam/webidl/pull/133 ? Or
does
> d8's global not share the immutable prototype behaviour

Raw JS Global Environment Record Objects are not immutable, but
Object.prototype.__proto__ is. `window`, etc. is enforced on WHATWG not JS with
https://html.spec.whatwg.org/multipage/browsers.html#windowproxy-setprototypeof
and https://html.spec.whatwg.org/multipage/browsers.html#location-setprototypeof

> 
>
https://codereview.chromium.org/2671553006/diff/1/test/mjsunit/es6/global-pro...
> test/mjsunit/es6/global-proto-proxy.js:56: assertEquals(true, calledGet);
> Nit: assertTrue() is usually recommended over assertEquals(true, ...), though
in
> practice they're the same thing.

Fixed

[Dan Ehrenberg, 2017-02-05 10:25:34]

Thanks for coming back and revisiting this! A couple optional nits:

- Could you word-wrap the description to 80 columns?
- This should be visible even in the web's immutable-global-prototype world of
Reflect.get; maybe that's worth a test.
- The latter could even be a test262 test! And these can be developed within V8
patches as described in
https://docs.google.com/document/d/16bj7AIDgZLv4WOsUEzQ5NzcEN9_xo095e88Pz8FC5...
if you're interested.

[Toon Verwaest, 2017-02-08 16:47:52]

https://codereview.chromium.org/2671553006/diff/40001/src/objects.cc
File src/objects.cc (left):

https://codereview.chromium.org/2671553006/diff/40001/src/objects.cc#oldcode1083
src/objects.cc:1083:
NewTypeError(MessageTemplate::kReadGlobalReferenceThroughProxy, name),
What about removing the message too?

[Bradley Farias, 2017-03-16 19:24:46]

On 2017/02/08 16:47:52, Toon Verwaest wrote:
> https://codereview.chromium.org/2671553006/diff/40001/src/objects.cc
> File src/objects.cc (left):
> 
>
https://codereview.chromium.org/2671553006/diff/40001/src/objects.cc#oldcode1083
> src/objects.cc:1083:
> NewTypeError(MessageTemplate::kReadGlobalReferenceThroughProxy, name),
> What about removing the message too?

Should be fixed.

[Toon Verwaest, 2017-03-17 13:31:10]

lgtm, thanks!

[Bradley Farias, 2017-03-21 17:19:06]

The CQ bit was checked by bradley.meck@gmail.com

[commit-bot: I haz the power, 2017-03-21 17:19:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 17:37:41]

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

[commit-bot: I haz the power, 2017-03-21 17:37:42]

Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Bradley Farias, 2017-03-22 21:43:26]

On 2017/03/21 17:37:42, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
>   v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

Toon Verwaest can you double check updated bytecode looks sane?

[caitp, 2017-03-23 18:00:52]

caitp@igalia.com changed reviewers:
+ bradley.meck@gmail.com
- Bradley.Meck@gmail.com

[caitp, 2017-03-23 18:00:53]

ditto for the other bytecode expectations changes.

https://codereview.chromium.org/2671553006/diff/80001/test/cctest/interpreter...
File test/cctest/interpreter/bytecode_expectations/ForAwaitOf.golden (right):

https://codereview.chromium.org/2671553006/diff/80001/test/cctest/interpreter...
test/cctest/interpreter/bytecode_expectations/ForAwaitOf.golden:220: B(Wide),
B(LdaSmi), I16(129),
these are just because the message ID changed, looks fine

[Bradley Farias, 2017-03-23 18:08:35]

The CQ bit was checked by bradley.meck@gmail.com

[Bradley Farias, 2017-03-23 18:08:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2671553006/#ps80001
(title: "update bytecode")

[commit-bot: I haz the power, 2017-03-23 18:08:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 18:33:38]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490292515061120,
"parent_rev": "e741957a6f26cbc2dce71b001518ce35dfab2d97", "commit_rev":
"b123ee34111f643fec6f7b662912deafd1165e3e"}

[commit-bot: I haz the power, 2017-03-23 18:33:44]

Description was changed from

==========
Allow global prototype to be a Proxy

There was a security issue from allowing WindowProxy to have its prototype
mutated in https://bugs.chromium.org/p/chromium/issues/detail?id=399951 . This
has since been resolved by making `window`, `location`, and
`Object.prototype.__proto__` immutable. This change was fixed in
https://bugs.chromium.org/p/v8/issues/detail?id=5149 .

Reverts https://codereview.chromium.org/1529303003

R=littledan@chromium.org
R=verwaest@chromium.org
==========

to

==========
Allow global prototype to be a Proxy

There was a security issue from allowing WindowProxy to have its prototype
mutated in https://bugs.chromium.org/p/chromium/issues/detail?id=399951 . This
has since been resolved by making `window`, `location`, and
`Object.prototype.__proto__` immutable. This change was fixed in
https://bugs.chromium.org/p/v8/issues/detail?id=5149 .

Reverts https://codereview.chromium.org/1529303003

R=littledan@chromium.org
R=verwaest@chromium.org

Review-Url: https://codereview.chromium.org/2671553006
Cr-Commit-Position: refs/heads/master@{#44078}
Committed:
https://chromium.googlesource.com/v8/v8/+/b123ee34111f643fec6f7b662912deafd11...
==========

[commit-bot: I haz the power, 2017-03-23 18:33:45]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/b123ee34111f643fec6f7b662912deafd11...