Added more checks to SafeStackFrameIterator to prevent crashes when profiling.

Added more checks to SafeStackFrameIterator to prevent crashes when profiling.
Tested by profiling 3d-morph.js a 100 times both in debug and release builds.

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

[Kasper Lund, 2009-03-25 11:17:13]

LGTM.

http://codereview.chromium.org/42600/diff/1/2
File src/frames.cc (right):

http://codereview.chromium.org/42600/diff/1/2#newcode234
Line 234: // see EntryFrame::GetCallerState
Capitalize see (see -> See) and terminate comment with . Maybe consider
explicitly describing that this is customized versions of GetCallerState for
entry frames and GetCallerStackPointer for argument adaptor frames and explain
why it it breaks without these guards.

http://codereview.chromium.org/42600/diff/1/2#newcode235
Line 235: if (!IsValidStackAddress(
Compute the stack address (caller_fp?) in a local variable before the
IsValidStackAddress check.

http://codereview.chromium.org/42600/diff/1/2#newcode241
Line 241: // see ArgumentsAdaptorFrame::GetCallerStackPointer
I think this comment should be improved like above. It's not super clear why you
need this.

http://codereview.chromium.org/42600/diff/1/2#newcode242
Line 242: if (!reinterpret_cast<ArgumentsAdaptorFrame*>(frame)->
I would break this by introducing a local variable for the adaptor_frame (after
the cast) to make it easier to read. Maybe also document what GetExpression(0)
is by introducing a local variable (with a name like number_of_arguments) for
that?

[Mikhail Naganov, 2009-03-25 12:58:35]

http://codereview.chromium.org/42600/diff/1/2
File src/frames.cc (right):

http://codereview.chromium.org/42600/diff/1/2#newcode234
Line 234: // see EntryFrame::GetCallerState
On 2009/03/25 11:17:13, Kasper Lund wrote:
> Capitalize see (see -> See) and terminate comment with . Maybe consider
> explicitly describing that this is customized versions of GetCallerState for
> entry frames and GetCallerStackPointer for argument adaptor frames and explain
> why it it breaks without these guards.

Done.

http://codereview.chromium.org/42600/diff/1/2#newcode235
Line 235: if (!IsValidStackAddress(
On 2009/03/25 11:17:13, Kasper Lund wrote:
> Compute the stack address (caller_fp?) in a local variable before the
> IsValidStackAddress check. 

Done.

http://codereview.chromium.org/42600/diff/1/2#newcode241
Line 241: // see ArgumentsAdaptorFrame::GetCallerStackPointer
On 2009/03/25 11:17:13, Kasper Lund wrote:
> I think this comment should be improved like above. It's not super clear why
you
> need this.

Done.

http://codereview.chromium.org/42600/diff/1/2#newcode242
Line 242: if (!reinterpret_cast<ArgumentsAdaptorFrame*>(frame)->
On 2009/03/25 11:17:13, Kasper Lund wrote:
> I would break this by introducing a local variable for the adaptor_frame
(after
> the cast) to make it easier to read. Maybe also document what GetExpression(0)
> is by introducing a local variable (with a name like number_of_arguments) for
> that?

Done.