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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

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."},

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Fix invalid JSON generation when dumping memory reports to file after viewing about:memory in the presence of child processes.

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.

Comment 2

[Nicholas Nethercote [inactive]]

Thanks for the fast review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d86e7b45208

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d86e7b45208