Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1001553004:
Fix race condition between attaching and destruction of native IME object. (Closed)
DescriptionFix race condition between attaching and destruction of native IME object.
There is an issue where a native IME object can be destructed while there is a pending DelayedDismissInput. This change does the attach between the java object and the native object immediately so that, in the case of the latter's destruction, it can properly notify the former and thus cancel the delayed operation.
BUG=447287
Committed: https://crrev.com/09f6510f4039c4dad8ead7afc62b1d7eff7dd9f3
Cr-Commit-Position: refs/heads/master@{#321200}
Patch Set 1 #Patch Set 2 : removed debug aids #Patch Set 3 : removed unused var name added for debugging #
Total comments: 1
Patch Set 4 : restore previous instant-dismiss behavior for most cases (safer for merge and fixes tests) #
Total comments: 4
Patch Set 5 : addressed review comments by jdduke and fixed some tests #
Total comments: 3
Patch Set 6 : make protected method private #
Total comments: 1
Patch Set 7 : moved clear of any existing delayed-dismiss to more appropriate spot #Patch Set 8 : cleaned up attach to not create delayed dismissals when they wouldn't do anything #Patch Set 9 : removed code added for other bug which no longer applies now that delayed-dismiss no longer calls a… #Patch Set 10 : remove now-usused reference to native IME adapter from DelayedDismissInput #
Total comments: 9
Patch Set 11 : address review comments about separate Dismiss class #
Total comments: 3
Messages
Total messages: 31 (4 generated)
bcwhite@chromium.org changed reviewers: + aurimas@chromium.org
Aurimas, what do you think of this? I haven't encountered any problems playing around on with various sites on my tablet but perhaps there are implications I haven't thought of?
lgtm https://codereview.chromium.org/1001553004/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1001553004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:253: public void attach(long nativeImeAdapter, int textInputType, int textInputFlags) { attach gets called from other places other than ImeAdapter#updateKeyboardVisibility() - ContentViewCore#onConfigurationChanged(). Do you know if delaying the dismissal will cause issues there?
On 2015/03/11 21:31:18, aurimas wrote: > lgtm > > https://codereview.chromium.org/1001553004/diff/40001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java > (right): > > https://codereview.chromium.org/1001553004/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:253: > public void attach(long nativeImeAdapter, int textInputType, int textInputFlags) > { > attach gets called from other places other than > ImeAdapter#updateKeyboardVisibility() - > ContentViewCore#onConfigurationChanged(). Do you know if delaying the dismissal > will cause issues there? I only saw ContentViewCore calling it and I think it's okay; dismiss was being called anyway, just not delayed. I don't expect a delay of 100ms to cause any issues and the dismissal gets scrubbed should the native be detached.
It seems like to you need to update the tests.
So I see. A bit concerning, that. <sigh>
Seems that tests are checking that the keyboard is being dismissed and of course they're not waiting 100ms. Comments on the issue are that this is important for stable release even without the launch of Unicorn so I've put back the immediate-dismiss functionality controlled by a flag to accept(...). That should keep the timing of keyboard dismissal the same as it was before while still fixing the bug. As an added bonus, the tests no longer fail due to the keyboard hanging around.
jdduke for added parameter in method call from ContentViewCore
bcwhite@chromium.org changed reviewers: + jdduke@chromium.org
No tests? :( https://codereview.chromium.org/1001553004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1001553004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1561: mImeAdapter.attach(nativeGetNativeImeAdapter(mNativeContentViewCore), Can't we make this: mImeAdapter.attach(nativeGetNativeImeAdapter(mNativeContentViewCore)); ? Then we can make the overloaded attach method private. https://codereview.chromium.org/1001553004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1001553004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:253: public void attach(long nativeImeAdapter, int textInputType, int textInputFlags, I wonder if we can make this method private. Also, The name "delay" doesn't convey a lot about its side effects, maybe delayDismissal or delayedDismiss or something like that?
I'm still investigating how to create a test for this crash. https://codereview.chromium.org/1001553004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1001553004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1561: mImeAdapter.attach(nativeGetNativeImeAdapter(mNativeContentViewCore), On 2015/03/12 03:02:06, jdduke wrote: > Can't we make this: > > mImeAdapter.attach(nativeGetNativeImeAdapter(mNativeContentViewCore)); > > ? Then we can make the overloaded attach method private. Good idea. Done. https://codereview.chromium.org/1001553004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1001553004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:253: public void attach(long nativeImeAdapter, int textInputType, int textInputFlags, On 2015/03/12 03:02:06, jdduke wrote: > I wonder if we can make this method private. Also, The name "delay" doesn't > convey a lot about its side effects, maybe delayDismissal or delayedDismiss or > something like that? Done.
ContentViewCore changes lgtm. Would be nice to have a description stating what this does to address the problem. https://codereview.chromium.org/1001553004/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1001553004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:256: protected void attach(long nativeImeAdapter, int textInputType, int textInputFlags, Why protected and not private? Is this called directly in tests?
https://codereview.chromium.org/1001553004/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1001553004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:256: protected void attach(long nativeImeAdapter, int textInputType, int textInputFlags, On 2015/03/12 18:15:40, jdduke wrote: > Why protected and not private? Is this called directly in tests? "Protected" wouldn't make it visible to a test anyway. It just seemed the kind of method that might want to be called by a subclass (not that there currently is one).
https://codereview.chromium.org/1001553004/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1001553004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:256: protected void attach(long nativeImeAdapter, int textInputType, int textInputFlags, On 2015/03/13 13:11:13, bcwhite wrote: > On 2015/03/12 18:15:40, jdduke wrote: > > Why protected and not private? Is this called directly in tests? > > "Protected" wouldn't make it visible to a test anyway. It just seemed the kind > of method that might want to be called by a subclass (not that there currently > is one). A test that subclasses ImeAdapter, which is the only case I could think might want to call this directly. Please make it private.
On 2015/03/13 14:06:52, jdduke wrote: > https://codereview.chromium.org/1001553004/diff/80001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java > (right): > > https://codereview.chromium.org/1001553004/diff/80001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:256: > protected void attach(long nativeImeAdapter, int textInputType, int > textInputFlags, > On 2015/03/13 13:11:13, bcwhite wrote: > > On 2015/03/12 18:15:40, jdduke wrote: > > > Why protected and not private? Is this called directly in tests? > > > > "Protected" wouldn't make it visible to a test anyway. It just seemed the > kind > > of method that might want to be called by a subclass (not that there currently > > is one). > > A test that subclasses ImeAdapter, which is the only case I could think might > want to call this directly. Please make it private. Thanks, still lgtm, let's land this.
On 2015/03/16 15:05:40, jdduke wrote: > On 2015/03/13 14:06:52, jdduke wrote: > > > https://codereview.chromium.org/1001553004/diff/80001/content/public/android/... > > File > > > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java > > (right): > > > > > https://codereview.chromium.org/1001553004/diff/80001/content/public/android/... > > > content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:256: > > protected void attach(long nativeImeAdapter, int textInputType, int > > textInputFlags, > > On 2015/03/13 13:11:13, bcwhite wrote: > > > On 2015/03/12 18:15:40, jdduke wrote: > > > > Why protected and not private? Is this called directly in tests? > > > > > > "Protected" wouldn't make it visible to a test anyway. It just seemed the > > kind > > > of method that might want to be called by a subclass (not that there > currently > > > is one). > > > > A test that subclasses ImeAdapter, which is the only case I could think might > > want to call this directly. Please make it private. > > Thanks, still lgtm, let's land this. Thanks. Still one bot failing that I'm trying to work out.
+boliu: You mentioned a recently proposed patch that caused the WebView GC test to fail. Did that land? We're seeing it fail here: http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r....
https://codereview.chromium.org/1001553004/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1001553004/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:272: mDismissInput = new DelayedDismissInput(nativeImeAdapter); It would be good to verify here that |mDismissInput| is not currently posted to the handler.
On 2015/03/16 15:13:15, jdduke wrote: > +boliu: You mentioned a recently proposed patch that caused the WebView GC test > to fail. Did that land? We're seeing it fail here: > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r.... Yes it did land with a WeakReference. testCreateAndGcManyTimes is not flaky afaik though. Could this just be another change that's breaking it?
Well after a day of head-banging and 100 or so extra lines of debug code... It seems the problem relates to the DelayedDismissInput and it's internal reference to the ImeAdapter preventing the GC from cleaning up the class. Newest patch doesn't schedule a "dismiss" if we're already at keybord-type "none" since the keyboard isn't present or already has a dismiss scheduled.
Definitely need Aurimas or somebody else more familiar with ImeAdapter to take a look here. https://codereview.chromium.org/1001553004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1001553004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:86: private class DelayedDismissInput implements Runnable { Do we still need a class for this? Perhaps we should just use |private final Runnable mDismissInputRunnable| and initialize in the constructor? https://codereview.chromium.org/1001553004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:249: if (textInputType == TextInputType.NONE && mTextInputType == TextInputType.NONE) { Can this conditioanl be one-lined? https://codereview.chromium.org/1001553004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:249: if (textInputType == TextInputType.NONE && mTextInputType == TextInputType.NONE) { It's not immediately obvious why we early out for the TextInputType.NONE case staying the same, but not the other cases? https://codereview.chromium.org/1001553004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:254: mDismissInput = null; No need to null this. https://codereview.chromium.org/1001553004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:259: mDismissInput = new DelayedDismissInput(); I don't believe we need to recreate this every time now? https://codereview.chromium.org/1001553004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:649: if (mDismissInput != null) { Nit: This conditional can be one-lined.
https://codereview.chromium.org/1001553004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1001553004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:86: private class DelayedDismissInput implements Runnable { On 2015/03/17 15:06:05, jdduke wrote: > Do we still need a class for this? > > Perhaps we should just use |private final Runnable mDismissInputRunnable| and > initialize in the constructor? Sure. https://codereview.chromium.org/1001553004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:249: if (textInputType == TextInputType.NONE && mTextInputType == TextInputType.NONE) { On 2015/03/17 15:06:05, jdduke wrote: > It's not immediately obvious why we early out for the TextInputType.NONE case > staying the same, but not the other cases? I considered that but elected to be specific for now. I don't see any reason to not cover all cases so I'll simplify it. https://codereview.chromium.org/1001553004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:254: mDismissInput = null; On 2015/03/17 15:06:05, jdduke wrote: > No need to null this. The object would continue to live, otherwise; might as well release it. But with the static Runnable there's no need for this var at all any longer.
Aurimas? Any new comments after the changes?
lgtm
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org Link to the patchset: https://codereview.chromium.org/1001553004/#ps200001 (title: "address review comments about separate Dismiss class")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1001553004/200001
https://codereview.chromium.org/1001553004/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (left): https://codereview.chromium.org/1001553004/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:228: mHandler.removeCallbacks(mDismissInput); I'm a little worried about removing this and only have it in attach() after a number of early exits. https://codereview.chromium.org/1001553004/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/1001553004/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:273: mIsShowWithoutHideOutstanding = true; Should we remove the delayed dismissal here?
https://codereview.chromium.org/1001553004/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (left): https://codereview.chromium.org/1001553004/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:228: mHandler.removeCallbacks(mDismissInput); The dismissal should only be pending if the current input-type is None. Any change to something else will fall through. With the move, the same function both creates and removes it (as does the companion detach() method) so it's actually easier to follow. Originally the creation of it was in this method.
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/09f6510f4039c4dad8ead7afc62b1d7eff7dd9f3 Cr-Commit-Position: refs/heads/master@{#321200} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
