Don't touch the prototype chain to get the private script controller.

Don't touch the prototype chain to get the private script controller.

Prior to this patch, private scripts attempted to get the
"privateScriptController" property off the global object without verifying if
the property actually exists on the global. If the property hasn't been set yet,
this operation could descend into the prototype chain and potentially return
a named property from the WindowProperties object, leading to release asserts
and general confusion.

BUG=668552
Committed: https://crrev.com/c093b7a74ddce32dd3b0e0be60f31becc6ce32f9
Cr-Commit-Position: refs/heads/master@{#434627}

[Mariusz Mlynski, 2016-11-25 19:42:55]

Description was changed from

==========
test

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

to

==========
Prior to this patch, private scripts attempted to get the
"privateScriptController" property off the global object without verifying if
the property actually exists on the global. If the property hasn't been set yet,
this operation could descend into the prototype chain and potentially return
a named property from the WindowProperties object, leading to release asserts
and general confusion.

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

[Mariusz Mlynski, 2016-11-25 19:44:19]

Description was changed from

==========
Prior to this patch, private scripts attempted to get the
"privateScriptController" property off the global object without verifying if
the property actually exists on the global. If the property hasn't been set yet,
this operation could descend into the prototype chain and potentially return
a named property from the WindowProperties object, leading to release asserts
and general confusion.

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

to

==========
Don't touch the prototype chain to get the private script controller.

Prior to this patch, private scripts attempted to get the
"privateScriptController" property off the global object without verifying if
the property actually exists on the global. If the property hasn't been set yet,
this operation could descend into the prototype chain and potentially return
a named property from the WindowProperties object, leading to release asserts
and general confusion.

BUG=668552
==========

[haraken, 2016-11-26 01:26:46]

LGTM, thanks for the fix!

[Mariusz Mlynski, 2016-11-26 12:00:41]

marius.mlynski@gmail.com changed reviewers:
+ haraken@chromium.org, jochen@chromium.org, yukishiino@chromium.org

[Mariusz Mlynski, 2016-11-26 12:00:41]

Thanks for the feedback! I've added the test.

[haraken, 2016-11-26 12:06:29]

On 2016/11/26 12:00:41, Mariusz Mlynski wrote:
> Thanks for the feedback! I've added the test.

LGTM!

[Yuki, 2016-11-28 05:13:34]

https://codereview.chromium.org/2529163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/2529163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp:62: if
(v8CallBoolean(global->HasOwnProperty(context, key))) {
nit: v8CallBoolean is obsolete.
Please use V8 APIs directly.

[Mariusz Mlynski, 2016-11-28 07:51:44]

https://codereview.chromium.org/2529163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/2529163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp:62: if
(v8CallBoolean(global->HasOwnProperty(context, key))) {
On 2016/11/28 05:13:34, Yuki wrote:
> nit: v8CallBoolean is obsolete.
> Please use V8 APIs directly.

Done. Btw, there's no deprecation note for v8CallBoolean in V8BindingMacros.h,
so one might be needed to prevent further usage.

[Yuki, 2016-11-28 08:01:29]

LGTM.  Thanks for finding and fixing the issue.

https://codereview.chromium.org/2529163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/2529163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp:62: if
(v8CallBoolean(global->HasOwnProperty(context, key))) {
On 2016/11/28 07:51:44, Mariusz Mlynski wrote:
> On 2016/11/28 05:13:34, Yuki wrote:
> > nit: v8CallBoolean is obsolete.
> > Please use V8 APIs directly.
> 
> Done. Btw, there's no deprecation note for v8CallBoolean in V8BindingMacros.h,
> so one might be needed to prevent further usage.

Acknowledged.

https://codereview.chromium.org/2529163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/2529163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp:62: if
(global->HasOwnProperty(context, key).FromMaybe(false)) {
Could you use FromJust() instead?

When FromMaybe(defaultValue) returns the defaultValue, then an exception must be
thrown by HasOwnProperty().  We don't want that exception to be unhandled. 
Crash is somewhat better than unhandled exceptions.

Since it's unlikely that HasOwnProperty throws an exception, we don't need to
care too much about the crash.

[haraken, 2016-11-28 08:04:57]

https://codereview.chromium.org/2529163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/2529163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp:62: if
(global->HasOwnProperty(context, key).FromMaybe(false)) {
On 2016/11/28 08:01:29, Yuki wrote:
> Could you use FromJust() instead?
> 
> When FromMaybe(defaultValue) returns the defaultValue, then an exception must
be
> thrown by HasOwnProperty().  We don't want that exception to be unhandled. 
> Crash is somewhat better than unhandled exceptions.
> 
> Since it's unlikely that HasOwnProperty throws an exception, we don't need to
> care too much about the crash.

We should use ToChecked() :) The plan is to deprecate all v8Call macros and
confusing V8 APIs like fromMaybe, fromJust etc and replace them with
To/ToLocal/ToChecked/ToLocalChecked consistently.

We should have a better comment about this. I'll add it.

[Mariusz Mlynski, 2016-11-28 08:18:20]

https://codereview.chromium.org/2529163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/2529163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/PrivateScriptRunner.cpp:62: if
(global->HasOwnProperty(context, key).FromMaybe(false)) {
On 2016/11/28 08:04:57, haraken wrote:
> On 2016/11/28 08:01:29, Yuki wrote:
> > Could you use FromJust() instead?
> > 
> > When FromMaybe(defaultValue) returns the defaultValue, then an exception
must
> be
> > thrown by HasOwnProperty().  We don't want that exception to be unhandled. 
> > Crash is somewhat better than unhandled exceptions.
> > 
> > Since it's unlikely that HasOwnProperty throws an exception, we don't need
to
> > care too much about the crash.
> 
> We should use ToChecked() :) The plan is to deprecate all v8Call macros and
> confusing V8 APIs like fromMaybe, fromJust etc and replace them with
> To/ToLocal/ToChecked/ToLocalChecked consistently.
> 
> We should have a better comment about this. I'll add it.

Done.

[jochen (gone - plz use gerrit), 2016-11-28 08:27:00]

lgtm

[Mariusz Mlynski, 2016-11-28 08:38:14]

The CQ bit was checked by marius.mlynski@gmail.com

[Mariusz Mlynski, 2016-11-28 08:38:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2529163002/#ps60001
(title: "Use ToChecked().")

[commit-bot: I haz the power, 2016-11-28 08:38:27]

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, 2016-11-28 10:18:07]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480322294289360,
"parent_rev": "1b9ab9ce536813fbbe730eacfa201be70c02a9f0", "commit_rev":
"bf2ddcd613c2263a56ce41f8cd4e98a8a9bb823a"}

[commit-bot: I haz the power, 2016-11-28 10:18:30]

Description was changed from

==========
Don't touch the prototype chain to get the private script controller.

Prior to this patch, private scripts attempted to get the
"privateScriptController" property off the global object without verifying if
the property actually exists on the global. If the property hasn't been set yet,
this operation could descend into the prototype chain and potentially return
a named property from the WindowProperties object, leading to release asserts
and general confusion.

BUG=668552
==========

to

==========
Don't touch the prototype chain to get the private script controller.

Prior to this patch, private scripts attempted to get the
"privateScriptController" property off the global object without verifying if
the property actually exists on the global. If the property hasn't been set yet,
this operation could descend into the prototype chain and potentially return
a named property from the WindowProperties object, leading to release asserts
and general confusion.

BUG=668552
==========

[commit-bot: I haz the power, 2016-11-28 10:18:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-28 10:20:42]

Description was changed from

==========
Don't touch the prototype chain to get the private script controller.

Prior to this patch, private scripts attempted to get the
"privateScriptController" property off the global object without verifying if
the property actually exists on the global. If the property hasn't been set yet,
this operation could descend into the prototype chain and potentially return
a named property from the WindowProperties object, leading to release asserts
and general confusion.

BUG=668552
==========

to

==========
Don't touch the prototype chain to get the private script controller.

Prior to this patch, private scripts attempted to get the
"privateScriptController" property off the global object without verifying if
the property actually exists on the global. If the property hasn't been set yet,
this operation could descend into the prototype chain and potentially return
a named property from the WindowProperties object, leading to release asserts
and general confusion.

BUG=668552

Committed: https://crrev.com/c093b7a74ddce32dd3b0e0be60f31becc6ce32f9
Cr-Commit-Position: refs/heads/master@{#434627}
==========

[commit-bot: I haz the power, 2016-11-28 10:20:43]

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