Closed Bug 981911 Opened 6 years ago Closed 6 years ago

MediaStreamGraph fails to call profiler_unregister_thread() on the early return path

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lsan][MemShrink])

Attachments

(1 file, 1 obsolete file)

This results in 3.67MB of leaks when running the content/media/webaudio/test directory.  It seems like we may create and destroy a bunch of these MediaStreamGraph threads?  If that's the case, this is kind of bad, though only for builds with profiling enabled, which I guess does not include release builds.
Attachment #8388898 - Flags: review?(roc)
Benoit, don't we want to switch to using something like this as the only way to register threads?  The current register/unregister idiom makes for fixing this bug everywhere!
Flags: needinfo?(bgirard)
I looked at the other places that call this function, and the rest are all in some kind of cleanup callback method, so I'm not sure if it can be used elsewhere.

Also, GCC < 4.8 creates a spurious unused variable warning for this patch, so I'm looking into fixing it.  See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=10416 One comment there suggested that adding an empty ctor makes it go away, so I'm going to try that.
Now with empty ctor to confused GCC.  Carrying forward roc's r+.  I don't know if this is the best way to do it, but it seems reasonable to me.

try run: https://tbpl.mozilla.org/?tree=Try&rev=b0881db54c3c
Attachment #8388898 - Attachment is obsolete: true
Attachment #8389407 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/47b3d99c58a5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8389407 [details] [diff] [review]
Always call profiler_unregister_thread() when returning from MediaStreamGraphImpl::RunThread(). r=roc

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

::: content/media/MediaStreamGraph.cpp
@@ +1143,5 @@
>      }
>    }
>  }
>  
> +struct AutoProfilerUnregisterThread

I'd like to see this move into the profiler headers instead IMO. We can't use an Auto object everywhere but I'd like to share that for where we can.
Attachment #8389407 - Flags: review-
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> Benoit, don't we want to switch to using something like this as the only way
> to register threads?  The current register/unregister idiom makes for fixing
> this bug everywhere!

For the profiler init case we couldn't use a RAII object everywhere. I suspect currently for threads we -can- but I don't want to assume we always can. I would like to see this go in the profiler headers instead.
Flags: needinfo?(bgirard)
I filed bug 982827 for moving it to profiler headers.  I don't think the RAII thing can be used anywhere else, but I'll double check.
You need to log in before you can comment on or make changes to this bug.