Closed Bug 1207645 Opened 6 years ago Closed 6 years ago

Define a global actor on the root process to shuffle heap snapshot files

Categories

(DevTools :: Memory, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 3 obsolete files)

Bug 1201597 let us save heap snapshots in sandboxed child processes that don't have access to the file system, but the MemoryActor (in the child process) is still trying to access the file system in the case that the client requests the heap snapshot file be sent over the RDP.

We need to split this file shuffling out from the MemoryActor to a new global actor in the parent process, which does have access to the file system.
Attached patch Create HeapSnapshotFileActor (obsolete) — Splinter Review
This commit creates the HeapSnapshotFileActor and moves the transferHeapSnapshot
method from MemoryActor to HeapSnapshotFileActor. This is necessary because
child processes in e10s are sandboxed and do not have access to the file system,
and because MemoryActor is in the sandboxed child process it cannot open the
file and send it over the RDP.

This complexity is hidden at the MemoryFront layer. Users of MemoryFront will
still simply call its saveHeapSnapshot method, and do not need to worry about
the inner workings of how heap snapshot files are transferred over the RDP. This
required adding a third parameter to MemoryFront's initialize method: the
listTabs response.
Attachment #8665145 - Flags: review?(jryans)
Comment on attachment 8665145 [details] [diff] [review]
Create HeapSnapshotFileActor

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

Add a comment to memory actor's use of `saveHeapSnapshot` about this being safe in the child even though it doesn't have file system access due to the special work you've done in bug 1201597.

