GeckoInputConnection concurrency issues

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rnewman, Assigned: jchen)

Tracking

(Blocks 1 bug)

Trunk
Firefox 34
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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.
(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.
(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.
Assignee: rnewman → nchen
Attachment #8466459 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/9688c8b7fdc1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.