While testing various text Selection scenarios for bug 988143 using: https://www.dropbox.com/s/j5jcon30y8vda3u/test_bug988143.html?dl=0 I find I can crash Fennec in GeckoEditable, from Release (36.0.4 for me) back to Nightly. I'm filing this under "Text Selection" component, though it doesn't directly involve our SelectionHandler/TextSelection UI code. It really falls into that middle area where core Gecko nsISelection meets the IME/Keyboard component where I normally work with jchen (widget/android/nsWindow.cpp, etc). (cc:ed jchen, and innocent bystander mike taylor 'cause he's looked at SelectionHandler and contentEditables and might be interested). STR: (1) Open the test case above. (2) Tap into the contentEditable <p> at the bottom of the screen to display the blinking nsCaret in its editor. (look for the TAP ME TAP ME TAP ME text) (3) Long press the text in the non-editable <p> above it and wait a second. (look for the LONGPRESS ME LONGPRESS ME LONGPRESS ME text) You should observe a crash with the logcat signature: E/GeckoEditable( 4066): invalid selection notification range: 2803 to 2812, length: 568 E/GeckoCrashHandler( 4066): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 25152 ("Gecko") E/GeckoCrashHandler( 4066): java.lang.IllegalArgumentException: invalid selection notification range E/GeckoCrashHandler( 4066): at org.mozilla.gecko.GeckoEditable.onSelectionChange(GeckoEditable.java:825) E/GeckoCrashHandler( 4066): at org.mozilla.gecko.GeckoAppShell.notifyIMEChange(GeckoAppShell.java:482) E/GeckoCrashHandler( 4066): at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method) E/GeckoCrashHandler( 4066): at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method) E/GeckoCrashHandler( 4066): at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:371) E/GeckoCrashHandler( 4066): at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:184) E/GeckoCrashHandler( 4066): Main thread (1) stack: E/GeckoCrashHandler( 4066): java.lang.reflect.AccessibleObject.<init>(AccessibleObject.java:103) E/GeckoCrashHandler( 4066): java.lang.reflect.Field.<init>(Field.java:94) E/GeckoCrashHandler( 4066): java.lang.Class.getDeclaredField(Native Method) E/GeckoCrashHandler( 4066): java.lang.Class.getPublicFieldRecursive(Class.java:732) E/GeckoCrashHandler( 4066): java.lang.Class.getField(Class.java:722) E/GeckoCrashHandler( 4066): org.mozilla.gecko.gfx.BitmapUtils.getResource(BitmapUtils.java:406) E/GeckoCrashHandler( 4066): org.mozilla.gecko.gfx.BitmapUtils.getDrawable(BitmapUtils.java:132) E/GeckoCrashHandler( 4066): org.mozilla.gecko.TextSelection$TextSelectionActionModeCallback.onPrepareActionMode$1451e89d(TextSelection.java:279) E/GeckoCrashHandler( 4066): org.mozilla.gecko.ActionModeCompat.invalidate(ActionModeCompat.java:86) E/GeckoCrashHandler( 4066): org.mozilla.gecko.BrowserApp.startActionModeCompat(BrowserApp.java:3311) E/GeckoCrashHandler( 4066): org.mozilla.gecko.TextSelection.access$800(TextSelection.java:36) E/GeckoCrashHandler( 4066): org.mozilla.gecko.TextSelection$2.run(TextSelection.java:156) E/GeckoCrashHandler( 4066): android.os.Handler.handleCallback(Handler.java:733) E/GeckoCrashHandler( 4066): android.os.Handler.dispatchMessage(Handler.java:95) E/GeckoCrashHandler( 4066): android.os.Looper.loop(Looper.java:136) E/GeckoCrashHandler( 4066): android.app.ActivityThread.main(ActivityThread.java:5001) E/GeckoCrashHandler( 4066): java.lang.reflect.Method.invokeNative(Native Method) E/GeckoCrashHandler( 4066): java.lang.reflect.Method.invoke(Method.java:515) E/GeckoCrashHandler( 4066): com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785) E/GeckoCrashHandler( 4066): com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601) E/GeckoCrashHandler( 4066): dalvik.system.NativeStart.main(Native Method)

Yep, I can reproduce 100% of the time from Stable to Nightly too (phew, not my fault >_<).

Thank you! Having a 100% testcase makes things 100% better! :)

So the crash seems to be a mixture of text selection bug and IME bug. On the text selection side, when you long tap on some text outside of an editor, the outside text is selected *without* the focus actually leaving the editor, i.e. the editor never receives a blur event. On the IME side, because the editor is still "focused", IME code receives a selection change event and assumes the selection is somewhere inside the editor, but it finds an error when the actual selection offsets (outside of the editor) don't agree with the text length inside the editor. I have a patch to fix the IME side, so that we ignore situations like this. I'm looking at the text selection side now to actually defocus the editor when you long tap outside of it.

Is this something to be adddressed on the Gecko/Core side? Or are you looking at a patch to SelectionHandler.js? After we switch mobile to the new Gecko Touch/Seelction carets (bug 988143) SelectionHandler.js will be replaced by ActionBarHandler.js. I'm trying to avoid putting anything too-specific / hacky in there that actively compensates for nsISelection/IME interactions, but instead is more process agnostic. That said, there already is a compensating piece in the new module where I have to blur/re-focus an element prior to performing a SelectAll() function on user request if there is an active IME composition in the editable at the time, so I don't know how far we can go to avoid these things.

