CPU profiler: add secure profiles by filtering out functions using security tokens.

CPU profiler: add secure profiles by filtering out functions using security tokens.

As several pages can run in a single V8 instance, it is possible to
have functions from different security contexts intermixed in a single
CPU profile.  To avoid exposing function names from one page to
another, filtering is introduced.

The basic idea is that instead of capturing return addresses from
stack, we're now capturing JSFunction addresses (as we anyway work
only with JS stack frames.)  Each JSFunction can reach out for
context's security token. When providing a profile to a page, the
profile is filtered using the security token of caller page. Any
functions with different security tokens are filtered out (yes, we
only do fast path check for now) and their ticks are attributed to
their parents.

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

[Søren Thygesen Gjesse, 2010-05-17 07:39:25]

I think it should be possible to use the profiler through the API without having
to worry about security contexts.

When a CPU profile where time spend in other security contexts are generated it
might make sense to have an indication of how much time is spend in the
different security contexts. Each node might have received time from "hidden"
functions in another security context, and it might make sense to provide
information that of the time in a given function XX% is attributed from a
different security context.

Also on the profile as a whole providing information on time spend in other
security contexts besides the one it is filtered for could be useful.

http://codereview.chromium.org/2083005/diff/1/2
File include/v8-profiler.h (right):

http://codereview.chromium.org/2083005/diff/1/2#newcode121
include/v8-profiler.h:121: class V8EXPORT CpuProfile {
Shouldn't it be possible to get a list of all the security contexts encountered
while collecting a profile?

http://codereview.chromium.org/2083005/diff/1/2#newcode140
include/v8-profiler.h:140: class V8EXPORT CpuProfiler {
Please add some comments about what the security_token argument is used for and
how it affects the results.

http://codereview.chromium.org/2083005/diff/1/2#newcode147
include/v8-profiler.h:147: static int GetProfilesCount(Handle<Value>
security_token);
Why not just use an empty handle as a default value to allow profiling while
ignoring security tokens and not break the API?

http://codereview.chromium.org/2083005/diff/1/2#newcode163
include/v8-profiler.h:163: static void StartProfiling(Handle<String> title);
Also please explain why StartProfiling does not require a security_token while
StopProfiling does.

http://codereview.chromium.org/2083005/diff/1/9
File src/profile-generator.cc (right):

http://codereview.chromium.org/2083005/diff/1/9#newcode65
src/profile-generator.cc:65: GlobalHandles::MakeWeak(handle.location(), this,
TokenRemovedCallback);
As far as I can see the lists in the token enumerator will keep growing, don't
know whether that will cause problems (not sure about the lifetime of a
CpuProfiler), but maybe some reorganization on GC should be considered.

http://codereview.chromium.org/2083005/diff/1/9#newcode377
src/profile-generator.cc:377: CpuProfile* clone = new CpuProfile(title_, uid_);
Shouldn't the filtered clone have the information on its "original" profile and
the security token it has been filtered for?

http://codereview.chromium.org/2083005/diff/1/9#newcode566
src/profile-generator.cc:566: int CpuProfilesCollection::TokenToPosition(int
security_token_id) {
TokenToPosition -> TokenToIndex?

[mnaganov (inactive), 2010-05-17 17:20:38]

As I observed for Chromium, function call subtrees from different contexts are
rarely intermixed. I guess that they should usually not, because the opposite
would mean one page could easily access functions from another page. So storing
ticks of functions filtered due to tokens mismatch doesn't seem useful.

http://codereview.chromium.org/2083005/diff/1/2
File include/v8-profiler.h (right):

http://codereview.chromium.org/2083005/diff/1/2#newcode121
include/v8-profiler.h:121: class V8EXPORT CpuProfile {
On 2010/05/17 07:39:26, Søren Gjesse wrote:
> Shouldn't it be possible to get a list of all the security contexts
encountered
> while collecting a profile?

Not sure why it's needed. The current scenario of using tokens is as follows:
page starts profiling, then stops it, passing its security token. For v8 shells,
security tokens are not used at all.

http://codereview.chromium.org/2083005/diff/1/2#newcode140
include/v8-profiler.h:140: class V8EXPORT CpuProfiler {
On 2010/05/17 07:39:26, Søren Gjesse wrote:
> Please add some comments about what the security_token argument is used for
and
> how it affects the results.

Done.

http://codereview.chromium.org/2083005/diff/1/2#newcode147
include/v8-profiler.h:147: static int GetProfilesCount(Handle<Value>
security_token);
On 2010/05/17 07:39:26, Søren Gjesse wrote:
> Why not just use an empty handle as a default value to allow profiling while
> ignoring security tokens and not break the API?

Great idea! Fixed.

http://codereview.chromium.org/2083005/diff/1/2#newcode163
include/v8-profiler.h:163: static void StartProfiling(Handle<String> title);
On 2010/05/17 07:39:26, Søren Gjesse wrote:
> Also please explain why StartProfiling does not require a security_token while
> StopProfiling does.

Done.

http://codereview.chromium.org/2083005/diff/1/9
File src/profile-generator.cc (right):

http://codereview.chromium.org/2083005/diff/1/9#newcode65
src/profile-generator.cc:65: GlobalHandles::MakeWeak(handle.location(), this,
TokenRemovedCallback);
On 2010/05/17 07:39:26, Søren Gjesse wrote:
> As far as I can see the lists in the token enumerator will keep growing, don't
> know whether that will cause problems (not sure about the lifetime of a
> CpuProfiler), but maybe some reorganization on GC should be considered.

CpuProfiler lives forever once instantiated. So every time a new security token
is used (I guess this might happen of a page opens different iframes every
time), and a profile is gathered, a new token will be added to the list.
However, this memory consumption is nothing compared to memory consumption by
profiles themselves. Currently profiles can't be deleted (simply because it is
so in WebKit). I added a corresponding bug here:
https://bugs.webkit.org/show_bug.cgi?id=39224. It will make sense to clean up
unused tokens on profiles deletion.

http://codereview.chromium.org/2083005/diff/1/9#newcode377
src/profile-generator.cc:377: CpuProfile* clone = new CpuProfile(title_, uid_);
On 2010/05/17 07:39:26, Søren Gjesse wrote:
> Shouldn't the filtered clone have the information on its "original" profile
and
> the security token it has been filtered for?

Probably it will be needed for implementing profile cleanups. Currently not.

http://codereview.chromium.org/2083005/diff/1/9#newcode566
src/profile-generator.cc:566: int CpuProfilesCollection::TokenToPosition(int
security_token_id) {
On 2010/05/17 07:39:26, Søren Gjesse wrote:
> TokenToPosition -> TokenToIndex?

Done.

[Søren Thygesen Gjesse, 2010-05-18 13:43:10]

LGTM

http://codereview.chromium.org/2083005/diff/6001/7001
File include/v8-profiler.h (right):

http://codereview.chromium.org/2083005/diff/6001/7001#newcode143
include/v8-profiler.h:143: * Note on security tokens usage. As several pages can
run in a
Please be more general, and don't mention "pages".

[mnaganov (inactive), 2010-05-18 14:11:52]

Thanks, Soeren!

http://codereview.chromium.org/2083005/diff/6001/7001
File include/v8-profiler.h (right):

http://codereview.chromium.org/2083005/diff/6001/7001#newcode143
include/v8-profiler.h:143: * Note on security tokens usage. As several pages can
run in a
On 2010/05/18 13:43:10, Søren Gjesse wrote:
> Please be more general, and don't mention "pages".

Done.