Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a334e7c7 authored by Wink Saville's avatar Wink Saville
Browse files

Allow reliable detection of a message that is in use. 

Because the standard Looper.loop code calls Message#recycle it is
imperative that Handler#handleMessage code not attempt to resue
a message it receives. If allowed to do so it will cause bugs that
could be difficult to diagnois.

This change adds Message#flags and uses one bit to reliably detect
a message is in use and throws an error in MessageQueue#enqueueMessage.
This allows early detection of this bug.

Note: This is not new functionality, but the current implementation does
not detect messages that are in use because it uses Message#when != 0
as the detection mechanism. The problem is that a Message#when value of 0
is valid value used to place a message at the front of the queue and is
thus unreliable.

Another option is to change the setting of Message#when in Message#enqueueMessage
so that it is never 0, although that does change subtly a publicly accessible
field.

Yet another option would be to use other fields but all candidates have
similar problems as when in that they are publicly accessible or even
settable such as Message#target.

Change-Id: I040d6e546376f7b1ed1e4daa0d5644cce8bf333a
parent 8d920850

core/java/android/os/Message.java

+22 −2
Original line number Diff line number Diff line
@@ -74,6 +74,17 @@ public final class Message implements Parcelable { 
     */

    public Messenger replyTo;



    /** If set message is in use */

    /*package*/ static final int FLAG_IN_USE = 1;



    /** Flags reserved for future use (All are reserved for now) */

    /*package*/ static final int FLAGS_RESERVED = ~FLAG_IN_USE;



    /** Flags to clear in the copyFrom method */

    /*package*/ static final int FLAGS_TO_CLEAR_ON_COPY_FROM = FLAGS_RESERVED | FLAG_IN_USE;



    /*package*/ int flags;



    /*package*/ long when;

    

    /*package*/ Bundle data;
@@ -253,6 +264,7 @@ public final class Message implements Parcelable { 
     * target/callback of the original message.

     */

    public void copyFrom(Message o) {

        this.flags = o.flags & ~FLAGS_TO_CLEAR_ON_COPY_FROM;

        this.what = o.what;

        this.arg1 = o.arg1;

        this.arg2 = o.arg2;
@@ -350,6 +362,7 @@ public final class Message implements Parcelable { 
    }



    /*package*/ void clearForRecycle() {

        flags = 0;

        what = 0;

        arg1 = 0;

        arg2 = 0;
@@ -361,6 +374,14 @@ public final class Message implements Parcelable { 
        data = null;

    }



    /*package*/ boolean isInUse() {

        return ((flags & FLAG_IN_USE) == FLAG_IN_USE);

    }



    /*package*/ void markInUse() {

        flags |= FLAG_IN_USE;

    }



    /** Constructor (but the preferred way to get a Message is to call {@link #obtain() Message.obtain()}).

    */

    public Message() {
@@ -453,4 +474,3 @@ public final class Message implements Parcelable { 
        replyTo = Messenger.readMessengerOrNullFromParcel(source);

    }

}

core/java/android/os/MessageQueue.java

+2 −1
Original line number Diff line number Diff line
@@ -120,6 +120,7 @@ public class MessageQueue { 
                now = SystemClock.uptimeMillis();

                Message msg = pullNextLocked(now);

                if (msg != null) {

                    msg.markInUse();

                    return msg;

                }
@@ -192,7 +193,7 @@ public class MessageQueue { 
    }



    final boolean enqueueMessage(Message msg, long when) {

        if (msg.when != 0) {

        if (msg.isInUse()) {

            throw new AndroidRuntimeException(msg

                    + " This message is already in use.");

        }

core/tests/coretests/src/android/os/MessageQueueTest.java

+174 −1
Original line number Diff line number Diff line
@@ -41,6 +41,10 @@ public class MessageQueueTest extends TestCase { 
        }



        public void handleMessage(Message msg) {

            if (!msg.isInUse()) {

                failure(new RuntimeException(

                        "msg.isInuse is false, should always be true, #" + msg.what));

            }

            if (mCount <= mLastMessage) {

                if (msg.what != mCount) {

                    failure(new RuntimeException(
@@ -99,5 +103,174 @@ public class MessageQueueTest extends TestCase { 


        tester.doTest(1000);

    }



    private static class TestFieldIntegrityHandler extends TestHandlerThread {

        Handler mHandler;

        int mLastMessage;

        int mCount;



        public TestFieldIntegrityHandler() {

        }



        public void go() {

            mHandler = new Handler() {

                public void handleMessage(Message msg) {

                    TestFieldIntegrityHandler.this.handleMessage(msg);

                }

            };

        }



        public void handleMessage(Message msg) {

            if (!msg.isInUse()) {

                failure(new RuntimeException(

                        "msg.isInuse is false, should always be true, #" + msg.what));

            }

            if (mCount <= mLastMessage) {

                if (msg.what != mCount) {

                    failure(new RuntimeException(

                            "Expected message #" + mCount

                                    + ", received #" + msg.what));

                } else if (mCount == mLastMessage) {

                    success();

                }

                mCount++;

            } else {

                failure(new RuntimeException(

                        "Message received after done, #" + msg.what));

            }

        }

    }



    @MediumTest

    public void testFieldIntegrity() throws Exception {



        TestHandlerThread tester = new TestFieldIntegrityHandler() {

            Bundle mBundle;



            public void go() {

                super.go();

                mLastMessage = 1;

                mCount = 0;

                mHandler.sendMessage(mHandler.obtainMessage(0));

            }



            public void handleMessage(Message msg) {

                super.handleMessage(msg);

                if (msg.what == 0) {

                    msg.flags = -1;

                    msg.what = 1;

                    msg.arg1 = 456;

                    msg.arg2 = 789;

                    msg.obj = this;

                    msg.replyTo = null;

                    mBundle = new Bundle();

                    msg.data = mBundle;

                    msg.data.putString("key", "value");



                    Message newMsg = mHandler.obtainMessage();

                    newMsg.copyFrom(msg);

                    if (newMsg.isInUse() != false) {

                        failure(new RuntimeException(

                                "newMsg.isInUse is true should be false after copyFrom"));

                    }

                    if (newMsg.flags != 0) {

                        failure(new RuntimeException(String.format(

                        "newMsg.flags is %d should be 0 after copyFrom", newMsg.flags)));

                    }

                    if (newMsg.what != 1) {

                        failure(new RuntimeException(String.format(

                                "newMsg.what is %d should be %d after copyFrom", newMsg.what, 1)));

                    }

                    if (newMsg.arg1 != 456) {

                        failure(new RuntimeException(String.format(

                                "newMsg.arg1 is %d should be %d after copyFrom", msg.arg1, 456)));

                    }

                    if (newMsg.arg2 != 789) {

                        failure(new RuntimeException(String.format(

                                "newMsg.arg2 is %d should be %d after copyFrom", msg.arg2, 789)));

                    }

                    if (newMsg.obj != this) {

                        failure(new RuntimeException(

                                "newMsg.obj should be 'this' after copyFrom"));

                    }

                    if (newMsg.replyTo != null) {

                        failure(new RuntimeException(

                                "newMsg.replyTo should be null after copyFrom"));

                    }

                    if (newMsg.data == mBundle) {

                        failure(new RuntimeException(

                                "newMsg.data should NOT be mBundle after copyFrom"));

                    }

                    if (!newMsg.data.getString("key").equals(mBundle.getString("key"))) {

                        failure(new RuntimeException(String.format(

                                "newMsg.data.getString(\"key\") is %s and does not equal" +

                                " mBundle.getString(\"key\") which is %s after copyFrom",

                                newMsg.data.getString("key"),  mBundle.getString("key"))));

                    }

                    if (newMsg.when != 0) {

                        failure(new RuntimeException(String.format(

                                "newMsg.when is %d should be 0 after copyFrom", newMsg.when)));

                    }

                    if (newMsg.target != mHandler) {

                        failure(new RuntimeException(

                                "newMsg.target is NOT mHandler after copyFrom"));

                    }

                    if (newMsg.callback != null) {

                        failure(new RuntimeException(

                                "newMsg.callback is NOT null after copyFrom"));

                    }



                    mHandler.sendMessage(newMsg);

                } else if (msg.what == 1) {

                    if (msg.isInUse() != true) {

                        failure(new RuntimeException(String.format(

                                "msg.isInUse is false should be true after when processing %d",

                                msg.what)));

                    }

                    if (msg.arg1 != 456) {

                        failure(new RuntimeException(String.format(

                                "msg.arg1 is %d should be %d when processing # %d",

                                msg.arg1, 456, msg.what)));

                    }

                    if (msg.arg2 != 789) {

                        failure(new RuntimeException(String.format(

                                "msg.arg2 is %d should be %d when processing # %d",

                                msg.arg2, 789, msg.what)));

                    }

                    if (msg.obj != this) {

                        failure(new RuntimeException(String.format(

                                "msg.obj should be 'this' when processing # %d", msg.what)));

                    }

                    if (msg.replyTo != null) {

                        failure(new RuntimeException(String.format(

                                "msg.replyTo should be null when processing # %d", msg.what)));

                    }

                    if (!msg.data.getString("key").equals(mBundle.getString("key"))) {

                        failure(new RuntimeException(String.format(

                                "msg.data.getString(\"key\") is %s and does not equal" +

                                " mBundle.getString(\"key\") which is %s when processing # %d",

                                msg.data.getString("key"),  mBundle.getString("key"), msg.what)));

                    }

                    if (msg.when != 0) {

                        failure(new RuntimeException(String.format(

                                "msg.when is %d should be 0 when processing # %d",

                                msg.when, msg.what)));

                    }

                    if (msg.target != null) {

                        failure(new RuntimeException(String.format(

                                "msg.target is NOT null when processing # %d", msg.what)));

                    }

                    if (msg.callback != null) {

                        failure(new RuntimeException(String.format(

                                "msg.callback is NOT null when processing # %d", msg.what)));

                    }

                } else {

                    failure(new RuntimeException(String.format(

                            "Unexpected msg.what is %d" + msg.what)));

                }

            }

        };



        tester.doTest(1000);

    }

}