1194014 - Firefox on Android will leak its media resource

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Jean-Yves Avenard [:jya]]

While going over the user of MediaResources as part of bug 1190238, I noticed a couple of issues in the AndroidMediaResourceServer.

1- It will always leak a MediaResource for each new media element
2- In the extremely unlikely case a URL hash already exists in the map, the old entry will be deleted which could cause a crash later.

Comment 1

[Jean-Yves Avenard [:jya]]

1. is due to this http://mxr.mozilla.org/mozilla-central/source/dom/media/android/AndroidMediaPluginHost.cpp#291

We have a lone AddRef on the mediaresource ; the AndroidMediaResourceServer already owns one.

I may have missed something, but at a glance, it's an imbalanced AddRef()/Release()

2. is due to this:
http://mxr.mozilla.org/mozilla-central/source/dom/media/android/AndroidMediaResourceServer.cpp#464

It test if an entry already exists in the current resource map but searching aUrl ; but aUrl will always be empty, so the find will always return false.

It should be using the random url instead.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

What sort of information do you need from me?

Comment 3

[Jean-Yves Avenard [:jya]]

Sorry, it was more to raise your attention to it.

Comment 4

[Ralph Giles (:rillian)]

Would be nice to get this sorted. Can you take a look esawin?

Comment 5

[Eugen Sawin [:esawin]]

> 1. is due to this
> http://mxr.mozilla.org/mozilla-central/source/dom/media/android/
> AndroidMediaPluginHost.cpp#291
> 
> We have a lone AddRef on the mediaresource ; the AndroidMediaResourceServer
> already owns one.

Agreed, AndroidMediaResourceServer::AddResource already increases the ref count, this looks redundant and will leak.

> 2. is due to this:
> http://mxr.mozilla.org/mozilla-central/source/dom/media/android/
> AndroidMediaResourceServer.cpp#464
> 
> It test if an entry already exists in the current resource map but searching
> aUrl ; but aUrl will always be empty, so the find will always return false.
> 
> It should be using the random url instead.

Agreed, I don't see how this has ever worked for multiple resources.

I'm also wondering how ServeResourceEvent::GetMediaResource can successfully retrieve a resource based on the original URL without the salt that is automatically added in AndroidMediaResourceServer::AddResource.

Optional: we should change ResourceMap to std::unordered_map for faster lookup.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1194014-Fix-resource-leaking-and-some-other-clea.patch

This patch fixes the media resource leaking and removes some redundant includes.

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8652611 [details] [diff] [review]
0001-Bug-1194014-Fix-resource-leaking-and-some-other-clea.patch

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

Would have been nicer to separate the patches so one patch == 1 narrow scope.
What happened with your comment that the salt was missing from the URL upon searching for a new key?

Comment 8

[Eugen Sawin [:esawin]]

> Would have been nicer to separate the patches so one patch == 1 narrow scope.
> What happened with your comment that the salt was missing from the URL upon
> searching for a new key?

You're right, I can split the patch before landing and I've marked my comment on the missing salt obsolete because the URL *is* is the salt, I've missed that before.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4922666890d
https://hg.mozilla.org/integration/mozilla-inbound/rev/81e9554308b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c546668be9b0

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4922666890d
https://hg.mozilla.org/mozilla-central/rev/81e9554308b0
https://hg.mozilla.org/mozilla-central/rev/c546668be9b0