Fix an assertion failure in the System memory reporter

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

5 years ago
I sometimes get this failure with the System memory reporter enabled:

> aboutMemory.js assertion failed: _amount already set for non-leaf node

It's because we have some reports whose paths are sub-paths of others, viz:

> {"process": "System", "path": "processes/other-files", "kind": 0, "units": 0, "amount": 109759488, "description": "This is the sum of all processes' 'other-files' numbers."},
> {"process": "System", "path": "processes/other-files/extensions", "kind": 0, "units": 0, "amount": 1155072, "description": "This is the sum of all processes' 'other-files/extensions' numbers."},
> {"process": "System", "path": "processes/other-files/fontconfig", "kind": 0, "units": 0, "amount": 627712, "description": "This is the sum of all processes' 'other-files/fontconfig' numbers."},

The first one is a sub-path of the other two, which is something that isn't
supported.
Assignee

Comment 1

5 years ago
The fix is simple: it just adds an extra "misc" path segment for files in the
"other-files" category that aren't fontconfig or extension files.
Attachment #8489768 - Flags: review?(erahm)
Comment on attachment 8489768 [details] [diff] [review]
Fix an assertion failure in the System memory reporter

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

r=me for cleaning up the asserts, but maybe you can look into if we're adding files without names (which seems like a bug)?

::: xpcom/base/SystemMemoryReporter.cpp
@@ +543,5 @@
>            aName.AppendLiteral("/extensions");
>          } else if (dirname.Find("/fontconfig") != -1) {
>            aName.AppendLiteral("/fontconfig");
> +        } else {
> +          aName.AppendLiteral("/misc");

I guess technically this fixes the assert, but it seems like we're working around a file with an empty basename. Maybe that really is an error or perhaps we should just do a basename.isEmpty check instead and not add the element.
Attachment #8489768 - Flags: review?(erahm) → review+
Assignee

Comment 3

5 years ago
> I guess technically this fixes the assert, but it seems like we're working
> around a file with an empty basename. Maybe that really is an error or
> perhaps we should just do a basename.isEmpty check instead and not add the
> element.

The problem is occurring on the aggregate case (for the "Other measurements" section, *not* the "explicit" tree). What happens is that |aTag| ends up empty, and is passed to aProcessSizes->Add(). The patch ensures that the tag in that case will instead be "/misc".
sorry had to backout this since i guess this caused test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=48360773&tree=Mozilla-Inbound
Assignee

Comment 6

5 years ago
This patch wasn't the cause of that failure. In fact, this patch only affects things if you have the memory.system_memory_reporter pref is set, and it's off by default.

I did a try run with the patch applied and the results were all green: https://tbpl.mozilla.org/?tree=Try&rev=5aaba1ac90b1. And bug 1069165 has been filed about the intermittent xpcshell failures.

So I'll try landing it again.
https://hg.mozilla.org/mozilla-central/rev/80e8680ef5cb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.