Really fix missing HandleScope to prevent local handles to DOMWindow leaking.

Really fix missing HandleScope to prevent local handles to DOMWindow leaking.

A follow-up to r9994.

R=vitalyr@chromium.org
BUG=102895
TEST=cctests/test-heap-profiler/NoHandleLeaks

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

[Vitaly Repeshko, 2011-11-15 20:06:01]

Will LGTM with a test :) and the comment addressed.

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

http://codereview.chromium.org/8468016/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:2476:
isolate->global_handles()->IterateAllRoots(&enumerator);
I'm a bit worried that by allocating handles we're creating new GC roots while
traversing GC roots. It's probably better to save raw pointers with
AssertNoAllocation first and then promote them to handles.

[Vitaly Repeshko, 2011-11-15 20:07:18]

Now will LGTM with a test only :)

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

http://codereview.chromium.org/8468016/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:2476:
isolate->global_handles()->IterateAllRoots(&enumerator);
On 2011/11/15 20:06:01, Vitaly Repeshko wrote:
> I'm a bit worried that by allocating handles we're creating new GC roots while
> traversing GC roots. It's probably better to save raw pointers with
> AssertNoAllocation first and then promote them to handles.

Ah, never mind. We only traverse global handles here.

[mnaganov (inactive), 2011-11-17 17:46:54]

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

http://codereview.chromium.org/8468016/diff/1/src/profile-generator.cc#newcod...
src/profile-generator.cc:2476:
isolate->global_handles()->IterateAllRoots(&enumerator);
On 2011/11/15 20:07:19, Vitaly Repeshko wrote:
> On 2011/11/15 20:06:01, Vitaly Repeshko wrote:
> > I'm a bit worried that by allocating handles we're creating new GC roots
while
> > traversing GC roots. It's probably better to save raw pointers with
> > AssertNoAllocation first and then promote them to handles.
> 
> Ah, never mind. We only traverse global handles here.

Added a test.

[Vitaly Repeshko, 2011-11-17 18:08:19]

LGTM

http://codereview.chromium.org/8468016/diff/4001/test/cctest/test-heap-profil...
File test/cctest/test-heap-profiler.cc (right):

http://codereview.chromium.org/8468016/diff/4001/test/cctest/test-heap-profil...
test/cctest/test-heap-profiler.cc:898: TEST(NoHandleLeaks) {
Can you also check that if there's a garbage global object at the time a
snapshot is taken that it won't end up in the snapshot?

[mnaganov (inactive), 2011-11-18 11:44:22]

On 2011/11/17 18:08:19, Vitaly Repeshko wrote:
> LGTM
> 
>
http://codereview.chromium.org/8468016/diff/4001/test/cctest/test-heap-profil...
> File test/cctest/test-heap-profiler.cc (right):
> 
>
http://codereview.chromium.org/8468016/diff/4001/test/cctest/test-heap-profil...
> test/cctest/test-heap-profiler.cc:898: TEST(NoHandleLeaks) {
> Can you also check that if there's a garbage global object at the time a
> snapshot is taken that it won't end up in the snapshot?

Thanks!

In the urls extractor, we only walk over global handles to find global objects.
I'm assuming that a "garbage global object" is the one that is already detached
from roots, right? In this case we wouldn't encounter it, so I don't see a point
in such a test.