1510669 - Performance Warning: Static Field Leaks

Status: NEW

(GeckoView :: General, enhancement, P5)

Description

[Emily Toop (:fluffyemily)]

A static field will leak contexts.  

Non-static inner classes have an implicit reference to their outer class. If that outer class is for example a Fragment or Activity, then this reference means that the long-running handler/loader/task will hold a reference to the activity which prevents it from getting garbage collected.  

Similarly, direct field references to activities and fragments from these longer running instances can cause leaks.  ViewModel classes should never point to Views or non-application Contexts.

Affected Classes:

GeckoAppShell
GeckoBatteryManager
GeckoMediaDrmBridgeV21
GeckoNetworkManager
GeckoSystemStateListener

Comment 1

[Jan Henning [:JanH]]

Some/most/all? (definitively the GeckoBatteryManager and GeckoNetworkManager) of those are singletons that are intended to live as long as the app itself and only hold onto an *application* context.

Comment 2

[Chris Peterson [:cpeterson]]

Enhancements should have severity N/A.

Comment 3

[Makoto Kato [:m_kato]]

• mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHlsPlayer.java:49: Do not place Android context classes in static fields (static reference to DefaultBandwidthMeter which has field context pointing to Context); this is a memory leak
• mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java:517: This AsyncTask class should be static or leaks might occur (org.mozilla.gecko.media.GeckoMediaDrmBridgeV21.PostRequestTask)
• mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoNetworkManager.java:47: Do not place Android context classes in static fields (static reference to GeckoNetworkManager which has field mContext pointing to Context); this is a memory leak

Adding @SuppressLint("StaticFieldLeak") and use static class for AsyncTask.

Comment 4

[Yash]

Hello!
I am new to contribute to open source projects.
I just wanted to ask whether this task could be assigned to me so that I can try to solve it and start my contributing journey.

Comment 5

[Makoto Kato [:m_kato]]

(In reply to Yash from comment #4)

Hello!
I am new to contribute to open source projects.
I just wanted to ask whether this task could be assigned to me so that I can try to solve it and start my contributing journey.

Assigned.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 7

[Hardik Koul]

If this bug is still open and unassigned, may i try to fix it?

Comment 8

[Makoto Kato [:m_kato]]

(In reply to Hardik Koul from comment #7)

If this bug is still open and unassigned, may i try to fix it?

Yes, see comment #3.

Comment 9

[Hardik Koul]

(In reply to Makoto Kato [:m_kato] from comment #8)

(In reply to Hardik Koul from comment #7)

If this bug is still open and unassigned, may i try to fix it?

Yes, see comment #3.

Sure , thanks. Starting off with setting the repo up in my local.

Comment 10

[Hardik Koul]

Hey Makoto,
I am done with mercurial and git hannibal setup, but while fetching remote , im getting this error

Fetching central
fatal: repository 'https://hg.mozilla.org/mozilla-central/' not found
error: Could not fetch central
Fetching inbound
fatal: repository 'https://hg.mozilla.org/integration/mozilla-inbound/' not found
error: Could not fetch inbound

are the URLs correct for the repos?

Comment 11

[Makoto Kato [:m_kato]]

What document do you reference? https://github.com/glandium/git-cinnabar/wiki/Mozilla:-A-git-workflow-for-Gecko-development is better.

Comment 12

[Hardik Koul]

(In reply to Makoto Kato [:m_kato] from comment #11)

What document do you reference? https://github.com/glandium/git-cinnabar/wiki/Mozilla:-A-git-workflow-for-Gecko-development is better.

Thanks, i was referring this one : https://firefox-source-docs.mozilla.org/mobile/android/geckoview/contributor/mc-quick-start.html

Comment 13

[Hardik Koul]

Attachment: WIP: Bug 1510669 - Adding @SuppressLint("StaticFieldLeak") and using static class for "PostRequestTask"

Comment 14

[Hardik Koul]

Attachment: Bug 1510669 - Adding @SuppressLint("StaticFieldLeak") and using static class for "PostRequestTask"

Comment 15

[Hardik Koul]

Here is the patch with the changes suggested : (https://phabricator.services.mozilla.com/D185046)

however im aware suppressing the Lint warning is not an optimal way to solve this memory leak issue, let me know if i can try to figure out any optimal solution for that.

Comment 16

[Hardik Koul]

also apologies for submitting a duplicate patch with just a white space change, please delete that if possible.

Comment 17

[Hardik Koul]

Any update on this ??

Comment 18

[Makoto Kato [:m_kato]]

(In reply to Hardik Koul from comment #17)

Any update on this ??

https://phabricator.services.mozilla.com/D185046's status is "Changes Planned". So this isn't reviewable status. If this is ready for code review, please change status to "Needs Review".

Comment 19

[Hardik Koul]

(In reply to Makoto Kato [:m_kato] from comment #18)

(In reply to Hardik Koul from comment #17)

Any update on this ??

https://phabricator.services.mozilla.com/D185046's status is "Changes Planned". So this isn't reviewable status. If this is ready for code review, please change status to "Needs Review".

Hey, sorry was not aware of that process, have updated the status now to "Needs Review" https://phabricator.services.mozilla.com/D185046

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.