Do security checks in the promise constructor

Do security checks in the promise constructor

Since we only can do limited checks during microtask execution, do the
checks before actually creating a promise

BUG=chromium:658194
R=bmeurer@chromium.org,gsathya@chromium.org

Review-Url: https://codereview.chromium.org/2628863002
Cr-Commit-Position: refs/heads/master@{#42265}
Committed: https://chromium.googlesource.com/v8/v8/+/81c62e070b8656432899fc17b46b882bfcf1d59a

[jochen (gone - plz use gerrit), 2017-01-11 12:35:32]

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

[commit-bot: I haz the power, 2017-01-11 12:35:37]

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

[jochen (gone - plz use gerrit), 2017-01-11 12:50:37]

chromium side here: https://codereview.chromium.org/2626003002

[commit-bot: I haz the power, 2017-01-11 13:15:46]

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

[commit-bot: I haz the power, 2017-01-11 13:15:47]

Dry run: This issue passed the CQ dry run.

[gsathya, 2017-01-11 17:27:31]

https://codereview.chromium.org/2628863002/diff/1/src/builtins/builtins-promi...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2628863002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise.cc:918: Bind(&loop_over_bound_function);
i wonder if its better to duplicate the IsJSFunction check outside this block
and mark this block as deferred

https://codereview.chromium.org/2628863002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise.cc:932: Node* native_context =
LoadNativeContext(context);
this can be an argument to this function, no need to load the native context
twice in the fast path

https://codereview.chromium.org/2628863002/diff/1/src/runtime/runtime-interna...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2628863002/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:496: RUNTIME_FUNCTION(Runtime_CountUsage) {
There is IncrementUseCounter which does the same

[jochen (gone - plz use gerrit), 2017-01-11 18:09:39]

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

[commit-bot: I haz the power, 2017-01-11 18:09:41]

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

[jochen (gone - plz use gerrit), 2017-01-11 18:09:57]

jochen@chromium.org changed reviewers:
+ ishell@chromium.org

[jochen (gone - plz use gerrit), 2017-01-11 18:09:58]

https://codereview.chromium.org/2628863002/diff/1/src/builtins/builtins-promi...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2628863002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise.cc:918: Bind(&loop_over_bound_function);
On 2017/01/11 at 17:27:31, gsathya wrote:
> i wonder if its better to duplicate the IsJSFunction check outside this block
and mark this block as deferred

afaik that shouldn't make that much of a difference as there's not much going on
in the block. Adding ishell@ to double-check

https://codereview.chromium.org/2628863002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise.cc:932: Node* native_context =
LoadNativeContext(context);
On 2017/01/11 at 17:27:31, gsathya wrote:
> this can be an argument to this function, no need to load the native context
twice in the fast path

done

https://codereview.chromium.org/2628863002/diff/1/src/runtime/runtime-interna...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/2628863002/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:496: RUNTIME_FUNCTION(Runtime_CountUsage) {
On 2017/01/11 at 17:27:31, gsathya wrote:
> There is IncrementUseCounter which does the same

good catch, thx

[gsathya, 2017-01-11 18:14:03]

lgtm

[commit-bot: I haz the power, 2017-01-11 18:48:30]

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

[commit-bot: I haz the power, 2017-01-11 18:48:31]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2017-01-12 08:31:11]

nit:

https://codereview.chromium.org/2628863002/diff/1/src/builtins/builtins-promi...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2628863002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise.cc:918: Bind(&loop_over_bound_function);
On 2017/01/11 18:09:57, jochen wrote:
> On 2017/01/11 at 17:27:31, gsathya wrote:
> > i wonder if its better to duplicate the IsJSFunction check outside this
block
> and mark this block as deferred
> 
> afaik that shouldn't make that much of a difference as there's not much going
on
> in the block. Adding ishell@ to double-check

I think it's fine to avoid marking the block as deferred.

https://codereview.chromium.org/2628863002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise.cc:921:
GotoUnless(IsJSBoundFunction(var_executor.value()), &call_runtime);
Here you are loading map and then instance type second time. Please fix this.

[jochen (gone - plz use gerrit), 2017-01-12 09:48:43]

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

[commit-bot: I haz the power, 2017-01-12 09:48:51]

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

[jochen (gone - plz use gerrit), 2017-01-12 09:48:52]

ptal

[Igor Sheludko, 2017-01-12 10:12:58]

lgtm with suggestion:

https://codereview.chromium.org/2628863002/diff/40001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2628863002/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:2979: Node*
CodeStubAssembler::IsJSFunctionInstanceType(Node* instance_type) {
I think it would be nice to have one helper method like
  IsInstanceType(Node* instance_type, InstanceType type)
or even
  InstanceTypeEqual(...)
instead.

And please put it below IsJSReceiverInstanceType().

[commit-bot: I haz the power, 2017-01-12 10:18:55]

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

[commit-bot: I haz the power, 2017-01-12 10:18:56]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-01-12 10:57:47]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-01-12 10:57:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
gsathya@chromium.org, ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2628863002/#ps60001
(title: "updates")

[commit-bot: I haz the power, 2017-01-12 10:58:00]

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

[jochen (gone - plz use gerrit), 2017-01-12 10:58:06]

done

[commit-bot: I haz the power, 2017-01-12 11:33:52]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484218667098360,
"parent_rev": "b8294aaa978e972978b8d82cedf63befad5d7af1", "commit_rev":
"81c62e070b8656432899fc17b46b882bfcf1d59a"}

[commit-bot: I haz the power, 2017-01-12 11:33:56]

Description was changed from

==========
Do security checks in the promise constructor

Since we only can do limited checks during microtask execution, do the
checks before actually creating a promise

BUG=chromium:658194
R=bmeurer@chromium.org,gsathya@chromium.org
==========

to

==========
Do security checks in the promise constructor

Since we only can do limited checks during microtask execution, do the
checks before actually creating a promise

BUG=chromium:658194
R=bmeurer@chromium.org,gsathya@chromium.org

Review-Url: https://codereview.chromium.org/2628863002
Cr-Commit-Position: refs/heads/master@{#42265}
Committed:
https://chromium.googlesource.com/v8/v8/+/81c62e070b8656432899fc17b46b882bfcf...
==========

[commit-bot: I haz the power, 2017-01-12 11:33:59]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/81c62e070b8656432899fc17b46b882bfcf...