805162 - Rework IME code to properly handle events and notifications across threads

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: WIP patch

We use a separate class, GeckoEditable, to properly marshal events and notifications back and forth between the Gecko thread and UI thread.

So far, the WIP patch fixes at least Bug 769520, Bug 770291, and Bug 751513.

Comment 1

[Chris Peterson [:cpeterson]]

Comment on attachment 674810 [details] [diff] [review]
WIP patch

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

* I'm a little nervous about the synchronized methods on GeckoInputConnection and GeckoEditable. I thought the UI thread was going to maintain its own shadow copy of the Gecko thread's text? Blocking either the Gecko or UI thread is probably a bad idea. However, blocking may be an acceptable trade-off if we can encapsulate all the shared state into one synchronized object to avoid queuing update messages between threads. 

* Why does GeckoEditable need a Proxy object and reflection? This code could use some more comments explaining why we need a Proxy.

* Rather than using a gecko-/ui- naming-convention for GeckoEditable methods, I would suggest splitting off the Gecko and UI methods into their own subclasses or interfaces, so each thread only has a reference that exposes the methods safe for that thread.

::: mobile/android/base/GeckoEditable.java
@@ +14,5 @@
> +import android.text.InputFilter;
> +import android.text.Selection;
> +import android.text.style.UnderlineSpan;
> +import android.text.style.ForegroundColorSpan;
> +import android.text.style.BackgroundColorSpan;

Alphabetize imports in each section (but keep android.* and java.* sections separate like you have).

@@ +27,5 @@
> +   GeckoEditable implements only some functions of Editable
> +   The field mInner contains the actual underlying
> +   SpannableStringBuilder/Editable that contains our text.
> +*/
> +class GeckoEditable

Can GeckoEditable be a final class?

@@ +43,5 @@
> +       being sent to the Gecko thread, the action stays on top of mActions
> +       queue. After the Gecko event is processed and replied, the action
> +       is removed from the queue
> +    */
> +    private static class Action {

Can Action be a final class?

@@ +98,5 @@
> +
> +    private final SpannableStringBuilder mInner;
> +    private final Editable mProxy;
> +
> +    /* Queue of text replacement ections sent to Gecko thread that

s/ections/actions/?

@@ +107,5 @@
> +    private int mSavedSelectionStart;
> +    private int mSelectionSeqno;
> +    private int mCompositionSeqno;
> +    private int mLastUpdatedCompositionSeqno;
> +    private boolean mUpdateComposition;

Consolidate all instance variables at the top of the class definition.

@@ +130,5 @@
> +
> +    private void uiSyncWithGecko() {
> +        if (!mActions.isEmpty()) {
> +            mActionsActive.acquireUninterruptibly();
> +            mActionsActive.release();

It looks like you are using the mActionsActive semaphore like a condition variable. You might consider replacing the ConcurrentLinkedQueue and Semaphore with an ArrayDeque Queue, Lock, and Condition.

@@ +134,5 @@
> +            mActionsActive.release();
> +        }
> +    }
> +
> +    private static void geckoPostToUI(Runnable runnable) {

Rather than using a gecko-/ui- naming-convention for GeckoEditable methods, I would suggest creating separate subclasses or interfaces for the Gecko and UI methods, so each thread only has a reference that exposes the methods safe for that thread.

@@ +164,5 @@
> +        ++mCompositionSeqno;
> +    }
> +
> +    private void geckoRemoveAction() {
> +        assert !mActions.isEmpty();

You should replace assert with a real exception because the Dalvik VM does not enable assertions by default, so no one is going to test this condition.

@@ +191,5 @@
> +        uiAddAction(new Action(Action.TYPE_EVENT));
> +    }
> +
> +    void geckoOnActionReply() {
> +        assert !mActions.isEmpty();

Replace assert with a real exception.

@@ +560,5 @@
> +
> +    public <T> T[] getSpans(int start, int end, Class<T> type) {
> +        throw new UnsupportedOperationException();
> +    }
> +    

Remove trailing whitespace.

::: mobile/android/base/GeckoEvent.java
@@ +451,5 @@
>          event.mLocation = l;
>          return event;
>      }
>  
> +    public static GeckoEvent createIMEEvent(int action) {

With your GeckoInputConnection.java changes, can you make createIMEEvent() a private method?

::: mobile/android/base/GeckoInputConnection.java
@@ +168,5 @@
> +                    getEditable().clear();
> +                } else {
> +                    GeckoAppShell.setClipboardText(text.substring(
> +                            Math.min(selStart, selEnd),
> +                            Math.max(selStart, selEnd)));

You don't need this min/max code if you initialize selStart/selEnd with GeckoInputConnection's getSelection() utility method.

::: mobile/android/base/Makefile.in
@@ +77,5 @@
>    GeckoConnectivityReceiver.java \
>    GeckoEvent.java \
>    GeckoHalDefines.java \
>    GeckoInputConnection.java \
> +  GeckoEditable.java \

Alphabetize list by filename.

::: widget/android/nsWindow.cpp
@@ +162,5 @@
>      mParent(nullptr),
>      mFocus(nullptr),
> +    mIMEComposing(false),
> +    mIMEMaskSelectionUpdate(false),
> +    mIMEMaskTextUpdate(false)

I found these "mIMEMask*Update" variable names a little confusing. I would suggest renaming them to something like "mMaskingIME*Updates".

@@ +640,5 @@
>          nsEventStatus status = mWidgetListener->HandleEvent(aEvent, mUseAttachedEvents);
>  
>          switch (aEvent->message) {
>          case NS_COMPOSITION_START:
>              mIMEComposing = true;

This may be outside the scope of your change, but can we MOZ_ASSERT(!mIMEComposing)?

@@ +643,5 @@
>          case NS_COMPOSITION_START:
>              mIMEComposing = true;
>              break;
>          case NS_COMPOSITION_END:
>              mIMEComposing = false;

This may be outside the scope of your change, but can we MOZ_ASSERT(mIMEComposing)?

@@ +649,3 @@
>              break;
>          case NS_TEXT_TEXT:
>              mIMEComposingText = static_cast<nsTextEvent*>(aEvent)->theText;

This may be outside the scope of your change, but can we MOZ_ASSERT(mIMEComposing)?

@@ +1820,2 @@
>  void
> +nsWindow::RemoveIMEComposition(void)

`(void)` is redundant; you only need `()`.

@@ +1824,5 @@
> +    bool savedMaskText = mIMEMaskTextUpdate;
> +    mIMEMaskSelectionUpdate = mIMEMaskTextUpdate = true;
> +
> +    // Remove composition on Gecko side
> +    if (mIMEComposing) {

You don't need to save the mask flags if !mIMEComposing. I would suggest moving this `if` to the top of the function and just returning early if !mIMEComposing.

@@ +1872,5 @@
> +
> +                Text updates are passed on, so the Java text can shadow the
> +                  Gecko text
> +            */
> +            mIMEMaskSelectionUpdate = true;

Can you MOZ_ASSERT(!mIMEMaskSelectionUpdate)? Managing multiple boolean flags can make for a confusing state machine. Asserting will help keep us honest. :)

