Closed Bug 1275117 Opened 4 years ago Closed 4 years ago

[e10s] leakcheck | default process: 8 bytes leaked (nsStringBuffer) when enabling NSPR logs

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s + ---
firefox49 --- fixed

People

(Reporter: jya, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Not sure if this is related to bug 1270752, nor if it's definitely IPC, but seeing that this issue only occurs with e10s on.

like here:
https://treeherder.mozilla.org/logviewer.html#?job_id=21278758&repo=try#L89077
https://treeherder.mozilla.org/logviewer.html#?job_id=21279204&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=21279828&repo=try

STR:
enable NSPR logging by editing testing/mochitest/runtests.py and modify NSPR_LOG_MODULES
Start mochitest

All e10s run with debug build will show the leaks.

 07:52:05     INFO -  websocket/process bridge listening on port 8191
 07:52:05     INFO -  Stopping websocket/process bridge
 07:52:05     INFO -  TEST-INFO | leakcheck | default process: leak threshold set at 0 bytes
 07:52:05     INFO -  TEST-INFO | leakcheck | plugin process: leak threshold set at 0 bytes
 07:52:05     INFO -  TEST-INFO | leakcheck | tab process: leak threshold set at 10000 bytes
 07:52:05     INFO -  TEST-INFO | leakcheck | geckomediaplugin process: leak threshold set at 20000 bytes
 07:52:05     INFO -  == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 1748
 07:52:05     INFO -       |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
 07:52:05     INFO -       |                                      | Per-Inst   Leaked|   Total      Rem|
 07:52:05     INFO -     0 |TOTAL                                 |       24        8| 4971024        1|
 07:52:05     INFO -  1215 |nsStringBuffer                        |        8        8|  177952        1|
 07:52:05     INFO -  nsTraceRefcnt::DumpStatistics: 1374 entries
 07:52:05     INFO -  TEST-INFO | leakcheck | default process: leaked 1 nsStringBuffer
 07:52:05  WARNING -  TEST-UNEXPECTED-FAIL | leakcheck | default process: 8 bytes leaked (nsStringBuffer)
 07:52:05     INFO -  == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 1750
 07:52:05     INFO -       |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
 07:52:05     INFO -       |                                      | Per-Inst   Leaked|   Total      Rem|
 07:52:05     INFO -     0 |TOTAL                                 |       22        0| 3768606        0|
 07:52:05     INFO -  nsTraceRefcnt::DumpStatistics: 943 entries
I think these are the static nsCString's I create at GeckoChildProcessHost::PerformAsyncLaunch.  Before that patch we were doing a silent strdup that has never been released and missed, because we don't track it that carefully as string buffers.

Need to figure out how to fix this...
Blocks: 1248565
Flags: needinfo?(honzab.moz)
I think converting the two static nsAutoCString restoreOrig*LogName to members of the class will do.  Even if GeckoChildProcessHost is not a singleton, we still revert the env to original state after the launch, so we are safe - we will read and cache the origin values again.
Flags: needinfo?(honzab.moz)
You can't use nsAutoCString in a class. There's a static analysis that will reject it. Maybe a regular nsCString will be okay?
(In reply to Andrew McCreight [:mccr8] from comment #3)
> You can't use nsAutoCString in a class. There's a static analysis that will
> reject it. Maybe a regular nsCString will be okay?

Of course.  Or direct buffers in UniquePtr's or something, whatever.
Attached patch v1Splinter Review
Jason, this is a super simple patch:

- moves the two strings we use to back the log file env vars from being static to being members of the class
- this makes them be released properly
- no issue if GeckoChildProcessHost is not a singleton, the values will always be the same and the method where they are used is not reentered 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1236a99ecdc
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8758317 - Flags: review?(jduell.mcbugs)
Attachment #8758317 - Flags: review?(jduell.mcbugs) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c028053889
Fix static strings leaks when mozlogging is on. r=jduell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e5c028053889
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.