Closed Bug 1565175 Opened 5 months ago Closed 5 months ago

HeapSnapshot can miss nodes

Categories

(DevTools :: Memory, task, P2)

task

Tracking

(firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file)

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.

Blocks: 1564017
Priority: -- → P2

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.

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

Type: defect → task
Attachment #9077354 - Attachment description: Bug 1565175 - Fix check for visited nodes in HeapSnapshotHandler. r?fitzgen! → Bug 1565175 - Fix HeapSnapshotHandler to not add nodes to |visited| until they're written to the core dump. r?fitzgen
Attachment #9077354 - Attachment description: Bug 1565175 - Fix HeapSnapshotHandler to not add nodes to |visited| until they're written to the core dump. r?fitzgen → Bug 1565175 - Fix HeapSnapshotHandler to not add nodes to |visited| until they're written to the core dump. r?fitzgen!
Pushed by jdemooij@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
You need to log in before you can comment on or make changes to this bug.