Firefox on Android will leak its media resource

RESOLVED FIXED in Firefox 43

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: esawin)

Tracking

(Blocks 1 bug)

Trunk
mozilla43
Unspecified
Android
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
OS: Unspecified → Android
(Reporter)

Comment 1

4 years ago
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)
(Reporter)

Comment 3

4 years ago
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)
Comment hidden (obsolete)
(Assignee)

Comment 6

4 years ago
This patch fixes the media resource leaking and removes some redundant includes.
Assignee: nobody → esawin
Status: NEW → ASSIGNED
Attachment #8652611 - Flags: review?(jyavenard)
(Reporter)

Comment 7

4 years ago
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+
(Assignee)

Comment 8

4 years ago
> 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.