Closed Bug 1100360 Opened 5 years ago Closed 5 years ago

Intermittent leakcheck | default process: 32 bytes leaked (ChromeHangAnnotations, nsStringBuffer) when Gecko 36 merges to beta

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla37
Tracking Status
firefox35 --- unaffected
firefox36 + verified
firefox37 --- verified
firefox-esr31 --- unaffected

People

(Reporter: RyanVM, Assigned: aklotz)

Details

(Keywords: intermittent-failure, memory-leak)

Attachments

(1 file, 1 obsolete file)

[Tracking Requested - why for this release]: Intermittent memory leaks when Gecko 36 merges to beta.

Sorry, I ran into this earlier and apparently forgot to file it. This leak shows up frequently on Windows mochitest-3 and mochitest-other when I push current m-c to Try with the necessary bits set to simulate it being on the Beta release channel.

Given the timing and the leak stack, I wonder if this is related to bug 818307 and/or bug 1087410. The affected test suites do correlate to plug-in related tests and the timing appears to match up. Also, e10s is disabled on !Trunk, so I'm wondering if that's relevant.

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3175661&repo=try

11:02:15 INFO - TEST-INFO | leakcheck | default process: leaked 1 ChromeHangAnnotations (16 bytes)
11:02:15 INFO - TEST-INFO | leakcheck | default process: leaked 2 nsStringBuffer (16 bytes)
11:02:15 WARNING - TEST-UNEXPECTED-FAIL | leakcheck | default process: 32 bytes leaked (ChromeHangAnnotations, nsStringBuffer) 

I can provide the full patchset if needed, but it may be as simple as changing the version number from 36.0a1 to 36.0.
Summary: Intermittent leakcheck | default process: 32 bytes leaked (ChromeHangAnnotations, nsStringBuffer) → Intermittent leakcheck | default process: 32 bytes leaked (ChromeHangAnnotations, nsStringBuffer) when Gecko 36 merges to beta
mccr8's the leak expert now! :)
Just looking at the code, the issue could be that TelemetryImpl::RecordChromeHang does not free aAnnotations in the early return at the start.  The code that passes in the ChromeHangAnnotations to it seems to assume that RecordChromeHang will take ownership.

There aren't many places these objects are allocated, so it shouldn't be too hard to figure out where they aren't getting freed.  Presumably there's some kind of chrome hang thing that is disabled on Beta but enabled for Aurora and Nightly.
Tracking because we don't want test failing in beta.
Any updates here? The leaks are still showing up in recent runs and the next uplift is on 12-Jan.
Flags: needinfo?(aklotz)
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Flags: needinfo?(aklotz)
Attachment #8542664 - Flags: review?(vdjeric)
Comment on attachment 8542664 [details] [diff] [review]
Convert chromehang annotations to use UniquePtr

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

::: xpcom/threads/HangMonitor.cpp
@@ +409,5 @@
>  #ifdef REPORT_CHROME_HANGS
>        if (waitCount >= 2) {
>          uint32_t hangDuration = PR_IntervalToSeconds(now - lastTimestamp);
>          Telemetry::RecordChromeHang(hangDuration, stack, systemUptime,
> +                                    firefoxUptime, annotations);

does this build? Don't you need a Move(annotations)?
Attachment #8542664 - Flags: review?(vdjeric)
(In reply to Vladan Djeric (:vladan) from comment #6)
> Comment on attachment 8542664 [details] [diff] [review]
> Convert chromehang annotations to use UniquePtr
> 
> Review of attachment 8542664 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/threads/HangMonitor.cpp
> @@ +409,5 @@
> >  #ifdef REPORT_CHROME_HANGS
> >        if (waitCount >= 2) {
> >          uint32_t hangDuration = PR_IntervalToSeconds(now - lastTimestamp);
> >          Telemetry::RecordChromeHang(hangDuration, stack, systemUptime,
> > +                                    firefoxUptime, annotations);
> 
> does this build? Don't you need a Move(annotations)?

Oddly enough it does. Fixing...
Comment on attachment 8542676 [details] [diff] [review]
Convert chromehang annotations to use UniquePtr (r2)

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

::: toolkit/components/telemetry/Telemetry.h
@@ +238,5 @@
>                        ProcessedStack &aStack,
>                        int32_t aSystemUptime,
>                        int32_t aFirefoxUptime,
> +                      mozilla::UniquePtr<mozilla::HangMonitor::HangAnnotations>
> +                        aAnnotations);

space alignment
Attachment #8542676 - Flags: review?(vdjeric) → review+
https://hg.mozilla.org/mozilla-central/rev/0365efa5999a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8542676 [details] [diff] [review]
Convert chromehang annotations to use UniquePtr (r2)

Approval Request Comment
[Feature/regressing bug #]: 818307
[User impact if declined]: Memory leaks
[Describe test coverage new/current, TBPL]: on m-c
[Risks and why]: None
[String/UUID change made/needed]: None
Attachment #8542676 - Flags: approval-mozilla-aurora?
Attachment #8542676 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No occurrences in my latest Try runs :)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.