1565175 - HeapSnapshot can miss nodes

Status: RESOLVED FIXED

(DevTools :: Memory, task, P2)

Description

[Jan de Mooij [:jandem]]

When I enable the baseline interpreter in the browser I get a failure in both of the test_MemoryActor_saveHeapSnapshot_0*.js xpcshell tests.

I think the problem is the pair of lines here: https://searchfox.org/mozilla-central/rev/f372470e10c8cb0691681603a1d6324dee5b3b8a/devtools/shared/heapsnapshot/HeapSnapshot.cpp#1287,1290

ShouldIncludeEdge is based on the |origin| Node in addition to the referent, but that seems wrong because it doesn't interact well with the |first| argument passed to operator(): if the first time we visit the node ShouldIncludeEdge returns false, and later it would have returned true, we can add an edge without adding the referent.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1565175 - Fix HeapSnapshotHandler to not add nodes to |visited| until they're written to the core dump. r?fitzgen!

Because ShouldIncludeEdge considers the |origin| node as well, it's not sufficient
to use the |first| argument passed to the function as that's only based on the edge's
referent node.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a task, but please change it back in case of error.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9fe4617e3a4
Fix HeapSnapshotHandler to not add nodes to |visited| until they're written to the core dump. r=fitzgen

Comment 4

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c9fe4617e3a4