1183920 - MediaResource object will leak if last release not on the main thread

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

This is an interesting one, in that the leak isn't reported.
I found it coincidentally while investigating bug 1183573. The SourceBufferResource inherit from MediaResource and happens to use MOZ_COUNT_DTOR/MOZ_COUNT_CTOR indicating that this object gets leaked.

The MediaResource object uses the macro NS_IMPL_RELEASE_WITH_DESTROY(MediaResource, Destroy())

Where Destroy() is called when the last object refcount drops to 0.
NS_IMPL_RELEASE_WITH_DESTROY will first decrease the refcount, and if it drops to 0, will call NS_LOG_RELEASE which will register the object has having been deleted (and so won't register again in the leak detector).

Once the refcount has been dropped to 0 and information logged, the refcount is set to 1 and then Destroy() is called.

When Destroy() is called, our MediaResource has a refcount of 1.
If on the main thread, Destroy() will do "delete this" so this is fine.

However, if not on the main thread. The refcount is increased (so now it's 2) and the object is passed to NS_ProxyRelease.
NS_ProxyRelease takes the raw pointer from the nsRefPtr<MediaResource> ; as such the refcount number is still 2, even when the doomed ref pointer goes out of scope.

NS_ProxyRelease then queue a task to the main thread, which will call Release(). At the end, the mediaresource object has a refcount of 1 and as such it won't be deleted.

So now we have an object that has been marked as deleted by the leak count blob, while it hasn't.

So now, we can't simply do an extra release in Destroy() ; because NS_ProxyRelease() would then drop the refcount back to 0 again and trigger NS_IMPL_RELEASE_WITH_DESTROY(MediaResource, Destroy()) to be called.
Which would again call NS_LOG_RELEASE, and lead to incorrectly report negative leaks.

I wonder how many other places incorrectly call NS_IMPL_RELEASE_WITH_DESTROY, leading to unreported memory leaks.

Comment 1

[Jean-Yves Avenard [:jya]]

To add, I found that the MediaResource last release appears to almost always be in the media task queue.
Leading to lots of undetected leaks.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Properly delete MediaResource when released on non main thread.

Leak was not detected as it was registered via NS_LOG_RELEASE.

Comment 3

[Andrew McCreight [:mccr8]]

LSan should in theory find leaks like that.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe41216c945

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4fe41216c945

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8633825 [details] [diff] [review]
Properly delete MediaResource when released on non main thread.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Memory leak
Fix Landed on Version: nightly
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]:975858
[User impact if declined]: memory leaks
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Very low. similar implementation to existing routines needing the same behaviour (always deleting on the main thread)
[String/UUID change made/needed]: none

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> Comment on attachment 8633825 [details] [diff] [review]
> Properly delete MediaResource when released on non main thread.
> 
> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR
> consideration:
> User impact if declined: Memory leak

Given that this was introduced in bug 975858, which landed in Firefox 30, we went the entire ESR31 cycle with this bug. Unless something has changed to increase the severity, I don't think we need to backport this to ESR38. (We try to limit changes on this branch.)
 
> Approval Request Comment
> [Feature/regressing bug #]:975858
> [User impact if declined]: memory leaks

Under what circumstances will we have memory leaks? Is this for any audio/video in the browser or only select cases?

> [Describe test coverage new/current, TreeHerder]:

The patch is small but I would still like to know how you confirmed that the fix is good and does not introduce some kind of regression. How have you verified that the fix is good?

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #7)

> Under what circumstances will we have memory leaks? Is this for any audio/video in the browser or only
> select cases?

Anything using a MediaResource could potentially leak if the object holding a reference to the MediaResource is last released on a thread other than the main thread.

This is most likely going to happen with MSE.

> The patch is small but I would still like to know how you confirmed that the
> fix is good and does not introduce some kind of regression. How have you
> verified that the fix is good?

I don't have tests proving that it doesn't create a regression. The code was leaking and unfortunately, the leak detection in placed where bypassed (I explained why in the description).

There are three possibilities:
1- The leak isn't fixed. It was leaking before, it's still leaking.
2- The leak is fixed. It's all good
3- The leak is "too" fixed, and objects are deleted multiple times.

The leak was found when I created an object inheriting from MediaResource which added an extra level of MOZ_COUNT_CTOR / MOZ_COUNT_DTOR

Those weren't bypassed and try runs started to indicate objects were leaking.
After the patch, the reports of leaks have disappeared. Showing that possibility 1 has been removed

If we had possibility 3 active, we wouldn't pass any try runs. It would crash fairly quickly

That leaves us with possibility 2. Manual debugging shows that the leak is indeed fixed.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8633825 [details] [diff] [review]
Properly delete MediaResource when released on non main thread.

Thank you for the detailed response in comment 8. Given your description of the problem and the possible outcomes, I think we're good to take this fix in beta6. Beta+ Aurora+

Unless a good case is made for ESR38, I don't think we need to backport the fix. ESR38-

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/42fb4e26d6a3

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/52295d3fbcfb