apache / cordova-android

Apache Cordova Android
https://cordova.apache.org/
Apache License 2.0
3.59k stars 1.52k forks source link

CordovaWebViewImpl memory leak #1641

Closed www586089 closed 1 week ago

www586089 commented 9 months ago

In CordovaWebViewImpl.java, there is a code snippet:

{ LOG.d(TAG, ">>> loadUrl(" + url + ")"); if (url.equals("about:blank") || url.startsWith("javascript:")) { engine.loadUrl(url, false); return; }

    recreatePlugins = recreatePlugins || (loadedUrl == null);

    if (recreatePlugins) {
        // Don't re-initialize on first load.
        if (loadedUrl != null) {
            appPlugin = null;
            pluginManager.init();
        }
        loadedUrl = url;
    }

    // Create a timeout timer for loadUrl
    final int currentLoadUrlTimeout = loadUrlTimeout;
    final int loadUrlTimeoutValue = preferences.getInteger("LoadUrlTimeoutValue", 20000);

    // Timeout error method
    final Runnable loadError = new Runnable() {
        public void run() {
            stopLoading();
            LOG.e(TAG, "CordovaWebView: TIMEOUT ERROR!");

            // Handle other errors by passing them to the webview in JS
            JSONObject data = new JSONObject();
            try {
                data.put("errorCode", -6);
                data.put("description", "The connection to the server was unsuccessful.");
                data.put("url", url);
            } catch (JSONException e) {
                // Will never happen.
            }
            pluginManager.postMessage("onReceivedError", data);
        }
    };

    // Timeout timer method
    final Runnable timeoutCheck = new Runnable() {
        public void run() {
            try {
                synchronized (this) {
                    wait(loadUrlTimeoutValue);
                }
            } catch (InterruptedException e) {
                e.printStackTrace();
            }

            // If timeout, then stop loading and handle error
            if (loadUrlTimeout == currentLoadUrlTimeout) {
                cordova.getActivity().runOnUiThread(loadError);
            }
        }
    };

    final boolean _recreatePlugins = recreatePlugins;
    cordova.getActivity().runOnUiThread(new Runnable() {
        public void run() {
            if (loadUrlTimeoutValue > 0) {
                cordova.getThreadPool().execute(timeoutCheck);
            }
            engine.loadUrl(url, _recreatePlugins);
        }
    });
}

cordova.getThreadPool().execute(timeoutCheck); this line code just execute the timeoutCheck in a seperate thread(in thread pool), and in timeoutCheck will wait 20s, then check if timeout happen, if this happen do loadError Runnable in ui thead. my question is if run timeoutCheck in a specific thread and wait 20s, then user finish the webview container activity(less than 20s), memory leak will happen. because the thread is wating, and hold some resource. and this action seem waste resource(hold a thread). I want to know why do like this and What are the considerations. (if post a delay message (20s), then do the check will more good ? or any way to avoid the memory leak).

and there is a code snippet(the same question as before)

        // Make app visible after 2 sec in case there was a JS error and Cordova JS never initialized correctly
        if (engine.getView().getVisibility() != View.VISIBLE) {
            Thread t = new Thread(new Runnable() {
                public void run() {
                    try {
                        Thread.sleep(2000);
                        cordova.getActivity().runOnUiThread(new Runnable() {
                            public void run() {
                                pluginManager.postMessage("spinner", "stop");
                            }
                        });
                    } catch (InterruptedException e) {
                    }
                }
            });
            t.start();
        }
breautek commented 9 months ago

This particular code was implemented and has been untouched for about 9 years, so way before my time... but does seem odd that wait is used without any notify or any obvious way to cancel the thread. So I guess it just simply waits for up to the timeout value and then carries on.

Seems like timeoutCheck will only actually engage on:

final boolean _recreatePlugins = recreatePlugins;
    cordova.getActivity().runOnUiThread(new Runnable() {
        public void run() {
            if (loadUrlTimeoutValue > 0) {
                cordova.getThreadPool().execute(timeoutCheck);
            }
            engine.loadUrl(url, _recreatePlugins);
        }
    });

