Ensure that the current isolate is initialized in the API function Context::GetEntered.

Ensure that the current isolate is initialized in the API function Context::GetEntered.

r8833 introduced a regression in our API semantics, showing up in e.g.
Chrome 12, which is fixed by this patch.
Committed: http://code.google.com/p/v8/source/detail?r=8987

[Vitaly Repeshko, 2011-08-19 14:15:38]

LGTM and thanks! ... with one suggestion.

http://codereview.chromium.org/7686005/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/7686005/diff/1/src/api.cc#newcode4121
src/api.cc:4121: if (!EnsureInitializedForIsolate(isolate,
"v8::Context::GetEntered()")) {
While this will work, we could be a bit fancier here. If the isolate is not
initialized, we can return an empty handle without trying to access the handle
scope implementer.

[Sven Panne, 2011-08-19 15:01:11]

http://codereview.chromium.org/7686005/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/7686005/diff/1/src/api.cc#newcode4121
src/api.cc:4121: if (!EnsureInitializedForIsolate(isolate,
"v8::Context::GetEntered()")) {
On 2011/08/19 14:15:38, Vitaly Repeshko wrote:
> While this will work, we could be a bit fancier here. If the isolate is not
> initialized, we can return an empty handle without trying to access the handle
> scope implementer.

The underlying problem is that r8833 changed our externally visible API
semantics in very subtle ways, which already caused some breakage, e.g. Chrome
12 not working or http://code.google.com/p/v8/issues/detail?id=1597. I think
that ensuring initialization here brings us closer back to our old semantics, so
I am a bit reluctant to be "fancy" here.

Another remark: What about other API functions in this file? I have the slightly
uneasy feeling that similar problems will pop up later. This CL only fixes
things enough that Chrome 12 works, but e.g. issue 1597 might imply that there
is more to do...

[Vitaly Repeshko, 2011-08-19 15:17:38]

http://codereview.chromium.org/7686005/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/7686005/diff/1/src/api.cc#newcode4121
src/api.cc:4121: if (!EnsureInitializedForIsolate(isolate,
"v8::Context::GetEntered()")) {
On 2011/08/19 15:01:11, Sven wrote:
> On 2011/08/19 14:15:38, Vitaly Repeshko wrote:
> > While this will work, we could be a bit fancier here. If the isolate is not
> > initialized, we can return an empty handle without trying to access the
handle
> > scope implementer.
> 
> The underlying problem is that r8833 changed our externally visible API
> semantics in very subtle ways, which already caused some breakage, e.g. Chrome
> 12 not working or http://code.google.com/p/v8/issues/detail?id=1597. I think
> that ensuring initialization here brings us closer back to our old semantics,
so
> I am a bit reluctant to be "fancy" here.
> 
> Another remark: What about other API functions in this file? I have the
slightly
> uneasy feeling that similar problems will pop up later. This CL only fixes
> things enough that Chrome 12 works, but e.g. issue 1597 might imply that there
> is more to do...

Well, the real underlying issue here is that our old (pre-isolate API) allowed
lazy VM initialization by an undocumented set of "entry" function that must be
used before it "makes sense" to touch V8 objects. (This is exactly the
difference between IsDeadCheck and EnsureInitialized.) Because old V8 state was
mostly static variables, it means even if an entry function is not called,
accessors like GetEntered would still work fine provided they happened to be
statically initialized to sane values. So Chromium actually violated the old
implicit contract here by calling an accessor before an entry function. With
isolates we tried to mimic the old behavior, but because the static variables
are gone it's a bit harder.

I think what you have now is totally fine as a bug fix. But given that this is
an accessor function that used to work (return an empty handle) without
initializing the VM, it makes sense to fix it so that it does exactly that.