Andrew McCreight [:mccr8] Assignee
	Description • 7 years ago

At startup, we load 6 .jsm files for OS file, which in turn load at least a couple of other JSMs. This is only used for telemetry. One import is unused, and the other isn't needed in the bit of the code that runs at startup.

	Andrew McCreight [:mccr8] Assignee
	Comment 1 • 7 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18dc0b3aedb10fe1a9922ed6435ce1048e400eb5

	Georg Fritzsche [:gfritzsche]
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8850208 [details]
Bug 1349389 - Lazily load osfile.jsm in telemetry.

https://reviewboard.mozilla.org/r/122870/#review125194

r=me if the function commented on is just some left-over.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:147
(Diff revision 1)
> +  getPropertyAsUint32(name) {
> +      return this.get(name);
> +  },

What is this for?

	Georg Fritzsche [:gfritzsche]
	Comment 4 • 7 years ago
mozreview-review-reply

Comment on attachment 8850208 [details]
Bug 1349389 - Lazily load osfile.jsm in telemetry.

https://reviewboard.mozilla.org/r/122870/#review125194

> What is this for?

Sorry, i now properly read your comment.

	Pulsebot
	Comment 5 • 7 years ago

Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9476c9a5f4c9
Lazily load osfile.jsm in telemetry. r=gfritzsche

	Carsten Book [:Tomcat]
	Comment 6 • 7 years ago

backed this out for causing perma failures in dt4 tests like https://treeherder.mozilla.org/logviewer.html#?job_id=85825719&repo=autoland

	Pulsebot
	Comment 7 • 7 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad36fc3e53b6
Backed out changeset 9476c9a5f4c9 for causing dt4 perma failures

	Andrew McCreight [:mccr8] Assignee
	Comment 8 • 7 years ago

The devtools heap snapshotting is failing with my patch, but only on OSX. There are actually two existing bugs that happen to cancel each other out, but my patch broke this accidental cancellation.

Bug 1: OS.Constants assumes that the value of TmpD does not change.

When osfile.jsm starts up, one thing it does is start the OSFileConstantsService. One of the things that does is to call into the directory service to get the current value of TmpD, and it saves it into a JS object. Whenever code later checks the value of OS.Constants.Path.tmpDir, it will get the snapshotted value (no pun intended) that it saw when it started up.

Despite what you might expect, the value of TmpD does change. ContentProcess::Init() changes the value of TmpD from the temp dir of the parent to a special child process temp dir. Without my patch, osfile.jsm is eagerly loaded when we start telemetry. Telemetry, in turn is started when the "app-startup" observer message goes out, which happens before ContentProcess::Init(). The entire point of my patch is to load osfile.jsm later, so we end up initializing the OSFileConstantsService AFTER ContentProcess::Init(). Thus, my patch changes the value of OS.Constants.Path.tmpDir from the parent process temp dir to the child process temp dir. (Which is correct!)

I’m not sure what the right fix is for this. The easiest thing would just be to assert that OSFileConstantsService does not start up before ContentProcess::Init() begins.

Bug 2: HeapSnapshot assumes that the parent and child processes have the same temp directory.

The HeapSnapshot devtool lets you save information about what is contained in memory to a file. In order to work with the content process sandbox, it calls into the parent process via PHeapSnapshotTempFileHelper::OpenHeapSnapshotTempFile(), which returns a file handle and a file name. The file was created by the parent process, so the file is located in the parent process’s temp directory. The file name has the format “<parent temp directory>/<some id>.fxsnaps”. This file name is then returned from ThreadSafeChromeUtils::SaveHeapSnapshot(), which passes it to getSnapshotIdFromPath() which tries to extract <some id> from the file name with this code: “path.slice(OS.Constants.Path.tmpDir.length + 1, path.length - ".fxsnapshot".length);” Note that we are in the child process now, so this implicitly requires that tmpDir have the same length in the parent and child processes.

Prior to my patch, OS.Constants.Path.tmpDir incorrectly returned the same value in both processes, so this worked. After my patch, it returns the much longer child process temp directory (it has a UUID tacked on) so getSnapshotIdFromPath() returns an empty string, which causes weird failures later, resulting in a hang.

I think the fix for this is to compute the ID in the parent process and return it instead of the path, as the path doesn’t seem to be used for anything else.

I'm not entirely sure why it fails only on OS X, but I think that's related to it using more sandboxing than Linux, but using slightly different code for some temp dir stuff than Windows.

	Andrew McCreight [:mccr8] Assignee
	Comment 9 • 7 years ago

I filed bug 1350435 for (1) and bug 1350438 for (2). I'm not sure how to fix the latter, but I think just fixing the first should be enough to get this landed.

	Pulsebot
	Comment 10 • 7 years ago

Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/401806ecf39c
Lazily load osfile.jsm in telemetry. r=gfritzsche

	Carsten Book [:Tomcat]
	Comment 11 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/401806ecf39c