Particularly if loadUrlTimeoutValue is greater than 0, which by default it will be.

my question is if run timeoutCheck in a specific thread and wait 20s, then user finish the webview container activity(less than 20s), memory leak will happen. because the thread is wating, and hold some resource. and this action seem waste resource(hold a thread). I want to know why do like this and What are the considerations. (if post a delay message (20s), then do the check will more good ? or any way to avoid the memory leak).

I don't really understand how the thread is waiting and holding resources. From what I can understand, it will wait (regardless) for 20 seconds then it will continue.

The synchronized block will cause a mutex so that only a single thread may enter the block at a time, which is probably not necessary since it's in a runnable so I don't see how multiple threads could access that function/block. The wait(ms) will cause the thread to wait for up to ms then it will continue on.

I don't think that's any different than using Thread.sleep(ms)

www586089 commented 9 months ago

这是来自QQ邮箱的假期自动回复邮件。   您好,我已经收到你的邮件,谢谢!

www586089 commented 8 months ago

yes, the waiting is seem odd; it will hold a thread(this thread can't do anything and just waiting util timeout); I think if post a message(that will do the timeout check) and do the check will be more good(just check in UI thread after timeout) ? in this way, there will be no memory leak and no thread be holding until timeout. In the origin code , timeout check is done in child thread, I don't know what will happen if do the check in UI thread, so it's a problem before we know why it wait in a child thread and what are the consideration for this(timeout check in cihld thread).

breautek commented 2 months ago

I still fail to understand exactly where the memory leak is.

final boolean _recreatePlugins = recreatePlugins;
    cordova.getActivity().runOnUiThread(new Runnable() {
        public void run() {
            if (loadUrlTimeoutValue > 0) {
                cordova.getThreadPool().execute(timeoutCheck);
            }
            engine.loadUrl(url, _recreatePlugins);
        }
    });

here it runs on the UI thread, and asusme we do have a non-zero positive timeout, it will execute the timeoutCheck runnable on a thread, then proceed to load the url in the webview.

So while the webview is loading, it will also run:

// Timeout timer method
    final Runnable timeoutCheck = new Runnable() {
        public void run() {
            try {
                synchronized (this) {
                    wait(loadUrlTimeoutValue);
                }
            } catch (InterruptedException e) {
                e.printStackTrace();
            }

            // If timeout, then stop loading and handle error
            if (loadUrlTimeout == currentLoadUrlTimeout) {
                cordova.getActivity().runOnUiThread(loadError);
            }
        }
    };

Here, while the webview is loading, it does wait up to the timeout value in milliseconds.

Then it proceeds to check if the timeout state is the same as when the timeout started, and if so run the loadError runnable. If the webview has finished loading either by error or by success, loadUrlTimeout is updated and thus won't match currentLoadUrlTimeout. There is no indefinite waiting and this method will exit eventually taking at most up to the timeout value. When the function does exit, it's resources will be freeable by the GC.

Lastly if it did enter the loadError runnable, then it does:

final Runnable loadError = new Runnable() {
        public void run() {
            stopLoading();
            LOG.e(TAG, "CordovaWebView: TIMEOUT ERROR!");

            // Handle other errors by passing them to the webview in JS
            JSONObject data = new JSONObject();
            try {
                data.put("errorCode", -6);
                data.put("description", "The connection to the server was unsuccessful.");
                data.put("url", url);
            } catch (JSONException e) {
                // Will never happen.
            }
            pluginManager.postMessage("onReceivedError", data);
        }
    };

At which point, it stops loading, and creates an error object to pass to the plugins. Again no indefinite waiting here either.

Can you identity precisely what is leaking memory?

www586089 commented 2 months ago

这是来自QQ邮箱的假期自动回复邮件。   您好,我已经收到你的邮件,谢谢!

breautek commented 1 week ago

Closing as stale.

www586089 commented 1 week ago

这是来自QQ邮箱的假期自动回复邮件。   您好,我已经收到你的邮件,谢谢!