DevTools heap snapshot does not show correct retaining path for iframes |
||
Issue description
The following simple html file leaks an iframe every 500ms. All iframes are stashed in window.retaining_path.child1.child2.leaking_frames array.
If you keep the html file open for a while and then take the heap snapshot, then
1) the snapshot should contain all the leaked Window,
2) a retaining path window.retaining_path.child1.child2.leaking_frames should be reported for the leaked Window (among other paths).
What actually happens is
1) There far less window objects than then the leaked window objects (~20 vs ~70).
2) None of the retainers mention window.retaining_path.child1.child2.leaking_frames
=============================================================================
<html>
<script>
var retaining_path = {
name: "root",
child1: {
name: "child1",
child2: {
name: "child2",
leaking_frames: []
}
}
};
function reload_frame() {
// Stash the iframe global object.
retaining_path.child1.child2.leaking_frames.push(window.frames[0]);
document.getElementById('counter').innerHTML =
"Leaking " + retaining_path.child1.child2.leaking_frames.length +
" window objects";
// %DebugTrackRetainingPath(window.frames[0]);
// Reload the iframe.
document.getElementById('myframe').src = document.getElementById('myframe').src
window.setTimeout(reload_frame, 500);
}
</script>
<body onload="reload_frame()">
<div>
<iframe src="https://www.wikipedia.org/" id="myframe" > </iframe>
</div>
<div id="counter"> </div>
</body>
</html>
=============================================================================
,
Jul 24 2017
Thanks, alph@. You're right about window objects being the same in the array. I fixed the test to produce 8 different window objects (see attachment). The heap snapshot contains all the leaking window objects. And the window.retaining_path is one of the retainers as expected. What still confuses me is that there are other shorter retaining paths in the heap snapshot (see screenshot). The actual retaining path has length 5. The heap snapshot contains the retaining paths of length 1, 2, 3, 4. We can check that the window.retaining_path is the real retaining path by clicking the "clear" button (sets window.retaining_path=null) and taking snapshot again (leaking window objects will disappear). So all the shorter retaining paths are false positives. In a large web app these false positives make it impossible to find the actual leak source (e.g. crbug.com/739371 ). Would you agree that this is an issue we should look into?
,
Jul 24 2017
Ulan, thanks for the updated test. I agree that this behavior is confusing and not helping to find the leak. However I can see a strong reference to the Window's native context @6321 from global handles (see the screenshot) which essentially makes the distance of 2. On the other hand heap profiler has a code that makes a special treatment for edges coming from debugger. We can adopt it to do it for this case as well. The question though how we distinguish these Window objects from the real roots?
,
Jul 25 2017
> However I can see a strong reference to the Window's native context @6321 from global handles Could it be a weak reference misreported as a strong reference? By hacking around I found a set of changes that fixes the retaining path (see screenshot). V8 patch: https://chromium-review.googlesource.com/c/585429/ Devtools patch: https://chromium-review.googlesource.com/585430 There are three issues: 1) V8HeapExplorer::SetGcSubrootReference adds artificial edge from the root to every global object (including the iframe window object). 2) The DevTools frontend seems to be mishandling weak edges at some place. Skipping recording of weak GC roots in the snapshot generator helps to fix the retaining path. 3) The DevTools frontend adds large penalty for GC roots when doing BFS over the graph.
,
Jul 25 2017
I think I know what's going on. Those global objects are in fact referenced with weak handles. The fix is coming.
,
Jul 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/927322652d2ba04d07d5aa90add3a14b7fd9280c commit 927322652d2ba04d07d5aa90add3a14b7fd9280c Author: Alexei Filippov <alph@chromium.org> Date: Wed Jul 26 20:26:40 2017 [heap-profiler] Fix reporting of fake global objects. The global objects lookup code mistakingly reports weakly referenced JSGlobalObject's as normal one. It should not. The fix just adds is_weak check into V8HeapExplorer::SetGcSubrootReference the rest is formatting. Bug: chromium:747382 Change-Id: I3fc62317dd3d8728d261f27bd58654aff13eb6fe Reviewed-on: https://chromium-review.googlesource.com/585385 Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Commit-Queue: Alexei Filippov <alph@chromium.org> Cr-Commit-Position: refs/heads/master@{#46913} [modify] https://crrev.com/927322652d2ba04d07d5aa90add3a14b7fd9280c/src/profiler/heap-snapshot-generator.cc
,
Jul 26 2017
,
Aug 28 2017
This regressed recently. Chromium at revision 497398 shows incorrect retaining path for the case from comment #3 (see screenshot). Looks like the same issue as crbug.com/749490 |
||
►
Sign in to add a comment |
||
Comment 1 by alph@chromium.org
, Jul 21 2017