673323 - write a test to ensure about:memory isn't broken by e10s

Status: RESOLVED DUPLICATE of bug 904321

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

about:memory already works with Fennec -- the content and chrome processes are both displayed.

Inspired by http://felipe.wordpress.com/2011/07/20/initial-electrolysis-patches-for-desktop-firefox-now-in-mozilla-central/, I just tried an --enable-e10s-compat build.  Results were mixed.

- If I open about:memory in the first tab, I only see data for one process, which makes sense.  

- If I open a 2nd tab in that window (thus spawning a remote process) and then reload about:memory in the first tab, I now see data for the remote process, which makes sense.  

- However, if I open about:memory in a tab other than the 1st tab, I only see data for the remote process.

Another problem, which is already present in Fennec, is that the GC/CC/MMP buttons at the bottom only affect to the chrome process.  We should have buttons at the bottom of each process's section, and make them affect the relevant process.

Comment 1

[Josh Matthews [:jdm]]

This is a front-end issue, as far as I know. Fennec made the choice to load all about: urls in the chrome process, and there's no equivalent code in the FF frontend.

Comment 2

[Nicholas Nethercote [inactive]]

This is a MemShrink:P1 bug because about:memory has become an enormously important tool for measuring memory usage, and it would be very sad if it didn't work completely when an e10s-enabled version of Firefox is released.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Suggestion: about:memory should provide a "merged" view by default, and verbose (or a different option) should provide a per-process view.

I'd assume a load of about:memory would trigger a request for the data to all the other processes; probably a timeout is needed on merging it for the global view.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The way fennec works with about:memory is neat, but it's ultimately kind of a hack. As far as I can tell content processes are not included when doing nsMemoryReporterManager::GetResident/GetExplicit/EnumerateReporters(). We need a real API for registering async reporters.

Something like this may work:

  interface nsIMemoryAsyncReporter : nsIMemoryMultiReporter {
    void cancelCollection();
  };

  interface nsIMemoryReporterManager  {
    ...

    void registerAsyncReporter(in nsIMemoryAsyncReporter reporter);
    void unregisterAsyncReporter(in nsIMemoryAsyncReporter reporter);

    void enumerateAsyncReporters(in unsigned long maxWaitSeconds,
                                 in nsIMemoryMultiReporterCallback callback,
                                 in nsISupports closure);
  };

ContentProcessParent would then register some kind of nsIMemoryAsyncReporter. When collectReports is called on it it would send an IPDL message to the other process. When the data comes back from the other process the nsIMemoryMultiReporterCallback would be called. If cancelCollection was called then no callback would be performed.

The implementation of the enumerateAsyncReporters method would basically call collectReports on all registered async reporters and then set a timer for the given time. Once that timer fired it would call cancelCollection on all those same reporters. Each reporter is passed the same callback that is passed to enumerateAsyncReporters.

Thoughts?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'd say this is not frontend really...

Comment 6

[Nicholas Nethercote [inactive]]

A bug of the form "ensure X doesn't get broken" is strange.  Normally we use tests to provide protection against a feature being broken, so we should do that here.  Bug 699307 is heading in that direction, but I'm not sure what it could do to satisfy this bug, because I'm not sure the ways that e10s might break about:memory.

Comment 7

[Justin Lebar (not reading bugmail)]

(In response to comment 4)
> Thoughts?

My feeling is that rather than have three observably different memory reporters, if we were going to go with the approach of adding async memory multi-reporters, we should just make nsIMemoryMultiReporter async.  I bet we could write a helper class which would make converting sync multi-reporters to async not so painful.