904321 - Test child process memory reporting in test_memoryReporters.xul

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

Child process memory reporting was broken (bug 903949) and we didn't realize because there wasn't a test for it.  There should be one.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Add a test for memory reporters in remote processes.

This is basically working -- the parent asks the child to send it some memory
reports, and the child does.  However, there are two things that need fixing:

- After window.open()ing the child process I use a 1 second timeout to give it
  enough time to initialize sufficiently to be able to handle the request.
  There must be a better way to do this.

- I open two remote windows, with the intention of creating two remote
  processes, but AFAICT only remote process is created.

evilpie, I'd really appreciate help with these two things.  Thanks!

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Multiple <browser> elements can reuse a process. The number of processes is capped at dom.ipc.processCount, which defaults to 1. You could probably alter the pref in your test.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Also, can you listen for the "load" event in the new windows (rather than the timeout)?

Comment 5

[Nicholas Nethercote [inactive]]

> Multiple <browser> elements can reuse a process. The number of processes is
> capped at dom.ipc.processCount, which defaults to 1. You could probably
> alter the pref in your test.

That works.  Thanks!

> Also, can you listen for the "load" event in the new windows (rather than the timeout)?

The parent does all the work, so it needs to know when the "load" event triggers in the children.  Is there a way to do that?

Comment 6

[Tom S. (please needinfo tschuster)]

You can add a messagelistener in chrome and from content you send a message when when the load event triggered.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

See this test for an example:
  http://mxr.mozilla.org/mozilla-central/source/dom/ipc/test.xul

You need to call addMessageListener and loadFrameScript in the onload handler for your .xul file. Then the script you load should wait for a load event and call sendAsyncMessage to notify the parent.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

One way to do this in tests is something like:

browser.messageManager.addMessageListener("test:ready", function() {
...
});
browser.messageManager.loadFrameScript("data:" + encodeURI("sendAsyncMessage('test:ready');", false);

untested, but it means you don't need a new file just to send a trivial message.

Comment 9

[Nicholas Nethercote [inactive]]

> You need to call addMessageListener and loadFrameScript in the onload
> handler for your .xul file.

Which .xul file?  I have test_memoryReporters2.xul which is the parent and does
all the work, and remote.xul which just contains a <browser> and nothing else.

> Then the script you load should wait for a load
> event and call sendAsyncMessage to notify the parent.

I read https://developer.mozilla.org/en-US/docs/The_message_manager.  I can see
how to trigger that script from remote.xul, but not how to get the message from
remote.xul back to test_memoryReporters2.xul.  Any message sent by the script
is received by the .xul file that loadFrameScript'd it, right?

Comment 10

[Mark Hammond [:markh] [:mhammond]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> > You need to call addMessageListener and loadFrameScript in the onload
> > handler for your .xul file.
> 
> Which .xul file?  I have test_memoryReporters2.xul which is the parent and
> does
> all the work, and remote.xul which just contains a <browser> and nothing
> else.

In your example, remote.xul is still in the "parent" - but its <browser> is remote.

I suspect you can do the message dance from either - eg, test_memoryReporters2.xul could possibly do something like:

w = window.open(".../remote.xul");
w.addEventListener("load", function loadHandler() {
  w.removeEventListener("load", loadHandler);
  // get the browser element from the new window.
  let remoteBrowser = w.document.getElementById("remote");
  let mm = remoteBrowser.messageManager;
  mm.addMessageListener("test:ready", function readyHandler() {
    mm.removeMessageListener("test:ready", readyHandler);
    .. // ready to continue testing
  });
  // assuming we don't actually need to wait for the content to load but just be
  // sure the remote world has initialized...
  // (If you *did* want to wait for content load, the frameScript would just have its
  // own load event handler and do the sendAsyncMessage in that)
  mm.loadFrameScript("data:" + encodeURI("sendAsyncMessage('test:ready');");
});

> > Then the script you load should wait for a load
> > event and call sendAsyncMessage to notify the parent.
> 
> I read https://developer.mozilla.org/en-US/docs/The_message_manager.  I can
> see
> how to trigger that script from remote.xul, but not how to get the message
> from
> remote.xul back to test_memoryReporters2.xul.  Any message sent by the script
> is received by the .xul file that loadFrameScript'd it, right?

It's received by the "messageManager" associated with the browser, but it should allow arbitrary listeners - so there's no reason I'm aware of that you can't do the above.

Comment 11

[Nicholas Nethercote [inactive]]

>   mm.loadFrameScript("data:" + encodeURI("sendAsyncMessage('test:ready');");

If I change "data:" to "data:," (copying from other examples in the code base)
it works.  Thanks!

> w = window.open(".../remote.xul");
> w.addEventListener("load", function loadHandler() {

Is this legit?  Registering the load listener after the window has been opened
feels very racy, though I fully admit to not knowing how this will be
sequenced.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: Add a test for memory reporters in remote processes.

BTW, the change in test_memoryReporters.xul is because the use of -1 to mean
"unknown" was removed ages ago, and I just found this relic.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 805822 [details] [diff] [review]
Add a test for memory reporters in remote processes.

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

::: toolkit/components/aboutmemory/tests/test_memoryReporters2.xul
@@ +1,5 @@
> +<?xml version="1.0"?>
> +<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
> +<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css"
> +                 type="text/css"?>
> +<window title="Test CPOWs"

Please change.

@@ +73,5 @@
> +      while (e.hasMoreElements()) {
> +        let r = e.getNext().QueryInterface(Ci.nsIMemoryReporter);
> +        r.collectReports(function(aProcess, aPath, aKind, aUnits, aAmount, aDesc) {
> +          if (aPath === "vsize") {
> +            vsizes[aProcess] = aAmount;

Just for kicks, can we verify that the vsize is reasonable (at least 10K or something)?

Comment 14

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3cc2c4f905

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/bb3cc2c4f905