@@ +1900,5 @@
> +                event.data = ae->Characters();
> +                DispatchEvent(&event);
> +            }
> +            AndroidBridge::NotifyIME(AndroidBridge::NOTIFY_IME_REPLY_EVENT, 0);
> +            mIMEMaskSelectionUpdate = false;

Can you MOZ_ASSERT(mIMEMaskSelectionUpdate)?

@@ +1911,5 @@
>  
> +                Selection updates are masked to prevent Java from being
> +                  notified of the new selection
> +            */
> +            mIMEMaskSelectionUpdate = true;

Can you MOZ_ASSERT(!mIMEMaskSelectionUpdate)?

@@ +1922,5 @@
> +                nsQueryContentEvent event(true, NS_QUERY_SELECTED_TEXT, this);
> +                InitEvent(event, nullptr);
> +                DispatchEvent(&event);
> +                NS_ASSERTION(event.mSucceeded && !event.mWasAsync,
> +                            "IME: cannot get selection");

New code should use MOZ_ASSERT(), not NS_ASSERTION(). MOZ_ASSERT() is fatal (in debug builds). NS_ASSERTION() just logs a warning to logcat.

@@ +1931,5 @@
> +                    end = int32_t(event.GetSelectionEnd());
> +            }
> +
> +            selEvent.mOffset = PR_MIN(start, end);
> +            selEvent.mLength = PR_MAX(start, end) - selEvent.mOffset;

New code should use std::min/max(), not PR_MIN/PR_MAX() or NS_MIN/NS_MAX(), as per bug 786533.

@@ +1936,5 @@
> +            selEvent.mReversed = start > end;
> +            selEvent.mExpandToClusterBoundary = false;
> +
> +            DispatchEvent(&selEvent);
> +            mIMEMaskSelectionUpdate = false;

Can you MOZ_ASSERT(mIMEMaskSelectionUpdate)?

@@ +1944,4 @@
>          {
> +            nsTextRange range;
> +            range.mStartOffset = ae->Start();
> +            range.mEndOffset = ae->End(); 

Remove trailing whitespace.

@@ +1956,5 @@
> +            range.mRangeStyle.mBackgroundColor = NS_RGBA(
> +                ((ae->RangeBackColor() >> 16) & 0xff),
> +                ((ae->RangeBackColor() >> 8) & 0xff),
> +                (ae->RangeBackColor() & 0xff),
> +                ((ae->RangeBackColor() >> 24) & 0xff));

You should consolidate this copy/pasted bit shifting code into a utility function like `nscolor ConvertGBARToRGBA()`.

@@ +1973,5 @@
> +
> +                Selection and text updates are masked so the result of
> +                  temporary events are not passed on to Java
> +            */
> +            mIMEMaskSelectionUpdate = mIMEMaskTextUpdate = true;

Can you MOZ_ASSERT(!mIMEMaskSelectionUpdate) and MOZ_ASSERT(!mIMEMaskTextUpdate)?

@@ +1996,5 @@
> +                        NS_QUERY_SELECTED_TEXT, this);
> +                InitEvent(queryEvent, nullptr);
> +                DispatchEvent(&queryEvent);
> +                NS_ASSERTION(queryEvent.mSucceeded && !queryEvent.mWasAsync,
> +                            "IME: cannot get selection");

s/NS_ASSERTION/MOZ_ASSERT/

@@ +2026,5 @@
>  #endif // DEBUG_ANDROID_IME
>  
>              DispatchEvent(&event);
>              mIMERanges.Clear();
> +            mIMEMaskSelectionUpdate = mIMEMaskTextUpdate = false;

Can you MOZ_ASSERT(mIMEMaskSelectionUpdate) and MOZ_ASSERT(mIMEMaskTextUpdate)?

