Store frame pointers

Store frame pointers

[Jacob, 2014-09-05 17:30:30]

Patchset #2 (id:20001) has been deleted

[Jacob, 2014-09-05 17:30:35]

Patchset #1 (id:1) has been deleted

[Jacob, 2014-09-05 17:32:28]

jacobr@google.com changed reviewers:
+ asiva@google.com, hausner@google.com

[Jacob, 2014-09-05 17:33:13]

Expose v8 stack pointers to the V8 debugger API used by Blink.

[hausner, 2014-09-05 18:08:36]

DBC. Someone who understand V8 should decide whether this LG.

https://codereview.chromium.org/544143003/diff/40001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/544143003/diff/40001/src/runtime.cc#newcode11417
src/runtime.cc:11417: ((static_cast<uintptr_t>(1)<<53) - 1);
How about building the mask as a const uint64_t value in a separate statement?
Also, what happens when you shift by 53 if you run on 32bit hardware? Can you
omit this address mangling (with ifdefs) when run on a 32bit platform?

[Jacob, 2014-09-05 23:46:26]

siva, can you take a look and recommend someone on the V8 team to review this
fully. We'll need to get this CL approved by the V8 team so that I can land

https://codereview.chromium.org/466243002/
in the Dart blink branch or we will need to start managing a [small] fork of the
v8 branch.

[Jacob, 2014-09-10 17:54:19]

PTAL. Switched to framePointerHigh and framePointerLow instead of truncating
frame pointers to 52 bits.

[Vyacheslav Egorov (Google), 2014-09-10 22:34:23]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2014-09-10 22:34:25]

DBC

https://codereview.chromium.org/544143003/diff/80001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/544143003/diff/80001/src/runtime.cc#newcode11415
src/runtime.cc:11415: *isolate->factory()->NewNumberFromUint(
this is GC unsafe in V8, you need to split it into two lines

Handle<...> number = isolate->factory()->NewNumberFromUint(...);
details->set(..., *number);

ditto the next line.

[Jacob, 2014-09-11 00:45:56]

jacobr@google.com changed reviewers:
+ yangguo@google.com

[Jacob, 2014-09-11 00:45:57]

Hello Yang Guo,
Vyacheslav Egorov suggested I ask you to review this v8 CL.
My goal is to expose frame pointer addresses to v8 debugger clients. Frame
pointers are needed by debuggers which merge stack traces from multiple
languages that can inter operate.
We use frame pointers to determine the ordering of frames.  The information is
exposed through FrameDetails because that is the mechanism used by the Blink
JavaScript debugger.

Thanks,
Jacob

https://codereview.chromium.org/544143003/diff/80001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/544143003/diff/80001/src/runtime.cc#newcode11415
src/runtime.cc:11415: *isolate->factory()->NewNumberFromUint(
On 2014/09/10 22:34:25, Vyacheslav Egorov (Google) wrote:
> this is GC unsafe in V8, you need to split it into two lines
> 
> Handle<...> number = isolate->factory()->NewNumberFromUint(...);
> details->set(..., *number);
> 
> ditto the next line.

Done.

[Jacob, 2014-09-18 17:04:35]

jacobr@google.com changed reviewers:
+ iposva@google.com

[Jacob, 2014-09-18 17:04:35]

+iposva

[Yang, 2014-09-18 17:49:49]

yangguo@chromium.org changed reviewers:
+ yangguo@chromium.org

[Yang, 2014-09-18 17:49:49]

sorry for overlooking this. I'll review this tomorrow!

[Yang, 2014-09-19 13:05:52]

On 2014/09/18 17:49:49, Yang wrote:
> sorry for overlooking this. I'll review this tomorrow!

While the CL looks good to me, I have some concerns about exposing internal
addresses to Javascript. I don't think we do that anywhere else, and while I
can't pinpoint the security issues this could cause, it seems bad style and
weird.

Since you will need to eventually convert the pointer address to C++ again,
would it make sense to use a C++ api in v8-debug.h that converts frame id to
address?

[Jacob, 2014-09-19 17:06:43]

On 2014/09/19 13:05:52, Yang wrote:
> On 2014/09/18 17:49:49, Yang wrote:
> > sorry for overlooking this. I'll review this tomorrow!
> 
> While the CL looks good to me, I have some concerns about exposing internal
> addresses to Javascript. I don't think we do that anywhere else, and while I
> can't pinpoint the security issues this could cause, it seems bad style and
> weird.
> 
> Since you will need to eventually convert the pointer address to C++ again,
> would it make sense to use a C++ api in v8-debug.h that converts frame id to
> address?

Thanks for taking a look.
Some clarification:
I first considered adding the API to v8-debug.h but the problem is that the
Chrome devtools rely on this API rather than the C++ API so at minimum I'd cause
all stack traces to be computed twice if I only exposed this field via the
v8-debug.h api. In addition, I hit edge cases where the list of frames exposed
via this API and the v8-debug.h API are different depending on which flags are
passed so merging results from the two APIs.

To mitigate the security concerns here are a couple options I would prefer to
moving this to v8-debug.h:
1. instead of exposing the frame pointer as a number, pass a V8 handle with the
frame pointer as a hidden field. C++ code on the blink side could then read the
frame pointer back but user code could not. This would also make the API
slightly cleaner as the code to convert possible 64 bit values into two numbers
could be removed.
2. Salt the frame pointers with a random seed. I don't care about the actual
memory addresses, only the relative order of frame pointers from VMs on the page
so as long as I can still compute the correct relative order I'm fine.