969929 - GeckEditable loses data ... Seen in TextSelection as regression using action bar SelectALL

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Capella [:capella]]

I'm filing this under the "General" component. The issue is apparent in "TextSelection" component, but seems to originate as a result of processing in /widget/android/nsWindow.cpp, which may be "Keyboards and IME" component.

In either case, we're losing text in a GeckoEditable, and failing to correctly handle selection processing, as a result.

I'm testing with an N7, and the Swift keyboard, though I'm not sure the problem is constrained to this scenario.

Setup the situation pictured here:
   https://www.dropbox.com/s/at8znik68yexzw6/Screenshot_2014-02-08-17-43-28.png

Basic Steps:
   Enter some text in an editable, tap into the middle of the text to attachCaret() and expose the actionBar.
   Notice part of the text |sele| is underlined by IME composition/styling (?terminology?).
   Press the actionbar SelectAll icon

Observe the resulting error here:
   https://www.dropbox.com/s/cq1r8x3qy10v3ny/Screenshot_2014-02-08-17-45-49.png

Comment 1

[Mark Capella [:capella]]

What happens is commented:

// User presses the actionbar SelectAll icon ...
  .....
  GeckoEditable: onSelectionChange() Starts
  GeckoEditable: newSetSelection() Starts
  GeckoEditable: ActionQueue.offer() Starts
  GeckoEditable: geckoActionReply() Starts
  GeckoEditable: geckoActionReply() Selection.getSelectionEnd(mText) [ 4]

  GeckoIinputConnection: onSelectionChange() STARTS
  GeckoIinputConnection: notifySelectionChange() STARTS
  GeckoIinputConnection: notifySelectionChange()    imm.updateSelection() from [ 0 , 17 ]
  GeckoIinputConnection: notifySelectionChange()    imm.updateSelection() to   [ 0 , 4 ]
  GeckoIinputConnection: notifySelectionChange() RETURNS
  GeckoIinputConnection: onSelectionChange() RETURNS
  GeckoIinputConnection: getExtractedText() STARTS

  GeckoEditable: setUpdateGecko(false) Starts
  GeckoEditable: setUpdateGecko()    Avoids a call to icUpdateGecko

  GeckoIinputConnection: getExtractedText() STARTS

  GeckoEditable: setUpdateGecko(false) Starts
  GeckoEditable: setUpdateGecko()    Avoids a call to icUpdateGecko
  GeckoEditable: removeSpan() Starts
  GeckoEditable: ActionQueue.offer() Starts
  GeckoEditable: geckoActionReply() Starts
  GeckoEditable: removeSpan() Starts
  GeckoEditable: ActionQueue.offer() Starts
  GeckoEditable: geckoActionReply() Starts

  GeckoIinputConnection: endBatchEdit() STARTS
  GeckoIinputConnection: endBatchEdit() RETURNS

  GeckoIinputConnection: endBatchEdit() STARTS
  GeckoEditable: setUpdateGecko(true) Starts
  GeckoEditable: setUpdateGecko()    Calls icUpdateGecko
  GeckoEditable: icUpdateGecko() STARTS
  GeckoEditable: icUpdateGecko()    selEnd [ 17 ]
  GeckoEditable: icUpdateGecko()    createIMESelectEvent() [ 0 , 17 ]
  GeckoEditable: icUpdateGecko() RETURNS ...   
  GeckoIinputConnection: endBatchEdit() RETURNS

// Seems to be critical ...
  nsWindow::OnIMEEvent("IME_SET_SELECTION") STARTS
  nsWindow::OnIMEEvent("IME_SET_SELECTION")    Calling RemoveIMEComposition() ... STARTS

    nsWindow::RemoveIMEComposition() STARTS
    nsWindow::RemoveIMEComposition()    CONTINUES mIMEComposingText.length() : [ 4 ]
    nsWindow::RemoveIMEComposition()    DispatchEvent(&textEvent);

    // Triggers notifySelectionChanged() in SelectionHandler.js ...
      SelectionHandler: notifySelectionChanged() STARTS
      SelectionHandler: notifySelectionChanged()    aDocument:                 [ [object XrayWrapper [object HTMLDocument]] ])
      SelectionHandler: notifySelectionChanged()    aSelection:                [  ])
      SelectionHandler: notifySelectionChanged()    aReason:                   [ 0 ])
      SelectionHandler: notifySelectionChanged()    this._targetElement:       [ [object XrayWrapper [object HTMLInputElement]] ])

    // Because |nsISelection| is lost ...
      SelectionHandler: _debugSelection()           aSelection.toString()      []
      SelectionHandler: _debugSelection()           aSelection.isCollapsed     [true]

    // And text value of GeckoEditable is *changed* ...
      SelectionHandler: notifySelectionChanged()    this._targetElement.value: [ sele ])

    // So we ...
      SelectionHandler: _closeSelection() STARTS
      SelectionHandler: _clearSelection() STARTS
      SelectionHandler: _deactivate() STARTS
      SelectionHandler: notifySelectionChanged() RETURNS ... selection no longer exists ?????????????????????????
      SelectionHandler: notifySelectionChanged() RETURNS ... after doing NOTHING ...

    nsWindow::RemoveIMEComposition()    DispatchEvent(WidgetCompositionEvent( NS_COMPOSITION_END ));
    nsWindow::RemoveIMEComposition() RETURNS

  nsWindow::OnIMEEvent("IME_SET_SELECTION")    Calling RemoveIMEComposition() ... FINISHES
  nsWindow::OnIMEEvent("IME_SET_SELECTION")   Notices: start [ 0 ] end [ 17 ]
  nsWindow::OnIMEEvent("IME_SET_SELECTION")   Notices: selEvent.mOffset   [ 0 ]
  nsWindow::OnIMEEvent("IME_SET_SELECTION")   Notices: selEvent.mLength   [ 17 ]
  nsWindow::OnIMEEvent("IME_SET_SELECTION")   Notices: selEvent.mReversed [ 0 ]
  nsWindow::OnIMEEvent("IME_SET_SELECTION") RETURNS ...

