Closed Bug 673323 Opened 9 years ago Closed 7 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 904321

People

(Reporter: njn, Unassigned)

References

(Blocks 1 open bug)

Details

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

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.
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.
Product: Core → Firefox
QA Contact: general → general
Blocks: fxe10s
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [MemShrink][e10s] → [MemShrink:P1][e10s]
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
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
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 904321
You need to log in before you can comment on or make changes to this bug.