Do not enter contexts implicitly

Do not enter contexts implicitly

Blink uses Isolate::GetEnteredContext() to implement HTML's "entry
context" concept, and thus depends on it not being changed except
explicitly (by Blink.) To support this, stop entering contexts
implicitly in all external API entry points; rather just set the
context as current. The only thing that changes the entered context
is now Context::Enter()/Context::Exit() (and Context::Scope.)

BUG=v8:6307
Review-Url: https://codereview.chromium.org/2862483003
Cr-Commit-Position: refs/heads/master@{#45064}
Committed: https://chromium.googlesource.com/v8/v8/+/40d01184a18a599392691c0eb931720628a44e80

[Jens Widell, 2017-05-03 08:11:10]

jl@opera.com changed reviewers:
+ jochen@chromium.org

[Jens Widell, 2017-05-03 08:11:11]

PTAL

[jochen (gone - plz use gerrit), 2017-05-03 10:37:39]

what about all the call-sites of Context::Enter/Exit (and ::Scope) inside v8?
will you remove them as well?

Is it possible to add a test for this?

[Jens Widell, 2017-05-03 11:09:26]

On 2017/05/03 at 10:37:39, jochen wrote:
> what about all the call-sites of Context::Enter/Exit (and ::Scope) inside v8?
will you remove them as well?

I don't think there are very many, if you exclude d8 and test/. There are a
bunch in src/inspector/, some of which I've added recently, but those are good;
that code seems to run "outside V8" and calls in through the API, and thus has a
responsibility to enter a context first.

So ScriptCompiler::Compile() in src/api.cc is really the only one that looks
potentially out of place, and it seems harmless. All calls with an entered
context is UnboundScript::BindToCurrentContext(). I suppose we could update only
the current context there, and not the entered context, but it hardly seems to
matter. WDYT?

> Is it possible to add a test for this?

Yes, that should be possible. Sorry about not doing that already.

I guess test/cctest/test-api.cc might be a reasonable place for such a test?
Could write a test that enters one context and then triggers a property accessor
and/or calls a function with a different context and then checks that the
entered context and current context are correct in the access/function callback.

[Jens Widell, 2017-05-03 13:15:56]

On 2017/05/03 11:09:26, Jens Widell wrote:
> On 2017/05/03 at 10:37:39, jochen wrote:
> > Is it possible to add a test for this?
> 
> Yes, that should be possible. Sorry about not doing that already.
> 
> I guess test/cctest/test-api.cc might be a reasonable place for such a test?
> Could write a test that enters one context and then triggers a property
accessor
> and/or calls a function with a different context and then checks that the
> entered context and current context are correct in the access/function
callback.

Test added.

[jochen (gone - plz use gerrit), 2017-05-03 13:18:27]

lgtm

[Jens Widell, 2017-05-03 13:20:18]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2017-05-03 13:20: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, 2017-05-03 13:26:36]

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

[commit-bot: I haz the power, 2017-05-03 13:26:37]

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

[Jens Widell, 2017-05-03 13:30:43]

The CQ bit was checked by jl@opera.com

[Jens Widell, 2017-05-03 13:30:44]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2862483003/#ps40001
(title: "Use correct NewInstance() overload")

[commit-bot: I haz the power, 2017-05-03 13:30:47]

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-05-03 13:59:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493818243790580,
"parent_rev": "17a0a575a3795141dfccabc688b3fa39f226e186", "commit_rev":
"40d01184a18a599392691c0eb931720628a44e80"}

[commit-bot: I haz the power, 2017-05-03 13:59:40]

Description was changed from

==========
Do not enter contexts implicitly

Blink uses Isolate::GetEnteredContext() to implement HTML's "entry
context" concept, and thus depends on it not being changed except
explicitly (by Blink.) To support this, stop entering contexts
implicitly in all external API entry points; rather just set the
context as current. The only thing that changes the entered context
is now Context::Enter()/Context::Exit() (and Context::Scope.)

BUG=v8:6307
==========

to

==========
Do not enter contexts implicitly

Blink uses Isolate::GetEnteredContext() to implement HTML's "entry
context" concept, and thus depends on it not being changed except
explicitly (by Blink.) To support this, stop entering contexts
implicitly in all external API entry points; rather just set the
context as current. The only thing that changes the entered context
is now Context::Enter()/Context::Exit() (and Context::Scope.)

BUG=v8:6307

Review-Url: https://codereview.chromium.org/2862483003
Cr-Commit-Position: refs/heads/master@{#45064}
Committed:
https://chromium.googlesource.com/v8/v8/+/40d01184a18a599392691c0eb931720628a...
==========

[commit-bot: I haz the power, 2017-05-03 13:59:41]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/40d01184a18a599392691c0eb931720628a...