1215954 - Export heap snapshots

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1215954-export.patch

Victor for styles, Nick for the rest

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8684437 [details] [diff] [review]
1215954-export.patch

Review of attachment 8684437 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments below addressed.

::: devtools/client/locales/en-US/memory.properties
@@ +33,5 @@
> +snapshot.io.save.window=Save Heap Snapshot
> +
> +# LOCALIZATION NOTE (snapshot.io.filter): The title for the filter used to
> +# filter file types (*.fxsnapshot)
> +snapshot.io.filter=Firefox Snapshots

I think this should be "Firefox Heap Snapshots" since otherwise "snapshots" is a little vague and could even be misinterpreted as a screenshot or something.

::: devtools/client/memory/actions/io.js
@@ +30,5 @@
> +
> +    dispatch({ type: actions.EXPORT_SNAPSHOT_START, snapshot });
> +
> +    assert(VALID_EXPORT_STATES.includes(snapshot.state),
> +      `Snapshot is in invalid state for exporting: ${snapshot.state}`);

Nice

@@ +36,5 @@
> +    try {
> +      yield OS.File.copy(snapshot.path, dest);
> +    } catch (error) {
> +      reportException("exportSnapshot", error);
> +      dispatch({ type: actions.EXPORT_SNAPSHOT_ERROR, snapshot, error });

Nice

::: devtools/client/memory/test/unit/test_action-export-snapshot.js
@@ +20,5 @@
> +
> +  let destPath = OS.Path.join(OS.Constants.Path.tmpDir, "tmp.fxsnapshot");
> +  yield OS.File.remove(destPath, { ignoreAbsent: true });
> +
> +  ok(!(yield OS.File.exists(destPath)), "destination file does not exist at start");

As soon as we add another test for saving snapshots, this will be all racy due to parallel xpcshell test execution and is going to result in intermittents. I experienced this with some of the early snapshot xpcshell tests, it isn't just theoretical.

Luckily, you can use this: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFile#createUnique%28%29

Then, you don't have to worry about deleting anything either, just overwrite the empty file you just createUnique'd. Alternatively, use createUnique to create a unique directory within the temp dir and then place your file in there with the knowledge that the directory will always be empty.

::: devtools/client/memory/test/unit/test_utils.js
@@ +34,5 @@
>    }), "utils.breakdownEquals() fails when deep properties are missing.");
>  
>    let s1 = utils.createSnapshot();
>    let s2 = utils.createSnapshot();
> +  equal(s1.state, states.SAVING, "utils.createSnapshot() creates snapshot in saving state");

Ugh

::: devtools/client/shared/redux/middleware/history.js
@@ +4,5 @@
> +"use strict";
> +
> +/**
> + * A middleware that stores every action coming through the store in the passed
> + * in logging object.

This needs a comment about how it will "leak" if you don't manually prune the history log. Maybe a note recommending it only for tests as well?

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1215954-export.patch

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/12974d59d668

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12974d59d668