Let SetEntropySource() fail if called after V8::Initialize().

Let SetEntropySource() fail if called after V8::Initialize().

BUG=v8:2905
R=dslomov@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16889

[Benedikt Meurer, 2013-09-23 10:09:07]

PTAL

[Dmitry Lomov (no reviews), 2013-09-23 11:24:15]

https://codereview.chromium.org/24357002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/24357002/diff/1/include/v8.h#newcode4518
include/v8.h:4518: *   calling any other V8-related function.
This is vague; the true statement is that SetEntropySource should be called
prior to V8::Intitialize.

Generally, we have a bunch of global parameter setters (V8::SetEntropySource,
V8::SetArrayBufferAllocator, V8::SetReturnAddressLocationResolver etc) that all
need to be called before V8::Initialize, but crucially do not depend on each
other (otherwise it will be a nightmare to call correctly)

https://codereview.chromium.org/24357002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/24357002/diff/1/src/api.cc#newcode5189
src/api.cc:5189: if (isolate != NULL && isolate->IsInitialized())
Looks like this should be isolate == NULL || !isolate->IsInitialized().

https://codereview.chromium.org/24357002/diff/1/src/api.cc#newcode5190
src/api.cc:5190: return false;
I think just returning false will be ignored (also completely useless for user
program - if the function returns false, there is no way for user program to
recover). I think it will make much more sense to assert this (use ApiCheck
function)

[Dmitry Lomov (no reviews), 2013-09-23 11:25:54]

https://codereview.chromium.org/24357002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/24357002/diff/1/src/api.cc#newcode5189
src/api.cc:5189: if (isolate != NULL && isolate->IsInitialized())
On 2013/09/23 11:24:15, Dmitry Lomov (chromium) wrote:
> Looks like this should be isolate == NULL || !isolate->IsInitialized().

Nevermind.

[Benedikt Meurer, 2013-09-23 11:36:12]

PTAL

[Benedikt Meurer, 2013-09-23 11:36:41]

https://codereview.chromium.org/24357002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/24357002/diff/1/include/v8.h#newcode4518
include/v8.h:4518: *   calling any other V8-related function.
On 2013/09/23 11:24:15, Dmitry Lomov (chromium) wrote:
> This is vague; the true statement is that SetEntropySource should be called
> prior to V8::Intitialize.
> 
> Generally, we have a bunch of global parameter setters (V8::SetEntropySource,
> V8::SetArrayBufferAllocator, V8::SetReturnAddressLocationResolver etc) that
all
> need to be called before V8::Initialize, but crucially do not depend on each
> other (otherwise it will be a nightmare to call correctly)

Done.

https://codereview.chromium.org/24357002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/24357002/diff/1/src/api.cc#newcode5190
src/api.cc:5190: return false;
On 2013/09/23 11:24:15, Dmitry Lomov (chromium) wrote:
> I think just returning false will be ignored (also completely useless for user
> program - if the function returns false, there is no way for user program to
> recover). I think it will make much more sense to assert this (use ApiCheck
> function)

Done.

[Dmitry Lomov (no reviews), 2013-09-23 12:24:54]

LGTM

[Benedikt Meurer, 2013-09-23 14:11:44]

Committed patchset #2 manually as r16889 (presubmit successful).

[Michael Starzinger, 2013-09-23 14:51:31]

Drive-by comment.

https://codereview.chromium.org/24357002/diff/8001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/24357002/diff/8001/src/api.cc#newcode5181
src/api.cc:5181: i::Isolate* isolate = i::Isolate::UncheckedCurrent();
We try to get rid of Isolate::Current, please try not to add new call-site to
this function.

[Benedikt Meurer, 2013-09-23 20:25:51]

https://codereview.chromium.org/24357002/diff/8001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/24357002/diff/8001/src/api.cc#newcode5181
src/api.cc:5181: i::Isolate* isolate = i::Isolate::UncheckedCurrent();
On 2013/09/23 14:51:31, Michael Starzinger wrote:
> We try to get rid of Isolate::Current, please try not to add new call-site to
> this function.

I know, but there's currently no other way to perform this check. We're planning
to add one, but that'll take some more cleanup time.