Closed Bug 1194014 Opened 5 years ago Closed 5 years ago

Firefox on Android will leak its media resource

Categories

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

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jya, Assigned: esawin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
OS: Unspecified → Android
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.
Flags: needinfo?(snorp)
What sort of information do you need from me?
Flags: needinfo?(snorp)
Sorry, it was more to raise your attention to it.
Priority: -- → P2
Would be nice to get this sorted. Can you take a look esawin?
Flags: needinfo?(esawin)
This patch fixes the media resource leaking and removes some redundant includes.
Assignee: nobody → esawin
Status: NEW → ASSIGNED
Attachment #8652611 - Flags: review?(jyavenard)
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?
Attachment #8652611 - Flags: review?(jyavenard) → 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?

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.
You need to log in before you can comment on or make changes to this bug.