Closed Bug 1349389 Opened 8 years ago Closed 8 years ago

Lazily load osfile.jsm in telemetry

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

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.
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?
Attachment #8850208 - Flags: review?(gfritzsche) → review+
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.
Priority: -- → P3
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9476c9a5f4c9 Lazily load osfile.jsm in telemetry. r=gfritzsche
backed this out for causing perma failures in dt4 tests like https://treeherder.mozilla.org/logviewer.html#?job_id=85825719&repo=autoland
Flags: needinfo?(continuation)
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ad36fc3e53b6 Backed out changeset 9476c9a5f4c9 for causing dt4 perma failures
Flags: needinfo?(continuation)
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.
Depends on: 1350435
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.
Blocks: 1350469
No longer blocks: 1350469
Blocks: 1350472
Depends on: 1350688
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/401806ecf39c Lazily load osfile.jsm in telemetry. r=gfritzsche
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: