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

RESOLVED DUPLICATE of bug 904321

Status

()

RESOLVED DUPLICATE of bug 904321
7 years ago
5 years ago

People

(Reporter: njn, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3][e10s])

(Reporter)

Description

7 years ago
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

7 years ago
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.
Component: General → General
Product: Core → Firefox
QA Contact: general → general
(Reporter)

Updated

7 years ago
Blocks: 653064

Updated

7 years ago
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Updated

7 years ago
Whiteboard: [MemShrink][e10s] → [MemShrink:P1][e10s]
(Reporter)

Comment 2

7 years ago
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.
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.
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?
I'd say this is not frontend really...
Component: General → XPCOM
Product: Firefox → Core
QA Contact: general → xpcom
(Reporter)

Comment 6

7 years ago
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.
Summary: e10s: Ensure about:memory works → write a test to ensure about:memory isn't broken by e10s
(Reporter)

Updated

7 years ago
Whiteboard: [MemShrink:P1][e10s] → [MemShrink:P2][e10s]
Whiteboard: [MemShrink:P2][e10s] → [MemShrink:P3][e10s]
(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.
(Reporter)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 904321
You need to log in before you can comment on or make changes to this bug.