Closed Bug 686172 Opened 8 years ago Closed 8 years ago

smaps trees shouldn't show up where smaps isn't available

Categories

(Toolkit :: about:memory, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(1 file)

The RSS/vsize/swap breakdown trees appear on Mac and Windows, even though we have no data to fill them in with.  They simply shouldn't be there.

An easy way to fix this (and improve the experience on Linux at the same time) would be not to show completely empty smaps trees.  On Linux, this would mean not showing the "swap usage" tree most of the time, since swap is usually empty.

This might be confusing, because someone expecting Firefox to tell them how much swap is being used wouldn't even see the heading.  OTOH, in normal circumstances, it means that you don't have to click the header to discover that swap is empty.
Depends on: 674290
IMO, the best behaviour is:
- Always show all three trees on Linux, even if empty.
- Never show the trees on other platforms.

(I've said before that having to click on the "swap" header to see that the tree is empty doesn't strike me as a problem.)

If that's too hard, then not showing empty trees on Linux is better than showing empty trees on other platforms.  But hopefully the best behaviour is possible.
I'll see how hard it is to do OS detection from aboutmemory.js.
Failing that, is it possible to communicate "present-but-empty" somehow from MapsMemoryReporter.cpp?  E.g. just a single "maps/swap" reporter that has a zero value?
Attached patch Patch v1Splinter Review
Attachment #560080 - Flags: review?(khuey)
Comment on attachment 560080 [details] [diff] [review]
Patch v1

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

::: xpcom/base/MapsMemoryReporter.cpp
@@ +120,5 @@
> +  }
> +
> +  PRBool mSeenResident;
> +  PRBool mSeenVsize;
> +  PRBool mSeenSwap;

Use C++ bool please.

@@ +256,5 @@
>  MapsReporter::ParseMapping(
>    FILE *aFile,
>    nsIMemoryMultiReporterCallback *aCallback,
> +  nsISupports *aClosure,
> +  CategoriesSeen &aCategoriesSeen)

I know you fixed this in the other patch, but make this a pointer here.
Attachment #560080 - Flags: review?(khuey) → review+
Assignee: nobody → justin.lebar+bug
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.