738624 - Add ghost windows to about:compartments

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Justin Lebar (not reading bugmail)]

Just like we have a multi-reporter which reports the number of compartments, I think we'll want a multi-reporter which reports just the names of ghost windows.  This way, we don't have to walk the dom of every window in order to generate the number of ghost windows for Telemetry.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

As with bug 738011, let's do njn's review first and then push to someone else.

Nick, this adds a new reporter kind, KIND_SUMMARY, which is used by the
compartments and ghost-windows multi-reporter.  I didn't update any IIDs
because the new kind doesn't change the interface from a binary-compatibility
point of view, but we can update them if you'd like.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 608796 [details] [diff] [review]
Patch v1

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

Just some minor nits.  I don't care about updating the IID.  I'm comfortable enough with the code in this patch that I'm happy to not have a second reviewer, if you like.

::: dom/base/nsWindowMemoryReporter.cpp
@@ +312,5 @@
>  {
> +  if (!strcmp(aTopic, DOM_WINDOW_DESTROYED_TOPIC)) {
> +    ObserveDOMWindowDetached(aSubject);
> +  }
> +  else if (!strcmp(aTopic, "after-minimize-memory-usage")) {

Cuddle-else? :P

@@ +541,5 @@
> +NS_IMETHODIMP
> +nsWindowMemoryReporter::
> +nsGhostWindowMemoryReporter::GetExplicitNonHeap(PRInt64* aOut)
> +{
> +  *aOut = 0;

You could add a comment here like "No 'explicit/' memory reported" if you wanted.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +165,5 @@
>      os.notifyObservers(null, "memory-pressure", "heap-minimize");
>  
>      if (++i < 3)
>        runSoon(sendHeapMinNotificationsInner);
> +    else {

This if/else should have had braces... please add them to the if body.

@@ +280,5 @@
>  
> +  } else if (aKind === KIND_SUMMARY) {
> +    assert(!aUnsafePath.startsWith("explicit/") &&
> +           !aUnsafePath.startsWith("smaps/"),
> +           "bad SUMMARY path");

The other branches in this chained if/else all look at the aUnsafePath in their condition.  Please do likewise here, i.e. make the condition succeed if aUnsafePath starts with "compartments/" or "ghost-windows/", and then within the body check for KIND_SUMMARY.

@@ +1559,4 @@
>    let compartmentsByProcess = getCompartmentsByProcess(mgr);
> +  let ghostWindowsByProcess = getGhostWindowsByProcess(mgr);
> +
> +  function handleProcess(proc) {

Rename |proc| as |aProcess|... it shadows another |aProcess|, but that's ok in this case.

@@ +1665,5 @@
>  
>    return compartmentsByProcess;
>  }
>  
> +function GhostWindow(aUnsafeURL)

If I were writing this code I'd put a "//--------" line above here :P

@@ +1682,5 @@
> +};
> +
> +function getGhostWindowsByProcess(aMgr)
> +{
> +  const ghostPath = "ghost-windows/";

Is this worth it?  The "ghost-windows/" literal appears elsewhere, and getCompartmentsByProcess() just used "compartments/" throughout.

@@ +1701,5 @@
> +                        aDescription)
> +  {
> +    if (!aUnsafePath.startsWith(ghostPath)) {
> +      return;
> +    }

This shouldn't be necessary, because ignoreSingle/ignoreMulti have already done the filtering.

@@ +1703,5 @@
> +    if (!aUnsafePath.startsWith(ghostPath)) {
> +      return;
> +    }
> +
> +    let unsafeURL = aUnsafePath.split('/')[1];

Asserting that the 0th element of that split result is "ghost-windows" would be nice, as in getCompartmentsByProcess().

@@ +1731,4 @@
>  {
> +  // aEntries might be null, e.g. if there are no ghost windows for this
> +  // process.
> +  aEntries = aEntries ? aEntries : {};

|null|?  Maybe |undefined|?

I wonder if requiring that the aGhostWindows passed to appendProcessAboutCompartmentsElements() be non-undefined might be nicer than dealing with it here.  I'll let you decide.

Comment 3

[Ed Morley [:emorley]]

Push backed out for mochitest-oth orange:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9e4d09efa335

https://tbpl.mozilla.org/php/getParsedLog.php?id=10542171&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=10540830&tree=Mozilla-Inbound
...and a few more.

(Had trouble loading the logs due to size and I've got to be up in a few hours, so don't have anything more specific right now, sorry.)

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

Comment 4

[Matt Brubeck (:mbrubeck)]

Sorry, this was backed out again on inbound because of WinXP debug reftest logs overflowing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61aa02b5a7a1

Comment 5

[Justin Lebar (not reading bugmail)]

Landed again: https://hg.mozilla.org/integration/mozilla-inbound/rev/1342c2f07d5c

Comment 6

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/1342c2f07d5c