Comment on attachment 8655178 [details] [diff] [review] Add a method for saving heap snapshots to MemoryActor Woops, forgot to flag!

Comment on attachment 8655178 [details] [diff] [review] Add a method for saving heap snapshots to MemoryActor Review of attachment 8655178 [details] [diff] [review]: ----------------------------------------------------------------- See comments about bulk data. I'd really like for you to be able to use it here, it's a great fit for this case, but we'll also need some alternate for FxOS apps. ::: toolkit/devtools/Loader.jsm @@ +30,5 @@ > let loaderModules = { > "Services": Object.create(Services), > "toolkit/loader": Loader, > + PromiseDebugging, > + ChromeUtils, Do we use this one anywhere? I'm only seeing uses of `ThreadSafeChromeUtils`. ::: toolkit/devtools/server/actors/memory.js @@ +4,5 @@ > > "use strict"; > > +const { Cc, Ci, Cu, components } = require("chrome"); > +const DevToolsUtils = require("devtools/toolkit/DevToolsUtils"); Seems unused? @@ +19,5 @@ > +loader.lazyRequireGetter(this, "Task", "resource://gre/modules/Task.jsm", true); > +loader.lazyRequireGetter(this, "OS", "resource://gre/modules/osfile.jsm", true); > +loader.lazyRequireGetter(this, "HeapSnapshotFileUtils", > + "devtools/toolkit/shared/HeapSnapshotFileUtils"); > +loader.lazyRequireGetter(this, "ThreadSafeChromeUtils"); Lazy seems unnecessary for the fixed set of Loader modules. @@ +40,5 @@ > + * @param {String} filePath > + * > + * @returns Promise<nsIInputStream> > + */ > +function openFileStream(filePath) { This seems like an obviously useful utility function... Maybe worth exposing somewhere for others to use? DevToolsUtils, or the jumbled set of things in transport/stream-utils, perhaps? Just an idea. I would like working with bulk data to involve less machinery, so maybe this and possibly other pieces could be extracted for generic use (not necessarily right now). @@ +145,5 @@ > + > + const streamPromise = openFileStream(snapshotFilePath); > + > + const { size } = yield OS.File.stat(snapshotFilePath); > + const bulkPromise = this.conn.startBulkSend({ Bulk transfers are currently unimplemented for child processes. For desktop child processes, you don't care here, since that's the same host and you can share the file. But for FxOS apps, you are on separate hosts, and there child processes too. So, we'll need some alternate path for this case. Here are some ideas: * Add a backup non-bulk path * Wait for bulk data support across processes boundaries (bug 1200875) * Make up some out of band transport that does work * Abort if bulk unavailable :( ::: toolkit/devtools/server/tests/unit/head_dbg.js @@ +26,5 @@ > const DevToolsUtils = require("devtools/toolkit/DevToolsUtils.js"); > const { DebuggerServer } = require("devtools/server/main"); > const { DebuggerServer: WorkerDebuggerServer } = worker.require("devtools/server/main"); > const { DebuggerClient, ObjectClient } = require("devtools/toolkit/client/main"); > +const { MemoryFront } = require("devtools/server/actors/memory"); Maybe use loader.lazyRequire so we don't load it for every test? @@ +96,5 @@ > + * arguments. > + * > + * @returns `run_test` function > + */ > +function makeMemoryActorTest(testGeneratorFunction) { Not harming anything to have this here, but you could also have a memory-specific head_mem.js or something for these parts. Mossop clarified that xpcshell allows multiple heads, so you could set `head = ` to 2 files for just your memory tests, if you like. ::: toolkit/devtools/shared/HeapSnapshotFileUtils.js @@ +39,5 @@ > + */ > +exports.getNewUniqueHeapSnapshotTempFilePath = function () { > + let file = new FileUtils.File(getHeapSnapshotFileTemplate()); > + // The call to createUnique will append "-N" after the leaf name (but before > + // the extension) until a new file is found and create it. THis guarantees we Nit: THis -> This ::: toolkit/devtools/shared/memory.js @@ +15,5 @@ > loader.lazyRequireGetter(this, "StackFrameCache", > "devtools/server/actors/utils/stack", true); > +loader.lazyRequireGetter(this, "ThreadSafeChromeUtils"); > +loader.lazyRequireGetter(this, "HeapSnapshotFileUtils", > + "devtools/toolkit/shared/HeapSnapshotFileUtils"); Nit: Others above are doing a 2-space instead of aligned indents. No preference, just make them match one way or another.

We already have to implement some custom IPDL stuff for e10s and fxos child processes because they are sandboxed and can't write to the filesystem; our heap snapshot APIs already need to be tweaked a bit. So we aren't planning to build what we end up shipping on this exactly, because (as you point out) it doesn't work for all of our targets. However, we do want to get this in tree now so we don't have to maintain feature branches that don't get their tests run on treeherder and all of that mess, and then we can add support for the rest of our targets in follow ups that contain the various platform changes needed and adjust the client/server at that time as well.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3) > @@ +19,5 @@ > > +loader.lazyRequireGetter(this, "Task", "resource://gre/modules/Task.jsm", true); > > +loader.lazyRequireGetter(this, "OS", "resource://gre/modules/osfile.jsm", true); > > +loader.lazyRequireGetter(this, "HeapSnapshotFileUtils", > > + "devtools/toolkit/shared/HeapSnapshotFileUtils"); > > +loader.lazyRequireGetter(this, "ThreadSafeChromeUtils"); > > Lazy seems unnecessary for the fixed set of Loader modules. But that requires future readers to also know the implementation of the loader, which seems unnecessary. Otherwise, they will helpfully "fix" it to be a lazy import as well.

Comment on attachment 8656182 [details] [diff] [review] Add a method for saving heap snapshots to MemoryActor Review of attachment 8656182 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/shared/memory.js @@ +137,5 @@ > + * graph. > + * > + * @returns {String} The snapshot id. > + */ > + saveHeapSnapshot: expectState("attached", function () { Maybe worth a comment here or at the transfer step about the platforms and targets that currently expected to work. Can be updated as the situation changes.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7) > Comment on attachment 8656182 [details] [diff] [review] > Add a method for saving heap snapshots to MemoryActor > > Review of attachment 8656182 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/devtools/shared/memory.js > @@ +137,5 @@ > > + * graph. > > + * > > + * @returns {String} The snapshot id. > > + */ > > + saveHeapSnapshot: expectState("attached", function () { > > Maybe worth a comment here or at the transfer step about the platforms and > targets that currently expected to work. Can be updated as the situation > changes. Yeah, that's reasonable.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5505a8f3df0

Comment on attachment 8656418 [details] [diff] [review] Add a method for saving heap snapshots to MemoryActor Fix bugs introduced when addressing review comments. https://treeherder.mozilla.org/#/jobs?repo=try&revision=10c11f07dee4

Comment on attachment 8656418 [details] [diff] [review] Add a method for saving heap snapshots to MemoryActor Review of attachment 8656418 [details] [diff] [review]: ----------------------------------------------------------------- Christoph, see this try push (https://treeherder.mozilla.org/#/jobs?repo=try&revision=e079ed1e229f) and failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11075102&repo=try. Just using either NetUtil.jsm or FileUtils.jsm from JS is leading to crashes. I feel strongly that we should remove this deprecated method since clearly nothing in tree is actually using it or else all debug builds would be failing. This would mean updating whichever jsm to call the non-deprecated version. You were the last person who touched this deprecated method, how hard do you think this is? Can you point me in the right direction? ::: toolkit/devtools/DevToolsUtils.js @@ +726,5 @@ > + */ > +exports.openFileStream = function (filePath) { > + return new Promise((resolve, reject) => { > + const uri = NetUtil.newURI(new FileUtils.File(filePath)); > + NetUtil.asyncFetch(uri, (stream, result) => { I believe this and the line above are what lead to the crash. (I previously thought it was the FileUtis call, but I think it may actually be one of the NetUtil calls.)

Comment on attachment 8656418 [details] [diff] [review] Add a method for saving heap snapshots to MemoryActor Review of attachment 8656418 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #12) > > +exports.openFileStream = function (filePath) { > > + return new Promise((resolve, reject) => { > > + const uri = NetUtil.newURI(new FileUtils.File(filePath)); > > + NetUtil.asyncFetch(uri, (stream, result) => { > > I believe this and the line above are what lead to the crash. > > (I previously thought it was the FileUtis call, but I think it may actually > be one of the NetUtil calls.) It's definitely asyncFetch that's causing problems. What you would have to do is: > NetUtil.asyncFetch({ uri: uri, loadUsingSystemPrincipal: true}, (stream, result) => { The first argument has become an object, which is then passed to newChannel [1] which is then passed to create a new channel internally. Have a look at the possible arguments here [2]. Most likely you will be fine using what I suggested. What is this code you are adding for? Is there are more accurate principal than using the systemPrincipal? The reason we don't crash anywhere else in the codebase is, because we have updated all the callsites of asyncFetch(). The reason we can not eliminate it completely is because addons might still use it. Hence we compromised to fail in debug builds so that we know we have to update our own code (own, as we as mozilla own). [1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#143 [2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#200

Comment on attachment 8657214 [details] [diff] [review] Add a method for saving heap snapshots to MemoryActor This includes the suggested asyncFetch changes. Now I am getting assertion failures when xpcom is shutting down :-/ https://treeherder.mozilla.org/#/jobs?repo=try&revision=463ddf9d456a

Comment on attachment 8657276 [details] [diff] [review] Add a method for saving heap snapshots to MemoryActor Removing the lazy load of MemoryFront in head_dbg.js fixed the xpcom shutdown assertion failures. Working hypothesis is that this created some kind of leak. *fitzgen waves hands* https://treeherder.mozilla.org/#/jobs?repo=try&revision=154fc07116bd