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.
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.
What sort of information do you need from me?
Sorry, it was more to raise your attention to it.
4 years ago
Would be nice to get this sorted. Can you take a look esawin?
Created attachment 8652611 [details] [diff] [review] 0001-Bug-1194014-Fix-resource-leaking-and-some-other-clea.patch 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.
https://hg.mozilla.org/mozilla-central/rev/d4922666890d https://hg.mozilla.org/mozilla-central/rev/81e9554308b0 https://hg.mozilla.org/mozilla-central/rev/c546668be9b0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.