@@ +2149,5 @@
>      AndroidBridge::NotifyIME(AndroidBridge::NOTIFY_IME_FOCUSCHANGE, 
>                               int(aFocus));
>  
>      if (aFocus) {
> +        OnIMETextChange(0, INT32_MAX, INT32_MAX);

Why are you passing INT32_MAX when the function parameters are uint32_t?

::: widget/android/nsWindow.h
@@ +165,5 @@
>      nsWindow *FindTopLevel();
>      bool DrawTo(gfxASurface *targetSurface);
>      bool DrawTo(gfxASurface *targetSurface, const nsIntRect &aRect);
>      bool IsTopLevel();
> +    void RemoveIMEComposition(void);

`(void)` is redundant; you only need `()`.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Thanks for the comments! Real patches coming soon...

> * I'm a little nervous about the synchronized methods on
> GeckoInputConnection and GeckoEditable. I thought the UI thread was going to
> maintain its own shadow copy of the Gecko thread's text? Blocking either the
> Gecko or UI thread is probably a bad idea. However, blocking may be an
> acceptable trade-off if we can encapsulate all the shared state into one
> synchronized object to avoid queuing update messages between threads.

Per my email, Gecko thread maybe block during geckoRemoveAction() if UI thread is running within the synchronized block in uiAddAction(), which should not take long to run. UI thread may block for Gecko to catch up (since UI thread depends on Gecko thread for the latest information)

> * Why does GeckoEditable need a Proxy object and reflection? This code could
> use some more comments explaining why we need a Proxy.

Proxy is used to have a cleaner way to synchronize with Gecko whenever a query method is called on Editable. It also provide a nice place to log all calls during debugging.

> * Rather than using a gecko-/ui- naming-convention for GeckoEditable
> methods, I would suggest splitting off the Gecko and UI methods into their
> own subclasses or interfaces, so each thread only has a reference that
> exposes the methods safe for that thread.

* GeckoAppShell now only has access to GeckoEditableListener which GeckoAppShell uses to inform GeckoEditable of notifications from Gecko.
* GeckoInputConnection now only has access to GeckoEditableClient which exposes only the methods that GeckoInputConnection needs.
* GeckoInputConnection also implements GeckoEditableListener. GeckoEditable takes notifications from GeckoAppShell, posts them to the UI thread, then calls the GeckoEditableListener methods on GeckoInputConnection.
* This way, GeckoAppShell never deals with the UI thread, and GeckoInputConnection never deals with the Gecko thread.

> @@ +130,5 @@
> > +
> > +    private void uiSyncWithGecko() {
> > +        if (!mActions.isEmpty()) {
> > +            mActionsActive.acquireUninterruptibly();
> > +            mActionsActive.release();
> 
> It looks like you are using the mActionsActive semaphore like a condition
> variable. You might consider replacing the ConcurrentLinkedQueue and
> Semaphore with an ArrayDeque Queue, Lock, and Condition.

Is there an advantage to using Lock and Condition?

> >  
> > +    public static GeckoEvent createIMEEvent(int action) {
> 
> With your GeckoInputConnection.java changes, can you make createIMEEvent() a
> private method?

We still call createIMEEvent in some places.

> > +                    getEditable().clear();
> > +                } else {
> > +                    GeckoAppShell.setClipboardText(text.substring(
> > +                            Math.min(selStart, selEnd),
> > +                            Math.max(selStart, selEnd)));
> 
> You don't need this min/max code if you initialize selStart/selEnd with
> GeckoInputConnection's getSelection() utility method.

I got rid of getSelection() :p

> ::: widget/android/nsWindow.cpp
> @@ +162,5 @@
> >      mParent(nullptr),
> >      mFocus(nullptr),
> > +    mIMEComposing(false),
> > +    mIMEMaskSelectionUpdate(false),
> > +    mIMEMaskTextUpdate(false)
> 
> I found these "mIMEMask*Update" variable names a little confusing. I would
> suggest renaming them to something like "mMaskingIME*Updates".

Hmm how do you find "mIMEMask*Update" confusing? I guess I was trying to match the other mIME* variables.

> >          switch (aEvent->message) {
> >          case NS_COMPOSITION_START:
> >              mIMEComposing = true;
> 
> This may be outside the scope of your change, but can we
> MOZ_ASSERT(!mIMEComposing)?

Done. I think everything should be within the scope of this bug :)

> @@ +2149,5 @@
> >      AndroidBridge::NotifyIME(AndroidBridge::NOTIFY_IME_FOCUSCHANGE, 
> >                               int(aFocus));
> >  
> >      if (aFocus) {
> > +        OnIMETextChange(0, INT32_MAX, INT32_MAX);
> 
> Why are you passing INT32_MAX when the function parameters are uint32_t?

Because they become int32 on the Java side.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: a. Expose GeckoAppShell only to Gecko-side IME interface (v1)

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: b. Redefine Java to Gecko IME events to align with Android IME design (v1)

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: c. Implement new Java to Gecko IME events in widget (v1)

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: d. Redefine and implement Gecko to Java IME notifications in widget (v1)

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: e. Implement GeckoEditableListener in GeckoInputConnection (v1)

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: f. Reimplement InputConnection methods using GeckoEditable (v1)

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: g. Reimplement key events in GeckoInputConnection using GeckoEditable (v1)

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: h. Remove unused fields in GeckoInputConnection (v1)

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: i. Refactor DebugGeckoInputConnection to be smaller by using reflection (v1)

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: j. Implement GeckoEditable to encapsulate and marshal Gecko-Java IME communication (v1)

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 675439 [details] [diff] [review]
a. Expose GeckoAppShell only to Gecko-side IME interface (v1)

GeckoEditableListener is the interface that GeckoAppShell calls on GeckoEditable for Gecko-to-Java notifications.

GeckoEditable then marshals these notifications from Gecko thread to UI thread, and calls the same GeckoEditableListener interface on GeckoInputConnection.

This way, GeckoAppShell always deals with the Gecko thread, and GeckoInputConnection always deals with the UI thread.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 675440 [details] [diff] [review]
b. Redefine Java to Gecko IME events to align with Android IME design (v1)

For the events from Java to Gecko, we used to encapsulate Gecko-defined IME operations in these events. The new approach is to encapsulate Android-defined IME operations. Therefore, the task of translating from Android IME operations to Gecko IME operations switches from Java code to Gecko code. This makes it easier to maintain the Gecko-side text in a consistent state.

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 675441 [details] [diff] [review]
c. Implement new Java to Gecko IME events in widget (v1)

New code to translate from Android IME operations to Gecko IME operations, so each event can generate multiple Gecko IME operations.

The mIMEMask*Update variables are used to prevent Java events from generating notifications back to Java.

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 675443 [details] [diff] [review]
e. Implement GeckoEditableListener in GeckoInputConnection (v1)

GeckoEditable forwards Gecko notifications to the UI thread and then calls the corresponding GeckoEditableListener methods in GeckoInputConnection. This makes sure that GeckoInputConnection only has to deal with the UI thread.

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 675445 [details] [diff] [review]
g. Reimplement key events in GeckoInputConnection using GeckoEditable (v1)

I think it's safe to get rid of the hasBuggyHardwareKeyboardLayout check because we are not handling onKeyPreIme anymore.

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 675446 [details] [diff] [review]
h. Remove unused fields in GeckoInputConnection (v1)

Removing unused imports and fields after refactoring GeckoInputConnection.

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 675447 [details] [diff] [review]
i. Refactor DebugGeckoInputConnection to be smaller by using reflection (v1)

It's cleaner to use reflection to assertOnUiThread and to log GeckoInputConnection calls.

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 675448 [details] [diff] [review]
j. Implement GeckoEditable to encapsulate and marshal Gecko-Java IME communication (v1)

Main implementation of GeckoEditable that manages communication between Gecko thread and UI thread.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 675439 [details] [diff] [review]
a. Expose GeckoAppShell only to Gecko-side IME interface (v1)

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

::: mobile/android/base/GeckoAppShell.java
@@ +588,5 @@
> +                                        String modeHint, String actionHint,
> +                                        boolean landscapeFS) {
> +        // notifyIMEEnabled() still needs the landscapeFS parameter
> +        // because it is called from JNI code that assumes it has the
> +        // same signature as XUL Fennec's (which does use landscapeFS).

file a follow up bug to fix this and note the bug number in the comment

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 675441 [details] [diff] [review]
c. Implement new Java to Gecko IME events in widget (v1)

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

::: widget/android/nsWindow.cpp
@@ +1883,5 @@
> +
> +                Text updates are passed on, so the Java text can shadow the
> +                  Gecko text
> +            */
> +            mIMEMaskSelectionUpdate = true;

I'm a little concerned that future patches may introduce an early return or break that will leave the mask turned on. Can we use a RAII pattern here and with other masks?

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: k. Use AutoIMEMask class to manage IME update masking variables (v1)

> I'm a little concerned that future patches may introduce an early return
> or break that will leave the mask turned on. Can we use a RAII pattern
> here and with other masks?

How about this?

Comment 24

[Chris Peterson [:cpeterson]]

Comment on attachment 675441 [details] [diff] [review]
c. Implement new Java to Gecko IME events in widget (v1)

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

::: widget/android/nsWindow.cpp
@@ +1818,5 @@
>  
> +#define ANDROID_COLOR_TO_NSCOLOR(c) NS_RGBA(((c) & 0x000000ff), \
> +                                           (((c) & 0x0000ff00) >> 8), \
> +                                           (((c) & 0x00ff0000) >> 16), \
> +                                           (((c) & 0xff000000) >> 24))

This macro is less efficient than an inline or static function because `c` will expand to `ae->RangeForeColor()` when called below. Thus this macro will call RangeForeColor() member function four times!

Comment 25

[Chris Peterson [:cpeterson]]

Comment on attachment 675443 [details] [diff] [review]
e. Implement GeckoEditableListener in GeckoInputConnection (v1)

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

review -> feedback

::: mobile/android/base/GeckoInputConnection.java
@@ +438,1 @@
>              return;

Is this a valid condition or a bug (in our code or a third-party IME)? If this condition indicates a bug, I think we should either throw an IllegalStateException or at least Log.d() a warning. Because the combination of our IME code and third-party IMEs is very fragile, my preference is to throw an exception to identify invalid expectations earlier. Be bold! :)

@@ +465,3 @@
>  
> +        if (mBatchEditCount > 0) {
> +            return;

Like above: is this a valid condition or a bug?

@@ -634,5 @@
> -        return sentKeyEvents;
> -    }
> -
> -    private KeyEvent[] synthesizeKeyEvents(char inputChar) {
> -        // Some symbol characters produce unusual key events on Froyo and Gingerbread.

Why are you no longer synthesizing KeyEvents?

@@ +752,5 @@
> +                                 final String modeHint,
> +                                 final String actionHint) {
> +        // For some input type we will use a widget to display the ui,
> +        // for those we must not display the ime. We can display a widget
> +        // for date and time types and, if the sdk version

You don't need to wrap lines to 80 columns. Because Java code is so verbose, we have (informally) adopted Google's Java coding guidelines which allow line wrapping at 100 columns:

http://source.android.com/source/code-style.html#limit-line-length

Comment 26

[Chris Peterson [:cpeterson]]

Comment on attachment 675444 [details] [diff] [review]
f. Reimplement InputConnection methods using GeckoEditable (v1)

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

LGTM with nits

::: mobile/android/base/GeckoInputConnection.java
@@ +176,5 @@
>                  // Copy the current selection or the empty string if nothing is selected.
> +                String copiedText = selStart == selEnd ? "" :
> +                        editable.toString().substring(
> +                            Math.min(selStart, selEnd),
> +                            Math.max(selStart, selEnd));

Fix the indentation so wrapped lines start below `selStart`.

@@ -556,5 @@
> -        // is restored from a VKB popup window (such as for entering accented characters)
> -        // back to our IME. We want to commit our active composition string. Bug 756429
> -        if (hasCompositionString()) {
> -            endComposition();
> -        }

Is this bug fix no longer needed? If not, you should consider checking for an active composition and throwing an IllegalStateException (or at least logging a warning).

@@ +610,5 @@
>                  instance = null;
>              }
>  
> +            // TimerTask.run() is running on a random background thread,
> +            // so post to UI thread.

You don't need to wrap lines to 80 columns. 100 is OK.

Comment 27

[Chris Peterson [:cpeterson]]

Comment on attachment 675445 [details] [diff] [review]
g. Reimplement key events in GeckoInputConnection using GeckoEditable (v1)

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

review -> feedback

::: mobile/android/base/GeckoInputConnection.java
@@ -220,5 @@
>  
> -    public boolean onKeyDel() {
> -        // Some IMEs don't update us on deletions
> -        // In that case we are not updated when a composition
> -        // is destroyed, and Bad Things happen

Does this case no longer apply?

@@ -382,5 @@
>      }
>  
>      public boolean onKeyPreIme(int keyCode, KeyEvent event) {
> -        if (hasBuggyHardwareKeyboardLayout())
> -            return false;

1. Your new onKeyPreIme() is a nop. Is it now unnecessary?
2. You removed the hasBuggyHardwareKeyboardLayout() check (which is currently called InputMethods.canUseInputMethodOnHKB() in mozilla-central). That check is necessary for devices with hardware keyboards with non-en_US layouts.

@@ -416,5 @@
>              case KeyEvent.KEYCODE_SEARCH:
>                  return false;
> -            case KeyEvent.KEYCODE_DEL:
> -                // See comments in GeckoInputConnection.onKeyDel
> -                if (onKeyDel()) {

Why do you no longer need to handle KEYCODE_DEL?

Comment 28

[Chris Peterson [:cpeterson]]

Comment on attachment 675446 [details] [diff] [review]
h. Remove unused fields in GeckoInputConnection (v1)

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

LGTM

::: mobile/android/base/GeckoInputConnection.java
@@ +49,4 @@
>      private static final int INLINE_IME_MIN_DISPLAY_SIZE = 480;
>  
> +    private static final Timer mIMETimer =
> +            new Timer("GeckoInputConnection Timer");

You don't need to wrap lines to 80 columns. 100 is OK.

Comment 29

[Chris Peterson [:cpeterson]]

Comment on attachment 675447 [details] [diff] [review]
i. Refactor DebugGeckoInputConnection to be smaller by using reflection (v1)

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

I like that you are shrinking DebugGeckoInputConnection, but I don't want to lose the GeckoApp.assertOnUiThread() and GeckoApp.assertOnGeckoThread() debug checks. I think they are especially important because this bug is about fixing threading bugs. I suggest moving the thread checks to the GeckoInputConnection superclass. I think we should remove the asserts' `if DEBUG` requirement, too.

::: mobile/android/base/GeckoInputConnection.java
@@ +585,5 @@
> +        } else if (Proxy.isProxyClass(obj.getClass())) {
> +            debugAppend(sb, Proxy.getInvocationHandler(obj));
> +        } else if (obj instanceof CharSequence) {
> +            sb.append("\"").append(obj.toString().replace('\n', '\u21b2'))
> +                    .append("\"");

Indent these `.append(` calls so the dots line up vertically.

@@ +591,5 @@
> +            Class cls = obj.getClass();
> +            sb.append(cls.getComponentType().getSimpleName())
> +                    .append("[")
> +                    .append(java.lang.reflect.Array.getLength(obj))
> +                    .append("]");

Indent these `.append(` calls so the dots line up vertically.

Comment 30

[Chris Peterson [:cpeterson]]

Comment on attachment 675448 [details] [diff] [review]
j. Implement GeckoEditable to encapsulate and marshal Gecko-Java IME communication (v1)

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

1. You should consider adding GeckoApp.assertOnUiThread() and GeckoApp.assertOnGeckoThread() checks to GeckoEditable's ui*() and gecko*() methods.

2. Auditing mActions and mActionsActive was challenging. The semaphore can be acquired through multiple code paths and is released on both the UI and Gecko threads. I had to draw some diagrams to understand why some of the mActions and mActionsActive code must be synchronized(this) when most of it is not synchronized.

I would suggest encapsulating mActions and mActionsActive in a static inner class of GeckoEditable. The class's interface would be pretty straight for and look something like:

class ActionQueue {
    pushAction()
    peekAction()
    popAction()
    waitUntilEmpty()
}

::: mobile/android/base/GeckoEditable.java
@@ +56,5 @@
> +
> +    // Filters to implement Editable's filtering functionality
> +    private InputFilter[] mFilters;
> +
> +    private final SpannableStringBuilder mInner;

I think `mInner` needs a more descriptive name.

@@ +61,5 @@
> +    private final Editable mProxy;
> +    private GeckoEditableListener mListener;
> +
> +    /* Queue of text replacement actions sent to Gecko thread that
> +       the Gecko thread has not reponded to yet */

s/reponded/responded/

@@ +69,5 @@
> +    private int mSavedSelectionStart;
> +    private int mSelectionSeqno;
> +    private int mCompositionSeqno;
> +    private int mLastUpdatedCompositionSeqno;
> +    private boolean mUpdateGecko; 

Remove trailing whitespace.

@@ +87,5 @@
> +        /* For Editable.setSpan(Selection...) call; use with IME_SYNCHRONIZE
> +           Note that we don't use this with IME_SET_SELECTION because
> +            we don't want to update the Gecko selection at the point
> +            of this action. The Gecko selection is updated only after UI
> +            has updated its selection (during IME_SYNCHRONIZE reply) */

Fix comment's indentation. Also note that you don't need to wrap lines at 80 columns. 100 is OK.

@@ +105,5 @@
> +        Action(int type) {
> +            mType = type;
> +        }
> +
> +        static Action newReplaceText(CharSequence text, int start, int end) {

I think these Action factory methods (newReplaceText, newSetSelection, and newSetSpan) would be good chokepoints to sanity check these start/end indexes. We should throw an IllegalArgumentException if start < 0 || start > end.

@@ +113,5 @@
> +            action.mEnd = end;
> +            return action;
> +        }
> +
> +        static Action newSetSelection(int start, int end) {

and here

@@ +120,5 @@
> +            action.mEnd = end;
> +            return action;
> +        }
> +
> +        static Action newSetSpan(Object object, int start, int end, int flags) {

and here

@@ +182,5 @@
> +    }
> +
> +    private void geckoRemoveAction() {
> +        if (DEBUG && mActions.isEmpty()) {
> +            throw new IllegalStateException("empty actions queue");

You should remove the `DEBUG &&` part of this check.

@@ +222,5 @@
> +        }
> +
> +        final int selStart = mInner.getSpanStart(Selection.SELECTION_START);
> +        final int selEnd = mInner.getSpanEnd(Selection.SELECTION_END);
> +        int composingStart = mInner.length(), composingEnd = 0;

Move composingEnd declaration to its own line.

@@ +230,5 @@
> +            if ((mInner.getSpanFlags(span) & Spanned.SPAN_COMPOSING) != 0) {
> +                composingStart = Math.min(composingStart,
> +                                          mInner.getSpanStart(span));
> +                composingEnd = Math.max(composingEnd,
> +                                        mInner.getSpanEnd(span));

You don't have to wrap these lines at 80 columns. 100 is OK.

@@ +274,5 @@
> +
> +            if (spans.length == 0) {
> +                rangeType = (selStart == rangeStart && selEnd == rangeEnd)
> +                        ? GeckoEvent.IME_RANGE_SELECTEDRAWTEXT
> +                        : GeckoEvent.IME_RANGE_RAWINPUT;

Fix the indentation so the ? and : operators line up with `(selStart`.

@@ +278,5 @@
> +                        : GeckoEvent.IME_RANGE_RAWINPUT;
> +            } else {
> +                rangeType = (selStart == rangeStart && selEnd == rangeEnd)
> +                        ? GeckoEvent.IME_RANGE_SELECTEDCONVERTEDTEXT
> +                        : GeckoEvent.IME_RANGE_CONVERTEDTEXT;

Fix the indentation so the ? and : operators line up with `(selStart`.

@@ +301,5 @@
> +
> +            if (DEBUG) {
> +                Log.d(LOGTAG, " added " + rangeType + " : " + rangeStyles +
> +                        " : " + Integer.toHexString(rangeForeColor) +
> +                        " : " + Integer.toHexString(rangeBackColor));

Fix the indentation so the quotes line up.

@@ +314,5 @@
> +
> +    @Override
> +    public void sendEvent(GeckoEvent event) {
> +        GeckoAppShell.sendEventToGecko(event);
> +        uiAddAction(new Action(Action.TYPE_EVENT));

Please add a comment explaining how sending the GeckoEvent before adding the Action is not a race condition.

@@ +332,5 @@
> +    }
> +
> +    @Override
> +    public void setListener(GeckoEditableListener listener) {
> +        mListener = listener;

if mListener != null, throw an IllegalStateException?

@@ +339,5 @@
> +    // GeckoEditableListener interface
> +
> +    void geckoActionReply() {
> +        if (DEBUG && mActions.isEmpty()) {
> +            throw new IllegalStateException("empty actions queue");

You can remove the `DEBUG &&` part of the condition because this method is going to crash below with a NullPointerException if mActions.peek() return null. I think think throwing this "empty actions queue" exception is better because it pinpoints the reason for the crash and the check documents to other developers that this method expects a non-empty mActions.

@@ +355,5 @@
> +                    final int end = Selection.getSelectionEnd(mInner);
> +                    if (mListener != null &&
> +                            selStart == start && selEnd == end) {
> +                        // There has not been another new selection
> +                        mListener.onSelectionChange(start, end);

If the selection has not changed, why are you calling onSelectionChange()?

@@ +402,5 @@
> +        });
> +    }
> +
> +    @Override
> +    public void onSelectionChange(final int start, final int end) {

We should throw an IllegalArgumentException if start < 0 || start > end. Can you also check whether end > mInner.length()?

@@ +416,5 @@
> +                uiSyncWithGecko();
> +                /* check to see there has not been another action
> +                   that potentially changed the selection */
> +                if (mSelectionSeqno == seqnoWhenPosted) {
> +                    Selection.setSelection(mProxy, start, end);

Please add a comment explaining why skipping outdated selection updates from Gecko is safe.

@@ +431,5 @@
> +           number to denote "end of the text". Fix that here */
> +        final int oldEnd = unboundedOldEnd > mInner.length() ?
> +                mInner.length() : unboundedOldEnd;
> +        // new end should always match text
> +        if (DEBUG && unboundedNewEnd < (start + text.length())) {

I think this `DEBUG &&` should be removed.

@@ +471,5 @@
> +        } else if (Proxy.isProxyClass(obj.getClass())) {
> +            debugAppend(sb, Proxy.getInvocationHandler(obj));
> +        } else if (obj instanceof CharSequence) {
> +            sb.append("\"").append(obj.toString().replace('\n', '\u21b2'))
> +                    .append("\"");

Fix indentation so `.append` dots line up.

@@ +477,5 @@
> +            Class cls = obj.getClass();
> +            sb.append(cls.getComponentType().getSimpleName())
> +                    .append("[")
> +                    .append(java.lang.reflect.Array.getLength(obj))
> +                    .append("]");

Fix indentation so `.append` dots line up.

@@ +501,5 @@
> +            uiSyncWithGecko();
> +            target = mInner;
> +        }
> +        if (DEBUG) {
> +            Object ret = method.invoke(target, args);

I don't like having separate DEBUG and non-DEBUG code paths. I would move the `ret = method.invoke()` before the `if (DEBUG) {}` and then just `return ret` at the end of the method.

@@ +605,5 @@
> +            CharSequence source, int start, int end) {
> +
> +        CharSequence text = source;
> +        if (start != 0 || end != text.length()) {
> +            text = text.subSequence(start, end);

We should throw an IllegalArgumentException if start < 0 || start > end || end > text.length?

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 676834 [details] [diff] [review]
k. Use AutoIMEMask class to manage IME update masking variables (v1)

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

::: widget/android/nsWindow.cpp
@@ +1823,5 @@
>                                             (((c) & 0xff000000) >> 24))
>  
> +class AutoIMEMask {
> +private:
> +    bool mOldMask, &mMask;

I have a slight preference for "bool mOldMask, *mMask;" for being easier to read and more explicit. But only slight.

Comment 32

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Chris Peterson (:cpeterson) from comment #24)
> Comment on attachment 675441 [details] [diff] [review]
> c. Implement new Java to Gecko IME events in widget (v1)
> 
> Review of attachment 675441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/nsWindow.cpp
> @@ +1818,5 @@
> >  
> > +#define ANDROID_COLOR_TO_NSCOLOR(c) NS_RGBA(((c) & 0x000000ff), \
> > +                                           (((c) & 0x0000ff00) >> 8), \
> > +                                           (((c) & 0x00ff0000) >> 16), \
> > +                                           (((c) & 0xff000000) >> 24))
> 
> This macro is less efficient than an inline or static function because `c`
> will expand to `ae->RangeForeColor()` when called below. Thus this macro
> will call RangeForeColor() member function four times!

I guess my rationale was RangeForeColor() would be inlined anyways, but I do see an issue with RangeForeColor() returning an int, and the macro trying to do an unsigned shift.

Comment 33

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Chris Peterson (:cpeterson) from comment #25)
> Comment on attachment 675443 [details] [diff] [review]
> e. Implement GeckoEditableListener in GeckoInputConnection (v1)
> 
> Review of attachment 675443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> review -> feedback
> 
> ::: mobile/android/base/GeckoInputConnection.java
> @@ +438,1 @@
> >              return;
> 
> Is this a valid condition or a bug (in our code or a third-party IME)? If
> this condition indicates a bug, I think we should either throw an
> IllegalStateException or at least Log.d() a warning. Because the combination
> of our IME code and third-party IMEs is very fragile, my preference is to
> throw an exception to identify invalid expectations earlier. Be bold! :)

It's a valid condition. 'mBatchEditCount > 0' because we should not do updates during batch editing per Android docs. 'mUpdateRequest == null' because we are not being asked to provide updates.

> > -
> > -    private KeyEvent[] synthesizeKeyEvents(char inputChar) {
> > -        // Some symbol characters produce unusual key events on Froyo and Gingerbread.
> 
> Why are you no longer synthesizing KeyEvents?

Was there any benefit to synthesizing them? I think it's hard to get it right (e.g. bug 790120). So I'd rather not deal with it, if we can get away with that :)

Comment 34

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Chris Peterson (:cpeterson) from comment #27)
> ::: mobile/android/base/GeckoInputConnection.java
> @@ -220,5 @@
> >  
> > -    public boolean onKeyDel() {
> > -        // Some IMEs don't update us on deletions
> > -        // In that case we are not updated when a composition
> > -        // is destroyed, and Bad Things happen
> 
> Does this case no longer apply?

Right, we don't need the IME to update us about compositions anymore, since we keep track of all of it in GeckoEditable.

> @@ -382,5 @@
> >      }
> >  
> >      public boolean onKeyPreIme(int keyCode, KeyEvent event) {
> > -        if (hasBuggyHardwareKeyboardLayout())
> > -            return false;
> 
> 1. Your new onKeyPreIme() is a nop. Is it now unnecessary?

I think it's unnecessary now. It was originally added for passing HKB events to Gecko (bug 611764), but I haven't seen that regressing.

> 2. You removed the hasBuggyHardwareKeyboardLayout() check (which is
> currently called InputMethods.canUseInputMethodOnHKB() in mozilla-central).
> That check is necessary for devices with hardware keyboards with non-en_US
> layouts.

If we always return false in onKeyPreIme, I don't think the check is necessary either.

> @@ -416,5 @@
> >              case KeyEvent.KEYCODE_SEARCH:
> >                  return false;
> > -            case KeyEvent.KEYCODE_DEL:
> > -                // See comments in GeckoInputConnection.onKeyDel
> > -                if (onKeyDel()) {
> 
> Why do you no longer need to handle KEYCODE_DEL?

See above.

Comment 35

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Chris Peterson (:cpeterson) from comment #29)
> Comment on attachment 675447 [details] [diff] [review]
> i. Refactor DebugGeckoInputConnection to be smaller by using reflection (v1)
> 
> Review of attachment 675447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like that you are shrinking DebugGeckoInputConnection, but I don't want to
> lose the GeckoApp.assertOnUiThread() and GeckoApp.assertOnGeckoThread()
> debug checks. I think they are especially important because this bug is
> about fixing threading bugs. I suggest moving the thread checks to the
> GeckoInputConnection superclass. I think we should remove the asserts' `if
> DEBUG` requirement, too.

We do still call assertOnUiThread in DebugGeckoInputConnection.invoke(), where we filter all calls to GeckoInputConnection. It's actually easier now, because our premise is GeckoInputConnection should only deal with the UI thread, so we only have to call assertOnUiThread.

Comment 36

[Chris Peterson [:cpeterson]]

Comment on attachment 675447 [details] [diff] [review]
i. Refactor DebugGeckoInputConnection to be smaller by using reflection (v1)

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

r+ since assertOnUiThread is called in DebugGeckoInputConnection.invoke().

Comment 37

[Chris Peterson [:cpeterson]]

(In reply to Jim Chen [:jchen :nchen] from comment #33)
> > Why are you no longer synthesizing KeyEvents?
> 
> Was there any benefit to synthesizing them? I think it's hard to get it
> right (e.g. bug 790120). So I'd rather not deal with it, if we can get away
> with that :)

KeyEvent synthesizing was added to improve compatibility for web content that was listening for keydown, keypress, and keyup events. You should verify that removing the KeyEvent code does not regress bug 687717, bug 721393, bug 755517, or bug 768727.

But you are right that getting this right is tricky. If we can get rid of that code without breaking much web content, then it might be worth it.

Comment 38

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: a. Expose GeckoAppShell only to Gecko-side IME interface (v1.1)

Addressed comment. Carried over r+

Comment 39

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: c. Implement new Java to Gecko IME events in widget (v2)

Changed color macro to function. Carried over r+

Comment 40

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: e. Implement GeckoEditableListener in GeckoInputConnection (v2)

(In reply to Chris Peterson (:cpeterson) from comment #37)
> 
> KeyEvent synthesizing was added to improve compatibility for web content
> that was listening for keydown, keypress, and keyup events. You should
> verify that removing the KeyEvent code does not regress bug 687717, bug
> 721393, bug 755517, or bug 768727.
> 
> But you are right that getting this right is tricky. If we can get rid of
> that code without breaking much web content, then it might be worth it.

The only thing I see not working is bug 721393, and that's related to bug 723810 (or more recently, bug 805766) so I think we should leave KeyEvent synthesizing out and fix those bugs instead.

Comment 41

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: f. Reimplement InputConnection methods using GeckoEditable (v1.1)

Addressed comments. Carried over r+

Comment 42

[Chris Peterson [:cpeterson]]

Comment on attachment 677092 [details] [diff] [review]
e. Implement GeckoEditableListener in GeckoInputConnection (v2)

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

LGTM

Comment 43

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: j. Implement GeckoEditable to encapsulate and marshal Gecko-Java IME communication (v2)

(In reply to Chris Peterson (:cpeterson) from comment #30)
> Comment on attachment 675448 [details] [diff] [review]
> j. Implement GeckoEditable to encapsulate and marshal Gecko-Java IME
> communication (v1)
> 
> Review of attachment 675448 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 1. You should consider adding GeckoApp.assertOnUiThread() and
> GeckoApp.assertOnGeckoThread() checks to GeckoEditable's ui*() and gecko*()
> methods.

I'm adding these checks to the interface methods.

> 2. Auditing mActions and mActionsActive was challenging. The semaphore can
> be acquired through multiple code paths and is released on both the UI and
> Gecko threads. I had to draw some diagrams to understand why some of the
> mActions and mActionsActive code must be synchronized(this) when most of it
> is not synchronized.
> 
> I would suggest encapsulating mActions and mActionsActive in a static inner
> class of GeckoEditable. The class's interface would be pretty straight for
> and look something like:

Added. Looks like ActionQueue cannot be static as it uses GeckoEditable's mCompositionSeqno field.

> > +    private final SpannableStringBuilder mInner;
> 
> I think `mInner` needs a more descriptive name.

Can't really think of a short, descriptive name. What would you suggest?

> @@ +314,5 @@
> > +
> > +    @Override
> > +    public void sendEvent(GeckoEvent event) {
> > +        GeckoAppShell.sendEventToGecko(event);
> > +        uiAddAction(new Action(Action.TYPE_EVENT));
> 
> Please add a comment explaining how sending the GeckoEvent before adding the
> Action is not a race condition.

Added. Basically we send 'event' to Gecko, add Action to queue, then send another sync event to Gecko. Only the sync event replies.

> > +    @Override
> > +    public void setListener(GeckoEditableListener listener) {
> > +        mListener = listener;
> 
> if mListener != null, throw an IllegalStateException?

Shouldn't be an exception because this actually happens in DebugGeckoInputConnection. It sets a proxy listener to replace the one set by GeckoInputConnection.

> 
> @@ +355,5 @@
> > +                    final int end = Selection.getSelectionEnd(mInner);
> > +                    if (mListener != null &&
> > +                            selStart == start && selEnd == end) {
> > +                        // There has not been another new selection
> > +                        mListener.onSelectionChange(start, end);
> 
> If the selection has not changed, why are you calling onSelectionChange()?

Changed comment. Basically we are avoiding out-of-date updates here.

> @@ +416,5 @@
> > +                uiSyncWithGecko();
> > +                /* check to see there has not been another action
> > +                   that potentially changed the selection */
> > +                if (mSelectionSeqno == seqnoWhenPosted) {
> > +                    Selection.setSelection(mProxy, start, end);
> 
> Please add a comment explaining why skipping outdated selection updates from
> Gecko is safe.

Added. The new, upcoming update will replace the effect of this update, so it's not necessary to have the old update.

Comment 44

[Chris Peterson [:cpeterson]]

Comment on attachment 677095 [details] [diff] [review]
j. Implement GeckoEditable to encapsulate and marshal Gecko-Java IME communication (v2)

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

LGTM!

Comment 45

[Chris Peterson [:cpeterson]]

(In reply to Jim Chen [:jchen :nchen] from comment #43)
> > > +    private final SpannableStringBuilder mInner;
> > 
> > I think `mInner` needs a more descriptive name.
> 
> Can't really think of a short, descriptive name. What would you suggest?

How about `mText`? It's a noun and it's even shorter than `mInner`! :)

If you think the name mText is inaccurate or misleading, then mInner is OK.

Comment 46

[Chris Peterson [:cpeterson]]

Comment on attachment 675445 [details] [diff] [review]
g. Reimplement key events in GeckoInputConnection using GeckoEditable (v1)

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

OK. I'm concerned that this change may cause regressions related to bug 669361 or bug 712018, but we can retest those after you land.

Comment 47

[Chris Peterson [:cpeterson]]

Here are some IME bugs that QA might want to retest after his fix lands: bug 756429, bug 669361, bug 712018, bug 687717, bug 721393, bug 755517, bug 768727

Comment 48

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: j. Implement GeckoEditable to encapsulate and marshal Gecko-Java IME communication (v2.1)

Changed to mText. Carried over r+. Thanks Chris!

Comment 49

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/533faa3c50ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/718abc1bd4ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/af2d5272c06b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5554e1345d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ef1b41b829
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a825311d11
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a51bcb3eb9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a01a327e8ec4
https://hg.mozilla.org/integration/mozilla-inbound/rev/973b0ed30b8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/39851bbcfaa1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a92d2d2a3b0e

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this was backed out due to Android bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2301d07ca953

Comment 51

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: l. Avoid unnecessarily setting selection, which can lead to side-effects in automated tests; r=

Basically the tests were failing because the tests would generate key events.
Then we would get notified of text/selection changes, which make us resync with Java.
Next, once Java is synced, it would try to make Gecko sync again.
The process of Java going back to Gecko is unnecessary because Gecko started the chain
in the first place. And this can lead to some side-effects such as scrolling that affect
automated tests.

Comment 52

[Chris Peterson [:cpeterson]]

Comment on attachment 677509 [details] [diff] [review]
l. Avoid unnecessarily setting selection, which can lead to side-effects in automated tests; r=

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

LGTM if you make mGeckoUpdateSeqno volatile.

::: mobile/android/base/GeckoEditable.java
@@ +62,5 @@
>      private GeckoEditableListener mListener;
>      private final ActionQueue mActionQueue;
>  
>      private int mSavedSelectionStart;
> +    private int mGeckoUpdateSeqno;

mGeckoUpdateSeqno is read by both the Gecko and UI threads without synchronization. mGeckoUpdateSeqno is only written by the Gecko thread, so I think marking mGeckoUpdateSeqno as volatile is sufficient to ensure both threads see the latest sequence number.

Does GeckoEditable have any other primitive types that are read and/or written by both threads?

::: widget/android/nsWindow.cpp
@@ +2191,5 @@
>      AndroidBridge::NotifyIMEChange(event.mReply.mString.get(),
>                                     event.mReply.mString.Length(),
>                                     aStart, aOldEnd, aNewEnd);
>  
> +    /* Make sure Java's selection is update-to-date */

s/update-to-date/up-to-date/?

Comment 53

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: l. Avoid unnecessarily setting selection, which can lead to side-effects in automated tests (v1.1)

(In reply to Chris Peterson (:cpeterson) from comment #52)
> Comment on attachment 677509 [details] [diff] [review]
> l. Avoid unnecessarily setting selection, which can lead to side-effects in
> automated tests; r=
> 
> Review of attachment 677509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM if you make mGeckoUpdateSeqno volatile.
> 
> ::: mobile/android/base/GeckoEditable.java
> @@ +62,5 @@
> >      private GeckoEditableListener mListener;
> >      private final ActionQueue mActionQueue;
> >  
> >      private int mSavedSelectionStart;
> > +    private int mGeckoUpdateSeqno;
> 
> mGeckoUpdateSeqno is read by both the Gecko and UI threads without
> synchronization. mGeckoUpdateSeqno is only written by the Gecko thread, so I
> think marking mGeckoUpdateSeqno as volatile is sufficient to ensure both
> threads see the latest sequence number.
> 
> Does GeckoEditable have any other primitive types that are read and/or
> written by both threads?

Changed mGeckoUpdateSeqno to volatile. I looked and seems like mGeckoUpdateSeqno is the only one. Carried over r+.

Comment 54

[(inactive) Jim Chen [:jchen] [:darchons]]

Try of relevant tests:
https://tbpl.mozilla.org/?tree=Try&rev=006ce28affac (oranges are due to bug 792873)

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/320a9b77053b
https://hg.mozilla.org/integration/mozilla-inbound/rev/abda35a08bd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7660aa2bc1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/889288379ae3
https://hg.mozilla.org/integration/mozilla-inbound/rev/5186a8d3722c
https://hg.mozilla.org/integration/mozilla-inbound/rev/951cf800f948
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a06620d9b65
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb69c9fd41d
https://hg.mozilla.org/integration/mozilla-inbound/rev/42a82ec9cc23
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a6d788cd66
https://hg.mozilla.org/integration/mozilla-inbound/rev/58f705e374c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/238ff6d0598c

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/238ff6d0598c
https://hg.mozilla.org/mozilla-central/rev/58f705e374c4
https://hg.mozilla.org/mozilla-central/rev/a1a6d788cd66
https://hg.mozilla.org/mozilla-central/rev/42a82ec9cc23
https://hg.mozilla.org/mozilla-central/rev/9cb69c9fd41d
https://hg.mozilla.org/mozilla-central/rev/9a06620d9b65
https://hg.mozilla.org/mozilla-central/rev/951cf800f948
https://hg.mozilla.org/mozilla-central/rev/5186a8d3722c
https://hg.mozilla.org/mozilla-central/rev/889288379ae3
https://hg.mozilla.org/mozilla-central/rev/d7660aa2bc1d
https://hg.mozilla.org/mozilla-central/rev/abda35a08bd9
https://hg.mozilla.org/mozilla-central/rev/320a9b77053b

Comment 57

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Roll-up patch

[Approval Request Comment]

Bug caused by (feature/regressing bug #): thread-unsafe operations in old Android code

User impact if declined: many IME-related bugs such as bug 770291, bug 805376, bug 794492, bug 803982, bug 790120, bug 803797

Testing completed (on m-c, etc.): try, and on m-c since Nov 1

Risk to taking this patch (and alternatives if risky): Some risk. Large patch, but only limited to Android. There are known issues with contentEditable editors (bug 780543), and there may be other regressions. But so far the known issues are from before and this fix has not made them worse.

String or UUID changes made by this patch: none

Comment 58

[bhavana bajaj [:bajaj]]

[Triage comment]
Considering the size of the patch and given where we are in the aurora cycle, we prefer this rides the train instead of the uplift based on the risk given in comment 57

Comment 59

[bhavana bajaj [:bajaj]]

Comment on attachment 679347 [details] [diff] [review]
Roll-up patch

Refer comment 58

Comment 60

[Alex Keybl [:akeybl]]

We may revisit this decision depending on the outcome of bug 797590. The rest of the IME bugs that this fixes appear to be longstanding issues.

Comment 61

[Aaron Train [:aaronmt]]

This still affects mozilla-beta (18) with awful bugs such as 770291. Can we revisit the notion of uplifting this so users wont have to wait till 19 for a dataloss fix?