Comment 2

[Mark Capella [:capella]]

mmm ... This issue is also present in Aurora / v29.0a2

Comment 3

[Mark Capella [:capella]]

wesj: I wasn't aware that you had knowledge of the IME side of the code. I'd asked jchen a couple days ago to help me with this, as I think that while this bug is demonstrable using STR involving TextSelection, the actual cause is data loss somehow occurring in nsWindow.cpp.

If you play this video using say VLC and do a frame-by-frame step:
   https://www.dropbox.com/s/modh38oukt2bylj/20140214_045827.mp4

You can see that after the user presses the selectAll key, SelectionHandler actually does it's job, fleetingly selecting all text and showing the UI handles.

But immediately after this, something causes the portion of the text from the original cursor position forward to be lost, in this critical shot:
   https://www.dropbox.com/s/5tgggodibuwqo1i/Screenshot%20from%202014-02-14%2005%3A11%3A50.png

I believe the IME code in nsWindow::RemoveIMEComposition() is responsible, perhaps trying to redraw the target text without the autoCorrect/autoSuggestion styling/underline highlight, that is visible on the screen at the start of this new example to the left of the original caret position under |lostDataThatYou|.

And that's where my knowledge of the IME code fails me. I know it does things under the covers to perform on-the-fly autoSuggestion-styling effects that it masks from being heard by Java.

Somehow in this case it's seeming to lose data on it's own, or being interrupted in-process.

Comment 4

[Mark Capella [:capella]]

Status Update:

Using the pictured example:
   https://www.dropbox.com/s/5tgggodibuwqo1i/Screenshot%20from%202014-02-14%2005%3A11%3A50.png

SelectionHandler.js has selected all the text in the editable, including the portion of the string that was
   |IME_RANGE_LINE_SOLID| ("loseDataThatYou" underlined).

This block of code kicks in :
   http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?mark=1802-1803#1790


nsWindow::OnGlobalAndroidEvent(IME_EVENT)
nsWindow::OnIMEEvent() STARTS
nsWindow::OnIMEEvent("IME_SET_SELECTION") STARTS
nsWindow::OnIMEEvent("IME_SET_SELECTION")    Calling RemoveIMEComposition() ... STARTS

   nsWindow::RemoveIMEComposition() STARTS
   nsWindow::RemoveIMEComposition()    Sets SelectionUpdate mask
   nsWindow::RemoveIMEComposition()    Sets TextUpdate mask
   nsWindow::RemoveIMEComposition()    CONTINUES mIMEComposingText.length() : [ 15 ]
   nsWindow::RemoveIMEComposition()    Dispatchs textEvent ( NS_TEXT_TEXT ) Starts

      nsWindow::DispatchEvent() Starts
      nsWindow::DispatchEvent()    Calls Event Handler Starts
      nsEventStateManager::PreHandleEvent("NS_TEXT_TEXT") Starts ...
      nsEventStateManager::PreHandleEvent("NS_TEXT_TEXT") Finishes ...

      // Collapse nsISelection and remove text "loseDataThatYouWereNotExpecting"
      nsWindow::NotifyIMEOfTextChange() STARTS [ start , aOldEnd , aNewEnd ] : [ 0 , 31 , 0 ]
      nsWindow::NotifyIMEOfTextChange() FINISHES ... IT WAS A MASKED TEXT UPDATE

      // Replace with "loseDataThatYou"
      nsWindow::NotifyIMEOfTextChange() STARTS [ start , aOldEnd , aNewEnd ] : [ 0 , 0 , 15 ]
      nsWindow::NotifyIMEOfTextChange() FINISHES ... IT WAS A MASKED TEXT UPDATE

      nsWindow::DispatchEvent()    Calls Event Handler Finishes
      nsWindow::DispatchEvent()    Was: NS_TEXT_TEXT
      nsWindow::DispatchEvent() Returns ...

   nsWindow::RemoveIMEComposition()    Dispatchs WidgetCompositionEvent ( NS_COMPOSITION_END ) Starts
      nsWindow::DispatchEvent() Starts
      nsWindow::DispatchEvent()    Calls Event Handler Starts
      nsWindow::DispatchEvent()    Calls Event Handler Finishes
      nsWindow::DispatchEvent()    Was: NS_COMPOSITION_END
      nsWindow::DispatchEvent() Returns ...
      nsWindow::RemoveIMEComposition()    Dispatchs WidgetCompositionEvent ( NS_COMPOSITION_END ) Finishes

   nsWindow::RemoveIMEComposition() RETURNS

