1350435 - getSnapshotIdFromPath() shouldn't assume parent and child process have same TmpD

Status: RESOLVED FIXED

(DevTools :: Memory, enhancement)

Description

[Andrew McCreight [:mccr8]]

See bug 1349389 for details.

But anyways, what happens is the parent process creates a file in the temp directory, sends the file name to the child process which does this: 

exports.getSnapshotIdFromPath = function (path) {
  return path.slice(OS.Constants.Path.tmpDir.length + 1,
                    path.length - ".fxsnapshot".length);
};

With the new fancy sandboxing work that is going on, the temp dir in the parent and child are not the same. The child process temp dir has some extra "Temp-{UUID}" stuff added on the end. Therefore, getSnapshotIdFromPath will return an empty string if OS.Constants.Path.tmpDir returns the correct value.

This only happens to incidentally work right now due to a bug in OS.Constants.Path snapshotting the value of tmpDir early in startup, when we eagerly load osfile.jsm from telemetry startup. (I'm going to file a separate bug on that.)

I think the right fix is to compute the snapshot ID in the parent, and send it in the message instead of the path.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1350435 - Compute snapshot ID in the parent process.

The parent and content processes can have different temp directories
when sandboxing is enabled, so the process that creates the file for a
heap snapshot must also determine the snapshot ID.

Review commit: https://reviewboard.mozilla.org/r/123662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123662/

Comment 2

[Andrew McCreight [:mccr8]]

I need to remember to get a DOM peer review for the .webidl change once fitzgen reviews this (I added a chrome-only method that is just a variation of another method, so I can't imagine it will need more than a rubber stamp DOM review).

Comment 3

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

Comment on attachment 8851347 [details]
Bug 1350435 - Compute snapshot ID in the parent process.

https://reviewboard.mozilla.org/r/123662/#review126370

Looks great! Thanks :mccr8 :)

Comment 4

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

Comment on attachment 8851347 [details]
Bug 1350435 - Compute snapshot ID in the parent process.

https://reviewboard.mozilla.org/r/123662/#review126372

Comment 5

[Andrew McCreight [:mccr8]]

Olli, could you look at the .webidl change? Thanks.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8851347 [details]
Bug 1350435 - Compute snapshot ID in the parent process.

https://reviewboard.mozilla.org/r/123662/#review126380

r+ for the .webidl

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/951e3cdf5b32
Compute snapshot ID in the parent process. r=fitzgen,smaug

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/951e3cdf5b32