1041082 - GeckoInputConnection concurrency issues

Status: RESOLVED FIXED

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

Description

[Richard Newman [:rnewman]]

private static Handler sBackgroundHandler;

        Thread backgroundThread = new Thread(new Runnable() {
            @Override
            public void run() {
                Looper.prepare();
                synchronized (GeckoInputConnection.class) {
                    sBackgroundHandler = new Handler();

...
        backgroundThread.start();
        while (sBackgroundHandler == null) {
            try {
                // wait for new thread to set sBackgroundHandler
                GeckoInputConnection.class.wait();


Amongst other sins.

Comment 1

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

Seems okay?

(In reply to Richard Newman [:rnewman] from comment #0)
> private static Handler sBackgroundHandler;
> 
>         Thread backgroundThread = new Thread(new Runnable() {
>             @Override
>             public void run() {
>                 Looper.prepare();
>                 synchronized (GeckoInputConnection.class) {
>                     sBackgroundHandler = new Handler();

sBackgroundHandler is accessed within GeckoInputConnection.class lock.

> 
> ...
>         backgroundThread.start();
>         while (sBackgroundHandler == null) {

sBackgroundHandler is accessed within GeckoInputConnection.class lock (the method is 'static synchronized').

>             try {
>                 // wait for new thread to set sBackgroundHandler
>                 GeckoInputConnection.class.wait();
> 
> 
> Amongst other sins.

Comment 2

[Richard Newman [:rnewman]]

(In reply to Jim Chen [:jchen :nchen] from comment #1)

> sBackgroundHandler is accessed within GeckoInputConnection.class lock.

Not always:

        Thread backgroundThread = new Thread(new Runnable() {
            @Override
            public void run() {
                Looper.prepare();
                synchronized (GeckoInputConnection.class) {
                    sBackgroundHandler = new Handler();
                    GeckoInputConnection.class.notify();
                }
                Looper.loop();
                sBackgroundHandler = null;    <<<<<<<<<<<<<<<<<<<<<<<<<
            }
        }, LOGTAG);


> > Amongst other sins.

Here's one:

    @Override
    public Handler getHandler(Handler defHandler) {
        if (!canReturnCustomHandler()) {
            return defHandler;
        }
        // getBackgroundHandler() is synchronized and requires locking,
        // but if we already have our handler, we don't have to lock
        final Handler newHandler = sBackgroundHandler != null
                                 ? sBackgroundHandler
                                 : getBackgroundHandler();


This is double-checked locking ("let's save ourselves the cost of taking the lock!"), and it's not safe in Java, even for reads (see JCIP).

(Related: <http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html>.)

We have a new thread here setting sBackgroundHandler to null outside a synchronized block, getHandler reading and returning it outside a synchronized block. Those two operations can interact, with results including newHandler = null. Quite apart from that, without synchronization or volatile, it's hard to predict exactly how the compiler will reorder these memory accesses.

sBackgroundHandler should either always have synchronized access, should be an atomic reference, or should be volatile.

Comment 3

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

(In reply to Richard Newman [:rnewman] from comment #2)
> (In reply to Jim Chen [:jchen :nchen] from comment #1)
> 
> > sBackgroundHandler is accessed within GeckoInputConnection.class lock.
> 
> Not always:
> 
>                 Looper.loop();
>                 sBackgroundHandler = null;    <<<<<<<<<<<<<<<<<<<<<<<<<

This line actually never runs because the thread never exits.

> 
> Here's one:
> 
>         // getBackgroundHandler() is synchronized and requires locking,
>         // but if we already have our handler, we don't have to lock
>         final Handler newHandler = sBackgroundHandler != null
>                                  ? sBackgroundHandler
>                                  : getBackgroundHandler();

This is true. Double-checked locking is a bad idea.

Comment 4

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

Attachment: Don't perform double-checked locking in GeckoInputConnection (v1)

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9688c8b7fdc1

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9688c8b7fdc1