Closed Bug 929791 Opened 6 years ago Closed 6 years ago

Dumping memory reports to file generates invalid JSON if about:memory has been loaded and child processes are present

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(1 file)

Currently the memory reporter dumping code ignores any memory reports that come from child processes.  Except... it still outputs a comma for any such memory reports.  So we can end up with a bunch of commas in a row, which is invalid JSON.  This can happen in normal builds due to the thumbnails process, and it can also happen in e10s-enabled builds.

Sample output (with the first description shortened for readability):

>     {"process": "Main Process (pid 12454)", "path": "page-faults-hard", "kind":2, "units": 2, "amount": 0, "description": "..."},,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
>    {"process": "Main Process (pid 12454)", "path": "explicit/layout/style-sheet-cache", "kind": 1, "units": 0, "amount": 389280, "description": "Memory used for some built-in style sheets."},
This patch fixes the problem.

- The crux of the change is moving the |aProcess.IsEmpty()| check to the start
  of DumpReport().  

- To safely facilitate that, I had to allow for the fact |mIsFirst| should not
  be set in that case, so I moved the setting of |mIsFirst| into DumpReport().

- Also, the |mIsFirst| stuff can be encapsulated within DumpReporterCallback
  now that we don't have both uni-reporters and multi-reporters.

It might be easier to just read the new code and decide if it looks ok, and
ignore the old code.  All this machinery is doing is setting things up to
precede the first JSON record with '[' and the rest with ','.

I haven't added a test.  I promise that in bug 929797 I'll add a test that will
cover dumping reports to file in the presence of multiple processes, that would
subsume any test I would add here.
Attachment #820711 - Flags: review?(continuation)
Attachment #820711 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/9d86e7b45208
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.