nsWindow::OnIMEEvent("IME_SET_SELECTION")    Calling RemoveIMEComposition() ... FINISHES
nsWindow::OnIMEEvent("IME_SET_SELECTION")   Notices: start [ 0 ] end [ 31 ]
nsWindow::OnIMEEvent("IME_SET_SELECTION")   Notices: selEvent.mOffset   [ 0 ]
nsWindow::OnIMEEvent("IME_SET_SELECTION")   Notices: selEvent.mLength   [ 31 ]
nsWindow::OnIMEEvent("IME_SET_SELECTION")   Notices: selEvent.mReversed [ 0 ]
   nsWindow::DispatchEvent() Starts
   nsWindow::DispatchEvent()    Calls Event Handler Starts
   nsWindow::DispatchEvent()    Calls Event Handler Finishes
   nsWindow::DispatchEvent() Returns ...

nsWindow::OnIMEEvent("IME_SET_SELECTION") RETURNS ...

Comment 5

[Mark Capella [:capella]]

Attachment: bug969929.diff

Now that I know how the IME composition code is getting confused, I see that after we complete bug 927882, we can layer on this simple change to fix this bug.

Basically, we force the IME code to clear/reset previous composition/stylings before we do our selectALL, instead of having it try to reset state after we've further invalidated it.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8376851 [details] [diff] [review]
bug969929.diff

Review of attachment 8376851 [details] [diff] [review]:
-----------------------------------------------------------------

Lets try this.

Comment 7

[Mark Capella [:capella]]

heheh ... lotsa work for a one-liner  :-P

https://hg.mozilla.org/integration/fx-team/rev/415e5d63cc33

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/415e5d63cc33

Comment 9

[Aaron Train [:aaronmt]]

Since this caused dataloss in my bug 956571 which this fixed, is this safe enough for 28?

Comment 10

[Mark Capella [:capella]]

This one-line change should be able to be pushed all the way to release/FF27 without any negative affect.

That being said, I don't believe there's an issue until IME composition / underline styling change(s) have been introduced, which is what seemed to cause the fuss. The video in bug 956571 is no longer available, so I can't confirm what was seen there (was underlining detectable?).

My local beta build FF28 does not show the issue.

Comment 11

[Aaron Train [:aaronmt]]

(In reply to Mark Capella [:capella] from comment #10)
> This one-line change should be able to be pushed all the way to release/FF27
> without any negative affect.
> 
> That being said, I don't believe there's an issue until IME composition /
> underline styling change(s) have been introduced, which is what seemed to
> cause the fuss. The video in bug 956571 is no longer available, so I can't
> confirm what was seen there (was underlining detectable?).
> 
> My local beta build FF28 does not show the issue.

I reposted in the same bug https://www.youtube.com/watch?v=6CyQfZ29ir0

Comment 12

[Mark Capella [:capella]]

Ah! So you did (my mistake)...

So testing on my Asus N7 w/stock Google keyboard w/local Beta/FF28 build, I can repro this issue, and by applying this patch, I can correct it.

Moving the patch to FF28 looks strongly suggested.

Comment 13

[Mark Capella [:capella]]

Comment on attachment 8376851 [details] [diff] [review]
bug969929.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): autoCorrection / autoSuggestion sytling / soft keyboards / IME.
User impact if declined: Visible loss of keyed data in circumstances involving "selectALL" text Selection.
Testing completed (on m-c, etc.): m-c/30 m-a/29 m-b/28
Risk to taking this patch (and alternatives if risky): Extremely low, one-line backout plan.
String or IDL/UUID changes made by this patch: none.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f565ffd596ef
https://hg.mozilla.org/releases/mozilla-beta/rev/27e269db5ea7

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/27e269db5ea7