Re: Issue 2368: LiveEdit crashes when new object/array literal is added (issue 11191039)

[yan...@chromium.org]

LGTM if comments are addressed.


http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc
File src/liveedit.cc (right):

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1056
src/liveedit.cc:1056: FindJSFunctions(shared_info, result);
Running twice over the heap seems really inefficient.

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1166
src/liveedit.cc:1166: // adjust array size.
Wouldn't adjusting array size imply cleaning cached values as well?
Please add that to the comment.

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1176
src/liveedit.cc:1176: for (int i = 0; i < function_instances->length();
i++) {
Instead of looping over function instances that you have to tediously
gather, couldn't you iterate over the heap here and skip every object
that is not a function instance of shared_info? That way you wouldn't
need to allocate a new FixedArray and wouldn't need to iterate the heap
twice to find out that FixedArray's size.

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1182
src/liveedit.cc:1182: old_literals->set(j,
isolate->heap()->undefined_value());
I guess you could use FixedArray->set_undefined here.

http://codereview.chromium.org/11191039/diff/6001/src/liveedit.cc#newcode1200
src/liveedit.cc:1200: }
I assume that we are not dealing with optimized code here, right? In
that case only, clearing the literals array is safe (according to
mstarzinger@).

http://codereview.chromium.org/11191039/diff/6001/test/mjsunit/debug-liveedit-literals.js
File test/mjsunit/debug-liveedit-literals.js (right):

http://codereview.chromium.org/11191039/diff/6001/test/mjsunit/debug-liveedit-literals.js#newcode48
test/mjsunit/debug-liveedit-literals.js:48:
Debug.LiveEdit.TestApi.ApplySingleChunkPatch(script, patch_pos,
old_expression.length, new_expression, change_log);
Please keep the 80-char limit.

http://codereview.chromium.org/11191039/diff/6001/test/mjsunit/debug-liveedit-literals.js#newcode75
test/mjsunit/debug-liveedit-literals.js:75:
Test("(/.A./i).exec('Cat')[0]", "{c:'Capybara'}.c");
the test for regexp -> no literals seems to be missing.

http://codereview.chromium.org/11191039/