Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(android): remove stored references to bridge that holds it in memory #6448

Merged
merged 3 commits into from
Mar 29, 2023

Conversation

carlpoole
Copy link
Member

Storing references to the Bridge in static classes will hold views in memory outside of their lifecycle, because the Bridge has an activity/fragment variable to keep a reference to its view. This is more obvious in Portals apps where memory leaks were found:

┬───
│ GC Root: System class
│
├─ java.net.CookieHandler class
│    Leaking: NO (a class is never leaking)
│    ↓ static CookieHandler.cookieHandler
│                           ~~~~~~~~~~~~~
├─ com.getcapacitor.plugin.CapacitorCookieManager instance
│    Leaking: UNKNOWN
│    Retaining 266.6 kB in 3585 objects
│    ↓ CapacitorCookieManager.bridge
│                             ~~~~~~
├─ com.getcapacitor.Bridge instance
│    Leaking: UNKNOWN
│    Retaining 266.5 kB in 3580 objects
│    context instance of com.vattenfall.androidnative.ui.PortalActivity with
│    mDestroyed = true
│    ↓ Bridge.context
│             ~~~~~~~
╰→ com.vattenfall.androidnative.ui.PortalActivity instance
​     Leaking: YES (ObjectWatcher was watching this because com.vattenfall.
​     androidnative.ui.PortalActivity received Activity#onDestroy() callback
​     and Activity#mDestroyed is true)
​     Retaining 170.8 kB in 2801 objects
​     key = d889e27d-5ecc-4c83-a43c-02a2190de86e
​     watchDurationMillis = 5540
​     retainedDurationMillis = 537
​     mApplication instance of com.vattenfall.androidnative.
​     AndroidNativeApplication
​     mBase instance of androidx.appcompat.view.ContextThemeWrapper

METADATA

Build.VERSION.SDK_INT: 31
Build.MANUFACTURER: Google
LeakCanary version: 2.10
App process name: com.vattenfall.androidnative
Class count: 23099
Instance count: 199481
Primitive array count: 133823
Object array count: 28068
Thread count: 35
Heap total bytes: 25058634
Bitmap count: 4
Bitmap total bytes: 153944
Large bitmap count: 0
Large bitmap total bytes: 0
Count of retained yet cleared: 4 KeyedWeakReference instances
Stats: LruCache[maxSize=3000,hits=110723,misses=191299,hitRate=36%]
RandomAccess[bytes=9439717,reads=191299,travel=55942458465,range=30650108,size=3
7711052]
Analysis duration: 3605 ms

After removing variables storing the Bridge on the HttpRequestHandler (a direct static reference) and on the CapacitorCookieManager (stored by static CookieHandler reference by the system) this memory leak seems to be resolved.

Comment on lines +42 to +43
this.localUrl = bridge.getLocalUrl();
this.serverUrl = bridge.getServerUrl();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace storing the Bridge with storing the strings needed for getSanitizedDomain

@@ -31,8 +31,7 @@ private void http(final PluginCall call, final String httpMethod) {
@Override
public void run() {
try {
HttpRequestHandler.bridge = bridge;
JSObject response = HttpRequestHandler.request(call, httpMethod);
JSObject response = HttpRequestHandler.request(call, httpMethod, getBridge());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace storing a static reference to the Bridge with passing an instance of the Bridge in to the request call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants