Web Inspector: heap profiler shows nodes with distance 0

[Yury Semikhatsky]

In some cases we see nodes with distance 0 retaining nodes with distance e.g. 7 All retainers of nodes with distance N should have distance >=N-1.

[Yury Semikhatsky]

It was likely broken in https://bugs.webkit.org/show_bug.cgi?id=100140 where 
v8::V8::SetGlobalGCPrologueCallback(&V8GCController::gcPrologue) call was replaced with v8::V8::AddGCPrologueCallback(&V8GCController::gcPrologue). Now heap profiler is not aware of the implicit references between DOM node wrappers and event listener functions. Such functions end up as having only weak handle to them and break heap profiler which doesn't expect any alive objects without strong references. The functions appear as having no retainers.

[Yury Semikhatsky]

Patch sent to v8: https://codereview.chromium.org/11953043/

[Yury Semikhatsky]

Created attachment 184217 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 184217 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184217&action=review

> Source/WebCore/inspector/front-end/JSHeapSnapshot.js:240
> +            var edgeType = containmentEdges[edgeIndex + edgeTypeOffset];
> +            var nodeOrdinal;
> +            if (edgeType === edgeShortcutType)
> +                nodeOrdinal = containmentEdges[edgeIndex + edgeToNodeOffset] / nodeFieldCount;
> +            else if (edgeType === edgeElementType) {
> +                node.nodeIndex = containmentEdges[edgeIndex + edgeToNodeOffset];
> +                if (node.isDocumentDOMTreesRoot())
> +                    nodeOrdinal = node.nodeIndex / nodeFieldCount;
> +                else
> +                    continue;
> +            } else
> +                continue;
> +            nodesToVisit[nodesToVisitLength++] = nodeOrdinal;
> +            flags[nodeOrdinal] |= visitedMarker;

I'd invert the conditions:

var edgeType = containmentEdges[edgeIndex + edgeTypeOffset];
var nodeIndex = containmentEdges[edgeIndex + edgeToNodeOffset];
if (edgeType === edgeElementType) {
    if (!node.isDocumentDOMTreesRoot())
        continue;
} else if (edgeType !== edgeShortcutType)
    continue;
var nodeOrdinal = nodeIndex / nodeFieldCount;
nodesToVisit[nodesToVisitLength++] = nodeOrdinal;
flags[nodeOrdinal] |= visitedMarker;

[Alexei Filippov]

Comment on attachment 184217 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184217&action=review

> Source/WebCore/inspector/front-end/HeapSnapshot.js:781
> +    distanceForUserRoot: function(node)

How about initialDistance?

[Yury Semikhatsky]

Created attachment 184224 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 184217 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184217&action=review

>> Source/WebCore/inspector/front-end/HeapSnapshot.js:781
>> +    distanceForUserRoot: function(node)
> 
> How about initialDistance?

I'd like the method name to reflect that is supposed to be called only on potential user roots. If node is not a user root it should return -1.

>> Source/WebCore/inspector/front-end/JSHeapSnapshot.js:240
>> +            flags[nodeOrdinal] |= visitedMarker;
> 
> I'd invert the conditions:
> 
> var edgeType = containmentEdges[edgeIndex + edgeTypeOffset];
> var nodeIndex = containmentEdges[edgeIndex + edgeToNodeOffset];
> if (edgeType === edgeElementType) {
>     if (!node.isDocumentDOMTreesRoot())
>         continue;
> } else if (edgeType !== edgeShortcutType)
>     continue;
> var nodeOrdinal = nodeIndex / nodeFieldCount;
> nodesToVisit[nodesToVisitLength++] = nodeOrdinal;
> flags[nodeOrdinal] |= visitedMarker;

done.

[Yury Semikhatsky]

Committed r140535: <http://trac.webkit.org/changeset/140535>

[Yury Semikhatsky]

V8 fix was committed https://code.google.com/p/v8/source/detail?r=13486, we should wait until it is rolled to Chromium and see if the problems disappears.

[Brian Burg]

Closing as invalid, as this bug pertains to the old inspector UI and/or its tests.
Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.