::: devtools/client/memory/panel.js
@@ +65,5 @@
>  };
>  
>  exports.MemoryPanel = MemoryPanel;
> +
> +function listTabs(client) {

Let's make `target` do this work for us!  You're not the only person to ask for this ability before.  I suppose it can't easily go in `get root()` since it's async, but let's do it somehow.  Maybe it changes to `function* getRoot()`?  Or the getter is just a promise...  Whatever you like.

::: devtools/client/memory/test/browser/head.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +function scopedCuImport(path) {

I was pretty sad after Shu's email to learn we've all been using Cu.import wrong all this time...  Is something like this available as a shared utility?  I think we'd want it even for production code.

@@ +22,5 @@
> +
> +/**
> + * Add a tab and have it load the given url.
> + */
> +function addTab(url, win = window) {

Many of these functions exist in the `shared-head` file[1].  You should include that here (example[2]) and de-duplicate the rest of this file.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js
[2]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/eyedropper/test/head.js#5

::: devtools/server/tests/unit/head_dbg.js
@@ +51,5 @@
>  
>    return function run_test() {
>      do_test_pending();
>      startTestDebuggerServer(TEST_GLOBAL_NAME).then(client => {
> +      DebuggerServer.registerModule("devtools/server/actors/heap-snapshot-file", {

Hmm, it doesn't seem like we should have to do this...  Can't we call `addBrowserActors` in `startTestDebuggerServer`?

On that topic `startTestDebuggerServer` seems to have some errors, it takes a `server` param, but then just uses `DebuggerServer` in some lines.
Attachment #8665145 - Flags: review?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> ::: devtools/client/memory/test/browser/head.js
> @@ +2,5 @@
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict";
> > +
> > +function scopedCuImport(path) {
> 
> I was pretty sad after Shu's email to learn we've all been using Cu.import
> wrong all this time...  Is something like this available as a shared
> utility?  I think we'd want it even for production code.

But then, how do you import this helper? :-/

> 
> @@ +22,5 @@
> > +
> > +/**
> > + * Add a tab and have it load the given url.
> > + */
> > +function addTab(url, win = window) {
> 
> Many of these functions exist in the `shared-head` file[1].  You should
> include that here (example[2]) and de-duplicate the rest of this file.
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/
> test/shared-head.js
> [2]:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/eyedropper/
> test/head.js#5

Good call, this was me being a lazy programmer.

> ::: devtools/server/tests/unit/head_dbg.js
> @@ +51,5 @@
> >  
> >    return function run_test() {
> >      do_test_pending();
> >      startTestDebuggerServer(TEST_GLOBAL_NAME).then(client => {
> > +      DebuggerServer.registerModule("devtools/server/actors/heap-snapshot-file", {
> 
> Hmm, it doesn't seem like we should have to do this...  Can't we call
> `addBrowserActors` in `startTestDebuggerServer`?

I tried that, and no matter what combination of addBrowserActors and addTabActors I did I couldn't avoid either errors about registering actors twice, missing certain actors, or getting mysterious errors where listTabs requests would always return an empty list of tabs. I spent probably the most time out of the whole patch on this issue, and ended up with this as the best solution that actually worked.

> On that topic `startTestDebuggerServer` seems to have some errors, it takes
> a `server` param, but then just uses `DebuggerServer` in some lines.

Not surprised, I can fix them all to reference the param while I'm here.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #4)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> > ::: devtools/client/memory/test/browser/head.js
> > @@ +2,5 @@
> > > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > > +
> > > +"use strict";
> > > +
> > > +function scopedCuImport(path) {
> > 
> > I was pretty sad after Shu's email to learn we've all been using Cu.import
> > wrong all this time...  Is something like this available as a shared
> > utility?  I think we'd want it even for production code.
> 
> But then, how do you import this helper? :-/

Hmm, I guess. :P Let's make Cu.importSanely! :)

> > ::: devtools/server/tests/unit/head_dbg.js
> > @@ +51,5 @@
> > >  
> > >    return function run_test() {
> > >      do_test_pending();
> > >      startTestDebuggerServer(TEST_GLOBAL_NAME).then(client => {
> > > +      DebuggerServer.registerModule("devtools/server/actors/heap-snapshot-file", {
> > 
> > Hmm, it doesn't seem like we should have to do this...  Can't we call
> > `addBrowserActors` in `startTestDebuggerServer`?
> 
> I tried that, and no matter what combination of addBrowserActors and
> addTabActors I did I couldn't avoid either errors about registering actors
> twice, missing certain actors, or getting mysterious errors where listTabs
> requests would always return an empty list of tabs. I spent probably the
> most time out of the whole patch on this issue, and ended up with this as
> the best solution that actually worked.

The "normal" init the server snippet is:

  if (!DebuggerServer.initialized) {
    DebuggerServer.init();
    DebuggerServer.addBrowserActors();
  }

but maybe this does not work in the debugger test's special setup. `addBrowserActors` calls `addTabActors` itself, so maybe changing to the above snippet and removing `addTabActors` from `startTestDebuggerServer` will do it?
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > > ::: devtools/server/tests/unit/head_dbg.js
> > > @@ +51,5 @@
> > > >  
> > > >    return function run_test() {
> > > >      do_test_pending();
> > > >      startTestDebuggerServer(TEST_GLOBAL_NAME).then(client => {
> > > > +      DebuggerServer.registerModule("devtools/server/actors/heap-snapshot-file", {
> > > 
> > > Hmm, it doesn't seem like we should have to do this...  Can't we call
> > > `addBrowserActors` in `startTestDebuggerServer`?
> > 
> > I tried that, and no matter what combination of addBrowserActors and
> > addTabActors I did I couldn't avoid either errors about registering actors
> > twice, missing certain actors, or getting mysterious errors where listTabs
> > requests would always return an empty list of tabs. I spent probably the
> > most time out of the whole patch on this issue, and ended up with this as
> > the best solution that actually worked.
> 
> The "normal" init the server snippet is:
> 
>   if (!DebuggerServer.initialized) {
>     DebuggerServer.init();
>     DebuggerServer.addBrowserActors();
>   }
> 
> but maybe this does not work in the debugger test's special setup.
> `addBrowserActors` calls `addTabActors` itself, so maybe changing to the
> above snippet and removing `addTabActors` from `startTestDebuggerServer`
> will do it?

I tried this, and this and it didn't work. I believe this is what led to the mysteriously empty listTabs responses.
Attached patch Create HeapSnapshotFileActor (obsolete) — Splinter Review
This commit creates the HeapSnapshotFileActor and moves the transferHeapSnapshot
method from MemoryActor to HeapSnapshotFileActor. This is necessary because
child processes in e10s are sandboxed and do not have access to the file system,
and because MemoryActor is in the sandboxed child process it cannot open the
file and send it over the RDP.

This complexity is hidden at the MemoryFront layer. Users of MemoryFront will
still simply call its saveHeapSnapshot method, and do not need to worry about
the inner workings of how heap snapshot files are transferred over the RDP. This
required adding a third parameter to MemoryFront's initialize method: the
listTabs response.
Attachment #8665145 - Attachment is obsolete: true
Attachment #8665665 - Flags: review?(jryans)
https://hg.mozilla.org/mozilla-central/rev/a23053342d5b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14708536&repo=mozilla-inbound
Status: RESOLVED → REOPENED
Flags: needinfo?(nfitzgerald)
Resolution: FIXED → ---
gaia? mocha? node_modules?? what looks to be v8 format stack traces?? I am pretty sure I got caught in some cross fire here, as my patch doesn't touch any of that and is completely unrelated.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c76785790080
Flags: needinfo?(nfitzgerald)
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #13)
> gaia? mocha? node_modules?? what looks to be v8 format stack traces?? I am
> pretty sure I got caught in some cross fire here, as my patch doesn't touch
> any of that and is completely unrelated.

Possibly, though Gij21 does use some devtools APIs I believe to track the number of reflows happening. Not sure if your patch would touch those APIs.
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #13)
> gaia? mocha? node_modules?? what looks to be v8 format stack traces?? I am
> pretty sure I got caught in some cross fire here, as my patch doesn't touch
> any of that and is completely unrelated.
> 
> New try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c76785790080

The try run would be more convincing if it included the test suites that failed last time.

Adding "-u gaia-js-integration" to your syntax should do it, I think.
Doesn't touch reflow stuff.

Woops re try push.
Ugh, it looks like there are two failures in Gij:

First failure. Not sure what this is testing or why it is failing yet.

> AssertionError: 0 == 3
>     at Context.<anonymous> (/home/worker/gaia/apps/communications/dialer/test/marionette/keypad_test.js:105:12)
>     at callFn (/home/worker/gaia/node_modules/mocha/lib/runnable.js:223:21)
>     at Test.Runnable.run (/home/worker/gaia/node_modules/mocha/lib/runnable.js:216:7)
>     at Test.MarionetteTest.run (/home/worker/gaia/node_modules/marionette-js-runner/lib/ui.js:25:31)
>     at Runner.runTest (/home/worker/gaia/node_modules/mocha/lib/runner.js:373:10)
>     at /home/worker/gaia/node_modules/mocha/lib/runner.js:451:12
>     at next (/home/worker/gaia/node_modules/mocha/lib/runner.js:298:14)
>     at /home/worker/gaia/node_modules/mocha/lib/runner.js:308:7
>     at next (/home/worker/gaia/node_modules/mocha/lib/runner.js:246:23)
>     at /home/worker/gaia/node_modules/mocha/lib/runner.js:270:7
>     at done (/home/worker/gaia/node_modules/mocha/lib/runnable.js:185:5)
>     at callFn (/home/worker/gaia/node_modules/mocha/lib/runnable.js:228:7)
>     at Hook.Runnable.run (/home/worker/gaia/node_modules/mocha/lib/runnable.js:216:7)
>     at next (/home/worker/gaia/node_modules/mocha/lib/runner.js:258:10)
>     at /home/worker/gaia/node_modules/mocha/lib/runner.js:270:7
>     at done (/home/worker/gaia/node_modules/mocha/lib/runnable.js:185:5)
>     at /home/worker/gaia/node_modules/mocha/lib/runnable.js:226:31
>     at /home/worker/gaia/node_modules/marionette-js-runner/node_modules/promise/lib/core.js:33:15
>     at flush (/home/worker/gaia/node_modules/marionette-js-runner/node_modules/promise/node_modules/asap/asap.js:27:13)
>     at process._tickCallback (node.js:442:13)

Second failure. I'm not touching any of the reflows stuff, so I have no idea why this would fail...

> AssertionError: we got 0 reflows instead of 2
>     at Context.<anonymous> (/home/worker/gaia/apps/system/test/marionette/homescreen_navigation_test.js:75:12)
>     at callFn (/home/worker/gaia/node_modules/mocha/lib/runnable.js:223:21)
>     at Test.Runnable.run (/home/worker/gaia/node_modules/mocha/lib/runnable.js:216:7)
>     at Test.MarionetteTest.run (/home/worker/gaia/node_modules/marionette-js-runner/lib/ui.js:25:31)
>     at Runner.runTest (/home/worker/gaia/node_modules/mocha/lib/runner.js:373:10)
>     at /home/worker/gaia/node_modules/mocha/lib/runner.js:451:12
>     at next (/home/worker/gaia/node_modules/mocha/lib/runner.js:298:14)
>     at /home/worker/gaia/node_modules/mocha/lib/runner.js:308:7
>     at next (/home/worker/gaia/node_modules/mocha/lib/runner.js:246:23)
>     at /home/worker/gaia/node_modules/mocha/lib/runner.js:270:7
>     at done (/home/worker/gaia/node_modules/mocha/lib/runnable.js:185:5)
>     at callFn (/home/worker/gaia/node_modules/mocha/lib/runnable.js:228:7)
>     at Hook.Runnable.run (/home/worker/gaia/node_modules/mocha/lib/runnable.js:216:7)
>     at next (/home/worker/gaia/node_modules/mocha/lib/runner.js:258:10)
>     at /home/worker/gaia/node_modules/mocha/lib/runner.js:270:7
>     at done (/home/worker/gaia/node_modules/mocha/lib/runnable.js:185:5)
>     at /home/worker/gaia/node_modules/mocha/lib/runnable.js:226:31
>     at /home/worker/gaia/node_modules/marionette-js-runner/node_modules/promise/lib/core.js:33:15
>     at flush (/home/worker/gaia/node_modules/marionette-js-runner/node_modules/promise/node_modules/asap/asap.js:27:13)
>     at process._tickCallback (node.js:442:13)
Ok, I rebased on m-c and all is well with the cosmos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a553502d5417
Nevermind, I've jumped the gun again. I need to stop being so optimistic...
Are any devtools metric payloads are changing, I haven't tracked it down, but I believe the reflow count comes from: http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/devtools/hud.js#258
Is that the little developer hud thing? This doesn't touch that at all.
No, but perhaps it could be influencing the metrics collection and breaking the thing that reads devtools metrics? You may want to check in with :janx, he should be familiar with this code.
jryans pointed out that the hud initializes a MemoryFront which I missed when updating callers because I assumed all uses were under devtools/.
This commit creates the HeapSnapshotFileActor and moves the transferHeapSnapshot
method from MemoryActor to HeapSnapshotFileActor. This is necessary because
child processes in e10s are sandboxed and do not have access to the file system,
and because MemoryActor is in the sandboxed child process it cannot open the
file and send it over the RDP.

This complexity is hidden at the MemoryFront layer. Users of MemoryFront will
still simply call its saveHeapSnapshot method, and do not need to worry about
the inner workings of how heap snapshot files are transferred over the RDP. This
required adding a third parameter to MemoryFront's initialize method: the
listTabs response.
Comment on attachment 8666221 [details] [diff] [review]
Create HeapSnapshotFileActor; r=jryans

This version just makes the rootForm optional and if you try to do anything that requires it, it throws an error.

Carrying over r+.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9d137e46a15
Attachment #8666221 - Flags: review+
Attachment #8665665 - Attachment is obsolete: true
Attachment #8665666 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5be7cb87827d
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.