Guard against undefined fields in global context.

Guard against undefined fields in global context.

BUG=v8:1860
TEST=
R=vegorov@chromium.org

Committed: http://code.google.com/p/v8/source/detail?r=10243

[ulan, 2011-12-12 19:11:16]

Thank you for hunting down the bug.

There are other places that access the global contest list, but they (including
visitors) seem to be safe.

http://codereview.chromium.org/8917014/diff/3/src/deoptimizer.cc
File src/deoptimizer.cc (right):

http://codereview.chromium.org/8917014/diff/3/src/deoptimizer.cc#newcode269
src/deoptimizer.cc:269: Object* global =
Context::cast(context)->get(Context::GLOBAL_INDEX);
Do we need check here? Is global object always defined?

[Vyacheslav Egorov (Chromium), 2011-12-13 12:26:20]

http://codereview.chromium.org/8917014/diff/3/src/deoptimizer.cc
File src/deoptimizer.cc (right):

http://codereview.chromium.org/8917014/diff/3/src/deoptimizer.cc#newcode269
src/deoptimizer.cc:269: Object* global =
Context::cast(context)->get(Context::GLOBAL_INDEX);
On 2011/12/12 19:11:16, ulan wrote:
> Do we need check here? Is global object always defined?

I think we need it. Judging from the control flow inside Genesis::Genesis we can
get GC before set_global was called. 

Please add a comment about this here (so people reading the code will know that
GC can happen when context is in the list but not yet fully initialized).

http://codereview.chromium.org/8917014/diff/3/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/8917014/diff/3/src/heap.cc#newcode647
src/heap.cc:647: Context::cast(context)->jsfunction_result_caches();
Should not you guard here instead of guarding below? 

(set_jsfunction_result_caches is called once the cache is fully constructed).

also please comment why we need to guard against undefined.

http://codereview.chromium.org/8917014/diff/3/src/heap.cc#newcode671
src/heap.cc:671: int index = Context::NORMALIZED_MAP_CACHE_INDEX;
I don't think we need separate variable just for index.

http://codereview.chromium.org/8917014/diff/3/src/heap.cc#newcode673
src/heap.cc:673: if (!cache->IsUndefined()) {
please comment why we need to guard against partial initialized objects.

http://codereview.chromium.org/8917014/diff/3/src/incremental-marking.cc
File src/incremental-marking.cc (right):

http://codereview.chromium.org/8917014/diff/3/src/incremental-marking.cc#newc...
src/incremental-marking.cc:680: int index = Context::NORMALIZED_MAP_CACHE_INDEX;
I don't think we need separate index variable.

http://codereview.chromium.org/8917014/diff/3/src/incremental-marking.cc#newc...
src/incremental-marking.cc:682: if (!cache_or_undefined->IsUndefined()) {
Please comment why can it be undefined.

http://codereview.chromium.org/8917014/diff/3/src/incremental-marking.cc#newc...
src/incremental-marking.cc:683: NormalizedMapCache* cache =
NormalizedMapCache::cast(cache_or_undefined);
you don't need a separate cache variable. just make cache_or_undefined cache.
(make it HeapObject as it is guaranteed to be one).

[ulan, 2011-12-13 13:02:57]

Uploaded new patch set, please take another look.

http://codereview.chromium.org/8917014/diff/3/src/deoptimizer.cc
File src/deoptimizer.cc (right):

http://codereview.chromium.org/8917014/diff/3/src/deoptimizer.cc#newcode269
src/deoptimizer.cc:269: Object* global =
Context::cast(context)->get(Context::GLOBAL_INDEX);
On 2011/12/13 12:26:20, Vyacheslav Egorov wrote:
> On 2011/12/12 19:11:16, ulan wrote:
> > Do we need check here? Is global object always defined?
> 
> I think we need it. Judging from the control flow inside Genesis::Genesis we
can
> get GC before set_global was called. 
> 
> Please add a comment about this here (so people reading the code will know
that
> GC can happen when context is in the list but not yet fully initialized).

Done.

http://codereview.chromium.org/8917014/diff/3/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/8917014/diff/3/src/heap.cc#newcode647
src/heap.cc:647: Context::cast(context)->jsfunction_result_caches();
On 2011/12/13 12:26:20, Vyacheslav Egorov wrote:
> Should not you guard here instead of guarding below? 
> 
> (set_jsfunction_result_caches is called once the cache is fully constructed).
> 
> also please comment why we need to guard against undefined.

Done.

http://codereview.chromium.org/8917014/diff/3/src/heap.cc#newcode671
src/heap.cc:671: int index = Context::NORMALIZED_MAP_CACHE_INDEX;
On 2011/12/13 12:26:20, Vyacheslav Egorov wrote:
> I don't think we need separate variable just for index.

Done.

http://codereview.chromium.org/8917014/diff/3/src/heap.cc#newcode673
src/heap.cc:673: if (!cache->IsUndefined()) {
On 2011/12/13 12:26:20, Vyacheslav Egorov wrote:
> please comment why we need to guard against partial initialized objects.

Done.

http://codereview.chromium.org/8917014/diff/3/src/incremental-marking.cc
File src/incremental-marking.cc (right):

http://codereview.chromium.org/8917014/diff/3/src/incremental-marking.cc#newc...
src/incremental-marking.cc:680: int index = Context::NORMALIZED_MAP_CACHE_INDEX;
On 2011/12/13 12:26:20, Vyacheslav Egorov wrote:
> I don't think we need separate index variable.

Done.

http://codereview.chromium.org/8917014/diff/3/src/incremental-marking.cc#newc...
src/incremental-marking.cc:682: if (!cache_or_undefined->IsUndefined()) {
On 2011/12/13 12:26:20, Vyacheslav Egorov wrote:
> Please comment why can it be undefined.

Done.

http://codereview.chromium.org/8917014/diff/3/src/incremental-marking.cc#newc...
src/incremental-marking.cc:683: NormalizedMapCache* cache =
NormalizedMapCache::cast(cache_or_undefined);
On 2011/12/13 12:26:20, Vyacheslav Egorov wrote:
> you don't need a separate cache variable. just make cache_or_undefined cache.
> (make it HeapObject as it is guaranteed to be one).

Done.

[Vyacheslav Egorov (Chromium), 2011-12-13 13:08:19]

LGTM!