Last Comment Bug 738624 - Add ghost windows to about:compartments
: Add ghost windows to about:compartments
Status: RESOLVED FIXED
[MemShrink:P1]
:
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 738011
Blocks: 737857
  Show dependency treegraph
 
Reported: 2012-03-23 06:38 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-04-10 04:19 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (28.48 KB, patch)
2012-03-23 11:44 PDT, Justin Lebar (not reading bugmail)
n.nethercote: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-03-23 06:38:16 PDT
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) 2012-03-23 11:44:18 PDT
Created attachment 608796 [details] [diff] [review]
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 [:njn] 2012-03-25 22:39:10 PDT
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] 2012-03-31 20:00:52 PDT
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) 2012-04-02 12:53:40 PDT
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) 2012-04-02 19:50:07 PDT
Landed again: https://hg.mozilla.org/integration/mozilla-inbound/rev/1342c2f07d5c
Comment 6 Matt Brubeck (:mbrubeck) 2012-04-03 10:56:14 PDT
https://hg.mozilla.org/mozilla-central/rev/1342c2f07d5c

Note You need to log in before you can comment on or make changes to this bug.