Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2537073002:
Fix leaks in InputConnectionHandlerThread (Closed)
|
Created:
4 years ago by Changwan Ryu Modified:
4 years ago CC:
chromium-reviews, jam, darin-cc_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix leaks in InputConnectionHandlerThread
On destruction of ContentViewCore and on view detachment, we reset mHandler,
allowing it a chance to be garbage-collected.
BUG=668692
Committed: https://crrev.com/c703b49a25b8b279fe7cbd1fbfe7ff67493fbf7b
Cr-Commit-Position: refs/heads/master@{#435136}
Patch Set 1 #
Total comments: 10
Patch Set 2 : fix tests #Messages
Total messages: 38 (18 generated)
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
changwan@chromium.org changed reviewers: + aelias@chromium.org, tobiasjs@chromium.org, yabinh@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
torne@chromium.org changed reviewers: + torne@chromium.org
I have one major question: is it actually intended/required that there be one of these threads for each WebView instance? I assumed throughout all the past discussion/bugs/CLs that this was a single global thread. I don't think we have any other cases where we create N threads for N webviews. It's also not clear what the lifetimes of objects are supposed to be here; this isn't how we normally use destroy() I don't think. https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java (right): https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:73: final Handler handler = mHandler; Why is this posted to the handler just to post back to the UI thread? https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:285: destroyHandler(); Destroying it and recreating it on detach/reattach seems unnecessary, is there any reason to do this? https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:289: public void destroy() { Normally we expect that destroy() methods leave objects in a permanently destroyed state. This looks like the handler will just get recreated if the object continues to be used? It's not really destroying the InputConnectionFactory itself.
OK. We need to get a fix for this in ASAP if we're going to get it into M55. I'd like answers to my questions at some point once we've solved the immediate problem, but for now: can you just get the tests fixed and get this in? None of the issues I'm wondering about are enough that they need to block the CL. Thanks Changwan.
Description was changed from ========== Fix leaks in InputConnectionHandlerThread When the current type is NONE, then we reset mHandler and mThreadedInputConnection, allowing them a chance to be garbage-collected. BUG=668692 ========== to ========== Fix leaks in InputConnectionHandlerThread When the current type is NONE, then we reset mHandler and mThreadedInputConnection, allowing them a chance to be garbage-collected. BUG=668692 ==========
sgurun@chromium.org changed reviewers: + tedchoc@chromium.org
On 2016/11/29 18:12:25, Torne wrote: > OK. We need to get a fix for this in ASAP if we're going to get it into M55. I'd > like answers to my questions at some point once we've solved the immediate > problem, but for now: can you just get the tests fixed and get this in? None of > the issues I'm wondering about are enough that they need to block the CL. Thanks > Changwan. lgtm but I am not an Owner. Ted, PTAL.
On 2016/11/29 22:11:14, sgurun wrote:
> On 2016/11/29 18:12:25, Torne wrote:
> > OK. We need to get a fix for this in ASAP if we're going to get it into M55.
> I'd
> > like answers to my questions at some point once we've solved the immediate
> > problem, but for now: can you just get the tests fixed and get this in? None
> of
> > the issues I'm wondering about are enough that they need to block the CL.
> Thanks
> > Changwan.
>
> lgtm but I am not an Owner. Ted, PTAL.
also failing tests should be fixed:
../../content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1829:
error: TestInputConnectionFactory is not abstract and does not override abstract
method destroy() in Factory
private static class TestInputConnectionFactory implements
ChromiumBaseInputConnection.Factory {
private static class TestInputConnectionFactory implements
ChromiumBaseInputConnection.Factory {
owners lgtm https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java (right): https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:289: public void destroy() { On 2016/11/29 13:22:55, Torne wrote: > Normally we expect that destroy() methods leave objects in a permanently > destroyed state. This looks like the handler will just get recreated if the > object continues to be used? It's not really destroying the > InputConnectionFactory itself. Maybe reset would be a better name for this then.
Description was changed from ========== Fix leaks in InputConnectionHandlerThread When the current type is NONE, then we reset mHandler and mThreadedInputConnection, allowing them a chance to be garbage-collected. BUG=668692 ========== to ========== Fix leaks in InputConnectionHandlerThread On destruction of ContentViewCore and on view detachment, we reset mHandler, allowing it a chance to be garbage-collected. BUG=668692 ==========
The CQ bit was checked by changwan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2537073002/#ps20001 (title: "fix tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
To torne@, I haven't looked into the possibility of keeping one global variable for the handler. Would it also fix this problem? Is there any potential problem when we keep a global variable? If the answer is no, I'll keep a separate crbug to address the multiple webview issue in a more efficient way. https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java (right): https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:73: final Handler handler = mHandler; On 2016/11/29 13:22:55, Torne wrote: > Why is this posted to the handler just to post back to the UI thread? In most cases, InputMethodManager should access our Handler in the following way: 1) Compare Looper against main looper. 2) Call sendMessage to it. Once Looper.quit() is called, sendMessage should return false. It seems mostly fine, but I've added additional UI loop in case I missed something... https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:285: destroyHandler(); On 2016/11/29 13:22:54, Torne wrote: > Destroying it and recreating it on detach/reattach seems unnecessary, is there > any reason to do this? If we're considering multiple WebView cases more carefully, I think we should be quitting the handler when the number of webview instances go from 1 to 0. https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:289: public void destroy() { On 2016/11/29 22:32:16, Ted C wrote: > On 2016/11/29 13:22:55, Torne wrote: > > Normally we expect that destroy() methods leave objects in a permanently > > destroyed state. This looks like the handler will just get recreated if the > > object continues to be used? It's not really destroying the > > InputConnectionFactory itself. > > Maybe reset would be a better name for this then. Hmm... We already have resetAndHidKeyboard() which is called by attach(). I could not come up with a good name yet, so let me keep the name as is for now.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480476445956300,
"parent_rev": "6d30d138d5a7982b39c5d0e0edacc8adeddb8027", "commit_rev":
"3303c536496841d5f7f9ad035407e845fc86b746"}
Message was sent while issue was closed.
Description was changed from ========== Fix leaks in InputConnectionHandlerThread On destruction of ContentViewCore and on view detachment, we reset mHandler, allowing it a chance to be garbage-collected. BUG=668692 ========== to ========== Fix leaks in InputConnectionHandlerThread On destruction of ContentViewCore and on view detachment, we reset mHandler, allowing it a chance to be garbage-collected. BUG=668692 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix leaks in InputConnectionHandlerThread On destruction of ContentViewCore and on view detachment, we reset mHandler, allowing it a chance to be garbage-collected. BUG=668692 ========== to ========== Fix leaks in InputConnectionHandlerThread On destruction of ContentViewCore and on view detachment, we reset mHandler, allowing it a chance to be garbage-collected. BUG=668692 Committed: https://crrev.com/c703b49a25b8b279fe7cbd1fbfe7ff67493fbf7b Cr-Commit-Position: refs/heads/master@{#435136} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c703b49a25b8b279fe7cbd1fbfe7ff67493fbf7b Cr-Commit-Position: refs/heads/master@{#435136}
Message was sent while issue was closed.
On 2016/11/29 23:33:19, Changwan Ryu wrote: > To torne@, I haven't looked into the possibility of keeping one global variable > for the handler. Would it also fix this problem? Is there any potential problem > when we keep a global variable? I don't understand what you mean by "potential problem" here, or why you're asking me really. The important question is does any of the input handling machinery actually depend on there being a separate thread handling the input connection for each WebView instance, which is a question for you :) If it's fine for all WebViews to share a single thread to deal with the input connection, then yes, we should just create a single global thread for this purpose, which would simplify this code significantly (no need to start/stop it on demand). Is there something specific you are concerned about with global variables? I'm not sure what you mean. We use lots of globals.
Message was sent while issue was closed.
https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java (right): https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:73: final Handler handler = mHandler; On 2016/11/29 23:33:18, Changwan Ryu wrote: > On 2016/11/29 13:22:55, Torne wrote: > > Why is this posted to the handler just to post back to the UI thread? > > In most cases, InputMethodManager should access our Handler in the following > way: > 1) Compare Looper against main looper. > 2) Call sendMessage to it. > > Once Looper.quit() is called, sendMessage should return false. It seems mostly > fine, > but I've added additional UI loop in case I missed something... I don't understand what you mean here at all; maybe I wasn't clear, but what I meant was why doesn't this just simply call mHandlerThread.quit() directly from the current thread, and avoid any posting at all? https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:285: destroyHandler(); On 2016/11/29 23:33:18, Changwan Ryu wrote: > On 2016/11/29 13:22:54, Torne wrote: > > Destroying it and recreating it on detach/reattach seems unnecessary, is there > > any reason to do this? > > If we're considering multiple WebView cases more carefully, I think we should be > quitting the handler when the number of webview instances go from 1 to 0. I don't understand your answer here either; what we do when there's no webviews doesn't have anything to do with what we should do on detach/attach, does it? Nothing else in WebView quits threads/shuts things down when there are 0 webviews; it's not something we consider important and it's not generally possible in most areas of Chromium (which are not designed to be shut down and then subsequently restarted). If there was just one handler thread, then it would be fine for it to continue running forever. It's only having a continuously increasing number of threads that's not ok. https://codereview.chromium.org/2537073002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/ThreadedInputConnectionFactory.java:289: public void destroy() { On 2016/11/29 23:33:18, Changwan Ryu wrote: > On 2016/11/29 22:32:16, Ted C wrote: > > On 2016/11/29 13:22:55, Torne wrote: > > > Normally we expect that destroy() methods leave objects in a permanently > > > destroyed state. This looks like the handler will just get recreated if the > > > object continues to be used? It's not really destroying the > > > InputConnectionFactory itself. > > > > Maybe reset would be a better name for this then. > > Hmm... We already have resetAndHidKeyboard() which is called by attach(). > I could not come up with a good name yet, so let me keep the name as is for now. Well, if we have a single handler thread then there's no need for a destroy method here at all, so if that's possible then that avoids any issue here :)
Message was sent while issue was closed.
On 2016/11/30 10:33:26, Torne wrote: > On 2016/11/29 23:33:19, Changwan Ryu wrote: > > To torne@, I haven't looked into the possibility of keeping one global > variable > > for the handler. Would it also fix this problem? Is there any potential > problem > > when we keep a global variable? > > I don't understand what you mean by "potential problem" here, or why you're > asking me really. The important question is does any of the input handling > machinery actually depend on there being a separate thread handling the input > connection for each WebView instance, which is a question for you :) > If it's fine for all WebViews to share a single thread to deal with the input > connection, then yes, we should just create a single global thread for this > purpose, which would simplify this code significantly (no need to start/stop it > on demand). > > Is there something specific you are concerned about with global variables? I'm > not sure what you mean. We use lots of globals. I was concerned about the scenario where an app creates a webview, uses it, and removes it. Shouldn't we allow the webview to be garbage-collected in the middle of the life cycle of the app? That's why I was curious about the case with # instance going from 1 to 0. Maybe we should still quit the loop in this case...
Message was sent while issue was closed.
On 2016/12/01 00:05:39, Changwan Ryu wrote: > On 2016/11/30 10:33:26, Torne wrote: > > On 2016/11/29 23:33:19, Changwan Ryu wrote: > > > To torne@, I haven't looked into the possibility of keeping one global > > variable > > > for the handler. Would it also fix this problem? Is there any potential > > problem > > > when we keep a global variable? > > > > I don't understand what you mean by "potential problem" here, or why you're > > asking me really. The important question is does any of the input handling > > machinery actually depend on there being a separate thread handling the input > > connection for each WebView instance, which is a question for you :) > > If it's fine for all WebViews to share a single thread to deal with the input > > connection, then yes, we should just create a single global thread for this > > purpose, which would simplify this code significantly (no need to start/stop > it > > on demand). > > > > Is there something specific you are concerned about with global variables? I'm > > not sure what you mean. We use lots of globals. > > I was concerned about the scenario where an app creates a webview, uses it, and > removes it. > Shouldn't we allow the webview to be garbage-collected in the middle of the life > cycle of the app? > > That's why I was curious about the case with # instance going from 1 to 0. Maybe > we should still > quit the loop in this case... In any case, that would be a relatively small problem. I'll revert this and go ahead with your suggestion. Thanks a ton!
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2546613002/ by changwan@chromium.org. The reason for reverting is: caused crbug.com/669886.
Message was sent while issue was closed.
On 2016/12/01 00:05:39, Changwan Ryu wrote: > On 2016/11/30 10:33:26, Torne wrote: > > Is there something specific you are concerned about with global variables? I'm > > not sure what you mean. We use lots of globals. > > I was concerned about the scenario where an app creates a webview, uses it, and > removes it. > Shouldn't we allow the webview to be garbage-collected in the middle of the life > cycle of the app? > That's why I was curious about the case with # instance going from 1 to 0. Maybe > we should still > quit the loop in this case... I'm still not 100% sure I get what you mean, but: the instance of WebView they've stopped using will be garbage collected. There's no reference from the thread to anything else, right? It's just a standard android HandlerThread that just runs whatever it's told to. The chromium code is never unloaded, none of the Java classes can ever be be garbage collected, none of the native C++ globals are ever cleaned up, and none of the chromium threads ever exit. Chromium is not designed to have a clean shutdown process that leaves the process in a state where chromium can be started up again, and so we don't try - it would be of minimal benefit and just another source of bugs. The only property we're preserving is that all resources that are allocated to a *specific* instance of WebView are cleaned up when that instance of WebView is GCed. Having a single global thread for handling IME events means it doesn't belong to any particular WebView and so there's no expected time for it to be destroyed. |
