Fix call stack sampling for the case when native callback invokes JS function

Fix call stack sampling for the case when native callback invokes JS function

The SafeStackFrameIterator used by CPU profiler checked if Isolate::c_entry_fp is null and if it is not it would think that the control flow currently is in some native code. This assumption is wrong because the native code could have called a JS function but JSEntryStub would not reset c_entry_fp to NULL in that case. This CL adds a check in SafeStackFrameIterator::IsValidTop for the case when there is a JAVA_SCRIPT frame on top of EXIT frame.

Also this CL changes ExternalCallbackScope behavior to provide access to the whole stack of the scope objects instead of only top one. This allowed to provide exact callback names for those EXIT frames where external callbacks are called. Without this change it was possible only for the top most native call.

BUG=None
R=loislo@chromium.org, yangguo@chromium.org

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

[loislo, 2013-07-22 14:18:54]

lgtm

https://codereview.chromium.org/19775017/diff/5001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/19775017/diff/5001/src/frames.cc#newcode262
src/frames.cc:262: if (!done()) Advance();
if (frame_ == NULL) return;

Advance();

if (frame_ != NULL &&
    !frame_->is_exit() &&
    external_callback_scope_ &&
    external_callback_scope_->scope_address() < frame_->fp()) {
  // Skip top ExternalCallbackScope if we already advanced to a JS frame
  // under it. Sampler will anyways take this top external callback.
  external_callback_scope_ = external_callback_scope_->previous();
}

[yurys, 2013-07-22 14:24:08]

https://codereview.chromium.org/19775017/diff/5001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/19775017/diff/5001/src/frames.cc#newcode262
src/frames.cc:262: if (!done()) Advance();
On 2013/07/22 14:18:54, loislo wrote:
> if (frame_ == NULL) return;
> 
> Advance();
> 
> if (frame_ != NULL &&
>     !frame_->is_exit() &&
>     external_callback_scope_ &&
>     external_callback_scope_->scope_address() < frame_->fp()) {
>   // Skip top ExternalCallbackScope if we already advanced to a JS frame
>   // under it. Sampler will anyways take this top external callback.
>   external_callback_scope_ = external_callback_scope_->previous();
> }

Done.

[yurys, 2013-07-22 14:24:24]

Yang, can you do OWNER review please?

[Yang, 2013-07-23 12:15:12]

LGTM. One comment.

https://codereview.chromium.org/19775017/diff/7002/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/19775017/diff/7002/src/frames.cc#newcode369
src/frames.cc:369: external_callback_scope_ =
external_callback_scope_->previous();
If I understood correctly, the previous external callback scope should then have
a higher address than frame_->fp(), right? Could we assert this?

[yurys, 2013-07-23 12:28:22]

https://codereview.chromium.org/19775017/diff/7002/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/19775017/diff/7002/src/frames.cc#newcode369
src/frames.cc:369: external_callback_scope_ =
external_callback_scope_->previous();
On 2013/07/23 12:15:12, Yang wrote:
> If I understood correctly, the previous external callback scope should then
have
> a higher address than frame_->fp(), right? Could we assert this?

That's correct. In theory one can allocate two ExternalCallbackScopes in a row
but this doesn't make sense. Added assert.

[Yang, 2013-07-23 12:34:46]

On 2013/07/23 12:28:22, Yury Semikhatsky wrote:
> https://codereview.chromium.org/19775017/diff/7002/src/frames.cc
> File src/frames.cc (right):
> 
> https://codereview.chromium.org/19775017/diff/7002/src/frames.cc#newcode369
> src/frames.cc:369: external_callback_scope_ =
> external_callback_scope_->previous();
> On 2013/07/23 12:15:12, Yang wrote:
> > If I understood correctly, the previous external callback scope should then
> have
> > a higher address than frame_->fp(), right? Could we assert this?
> 
> That's correct. In theory one can allocate two ExternalCallbackScopes in a row
> but this doesn't make sense. Added assert.

lgtm.

[yurys, 2013-07-23 15:01:49]

Committed patchset #7 manually as r15832 (presubmit successful).