Yeah I'm looking at SelectionHandler.js right now, more of a stopgap since it's going away in the future.

This patch makes us ignore change notificaions that come from outside of the editor. This prevents us from crashing, but doesn't fix the underlying issue in SelectionHandler.js.

Nice! I tested this out with our current SelectionHandler and our "in-progress" ActionBarHandler and it works as expected with both! I'm not sure how to parse your comment re: "doesn't fix the underlying issue in SelectionHandler.js." ... if you mean you didn't have to touch SelectionHandler, then \o/ ! If you mean, there's something outstanding left to fix in SelectionHandler, I'd need more information because I don't see any issue. This seems perfect :-)

This seems to work. It clears any existing focus before we start a new selection, so that we don't change the selection to outside of an editor while the editor is still focused.

Sorry, IMO "the underlying issue" is that SelectionHandler tends to change selections without changing the focus as well, so you end up in situations where it changed selection to outside of an editor, while the focus is still inside the editor. The focus is the element that key events, for example, are delivered to. The selection and focus are related concepts, but they are implemented separately in Gecko.

Comment on attachment 8586896 [details] [diff] [review] Ignore IME notifications outside of the focused editor (v1) Review of attachment 8586896 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. We still need to fix the actual selection, do you want to handle it this bug, as it does seem to be the other side of this issue? ::: widget/android/nsWindow.cpp @@ +2205,5 @@ > + nsCOMPtr<nsISelection> imeSelection; > + nsCOMPtr<nsIContent> imeRoot; > + > + // If we are receiving notifications, we must have selection/root content. > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(IMEStateManager::GetFocusSelectionAndRoot( Is there a reason to continue without a root on release builds? Why not use NS_ENSURE_TRUE_VOID instead? Also, missing include for IMEStateManager.

Comment on attachment 8587011 [details] [diff] [review] Clear existing focus when closing selection (v1) Review of attachment 8587011 [details] [diff] [review]: ----------------------------------------------------------------- Ah! Odd I never noticed that detail :/

(In reply to Eugen Sawin [:esawin] from comment #10) > Comment on attachment 8586896 [details] [diff] [review] > Ignore IME notifications outside of the focused editor (v1) > > + > > + // If we are receiving notifications, we must have selection/root content. > > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(IMEStateManager::GetFocusSelectionAndRoot( > > Is there a reason to continue without a root on release builds? Why not use > NS_ENSURE_TRUE_VOID instead? I think I prefer MOZ_ALWAYS_TRUE because it acts as an assertion in debug builds, so it's more strict than NS_ENSURE_TRUE_VOID. We're saying that this statement must succeed.

(In reply to Jim Chen [:jchen] from comment #12) > I think I prefer MOZ_ALWAYS_TRUE because it acts as an assertion in debug > builds, so it's more strict than NS_ENSURE_TRUE_VOID. We're saying that this > statement must succeed. I agree that it's a stronger statement, but MOZ_ALWAYS_TRUE doesn't return on failure on release. I think we actually want to assert or warn and return here.

So if we know it will always succeed, it's okay if it doesn't return on failure. In other words, we are asserting that it will never fail, so it's not necessary to handle the failure case. I believe that assertion is valid. Of course, if that assertion is not valid, if it does fail sometimes, then we should handle the failure case. But I think I'm just nitpicking now :) There's really not much difference either way.

https://hg.mozilla.org/integration/fx-team/rev/da2d02fa2472 https://hg.mozilla.org/integration/fx-team/rev/fb454c045bae

https://hg.mozilla.org/mozilla-central/rev/da2d02fa2472 https://hg.mozilla.org/mozilla-central/rev/fb454c045bae

Comment on attachment 8586896 [details] [diff] [review] Ignore IME notifications outside of the focused editor (v1) Approval Request Comment [Feature/regressing bug #]: N/A [User impact if declined]: Occasional crash in all current versions when selecting text [Describe test coverage new/current, TreeHerder]: Locally, m-c [Risks and why]: Very small; patch only adds a safety check to make sure we don't run into the error condition [String/UUID change made/needed]: None

Since this has steps to reproduce let's see how it fares in manual testing on Nightly before uplifting it. Ioana would you mind taking a look at this to verify the fix on 40?

I'm not sure what the right account is for Ioana. Setting ni on a different account.

Comment on attachment 8586896 [details] [diff] [review] Ignore IME notifications outside of the focused editor (v1) This fix has been on m-c for 14 days now. Let's get the fix onto Aurora. We can verify on Aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/83dadcb2d604 https://hg.mozilla.org/releases/mozilla-aurora/rev/a5eb17c5f04a

Looks like we need bug 1149172 on Aurora to prevent Android build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=781801&repo=mozilla-aurora

Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/ddebce1c80aa for now until the dependencies get sorted out.

Here's the patch for aurora that avoids the dependency by including an extra header. Can you uplift this one, Ryan?

Jim, we would probably want this uplifted to beta/release, too, since bug 1051556 is tracking 38.

I guess this is tracking 39. I feel better riding out the train on this one.

Verified as fixed on Firefox 42 Beta 1