The default bug view has changed. See this FAQ.

Add ghost windows to about:compartments

RESOLVED FIXED in mozilla14

Status

()

Toolkit
about:memory
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Updated

5 years ago
Whiteboard: [MemShrink]
(Assignee)

Comment 1

5 years ago
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.
Attachment #608796 - Flags: review?(n.nethercote)
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.
Attachment #608796 - Flags: review?(n.nethercote) → review+
Whiteboard: [MemShrink] → [MemShrink:P1]
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
Sorry, this was backed out again on inbound because of WinXP debug reftest logs overflowing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61aa02b5a7a1
(Assignee)

Comment 5

5 years ago
Landed again: https://hg.mozilla.org/integration/mozilla-inbound/rev/1342c2f07d5c
https://hg.mozilla.org/mozilla-central/rev/1342c2f07d5c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.