24

I have an app in production for a few weeks, using ACRA, and I had zero errors until one strange error reported today.

I've got:

    android.view.ViewRootImpl$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views.

coming from this method in the stack trace (retraced):

at my.app.CountdownFragment$1.void onPostExecute(java.lang.Object)(SourceFile:1)

And this is the relevant source snippet:

    private void addInstructionsIfNeeded() {
    if (S.sDisplayAssist) {
        new AsyncTask<String, Void, String>() {

            @Override
            protected String doInBackground(String... params) {
                return null;
            }

            /*
             * runs on the ui thread
             */
            protected void onPostExecute(String result) {

                Activity a = getActivity();

                if (S.sHelpEnabled && a != null) {

                    in = new InstructionsView(a.getApplicationContext());

                    RelativeLayout mv = (RelativeLayout) a
                            .findViewById(R.id.main_place);

                    mv.addView(in.prepareView());
                }

            };

        }.execute("");
    }
}

Where addInstructionsIfNeeded() is called from a handler dispatched message (the UI thead).

  • onPostExecute() runs on the UI thread, so why I've got "wrong thread"?
  • This code ran already on more than 150 devices, and more than 100000 times (according to Flurry), and never had this error.
  • The originating device is Samsung SGH-I997 running SDK 4.0.4

My question is: How could it be?

EDIT: This all happens in a fragment

Amir Uval
  • 14,425
  • 4
  • 50
  • 74
  • The problem is in the line `Activity a = getActivity();` you should do that before going into the AsyncTask.and you should use class constructor for these kind of initialization – Akram May 03 '12 at 06:25
  • 1
    If I keep a reference to the activity in a member variable, won't it in some cases keep an obsolete activity object alive, causing a memory leak? (until the inner class instance dies) – Amir Uval May 03 '12 at 07:44

6 Answers6

45

i was suffering from the same problem, this is another android framework bug...

what is happening:

in certain circumstances an application can have more than one "looper" and therefore more than one "UI thread"

--side note-- i am using the term "UI thread" in the loosest of senses in this answer, since when people say "UI thread" they usually mean main or entry thread, Android like many of other OS before it, allow for for multiple message pumps (called a Looper in Android, see: http://en.wikipedia.org/wiki/Event_loop) for different UI trees, as such android for all intents and purposes is capable of running more than one "UI thread" in certain circumstances and using that term leads to rampant ambiguities... --end side note--

this means:

since an application can have more than one "UI thread" and an AsyncTask always "Runs on the UI thread" [ref], someone decided [poorly] that instead of the AsyncTask always running on its creation thread (which in 99.999999% of cases would be the correct "UI thread") they decided to use hocus pocus (or a poorly crafted shortcut, you decide) to execute on the "main looper"..

example:

    Log.i("AsyncTask / Handler created ON: " + Thread.currentThread().getId());
    Log.i("Main Looper: " + Looper.getMainLooper().getThread().getId() + "      myLooper: "+ Looper.myLooper().getThread().getId());

    new AsyncTask<Void, Void, Void>() {

        @Override
        protected Void doInBackground(Void... params) {
            Log.i("doInBackground ran ON: " + Thread.currentThread().getId());
            // I'm in the background, all is normal

            handler.post(new Runnable() {

                @Override
                public void run() {
                    Log.i("Handler posted runnable ON: " + Thread.currentThread().getId());
                    // this is the correct thread, that onPostExecute should be on
                }
            });

            return null;
        }

        @Override
        protected void onPostExecute(Void result) {
            Log.i("onPostExecute ran ON: " + Thread.currentThread().getId());
            // this CAN be the wrong thread in certain situations
        }

    }.execute();

if called from the bad situation described above the output will look something like this:

    AsyncTask / Handler created ON: 16
    Main Looper: 1      myLooper: 16
    doInBackground ran ON: 12
    onPostExecute ran ON: 1
    Handler posted runnable ON: 16

that's a huge FAIL for AsyncTask

as shown this can be mitigated using a Handler.post(Runnable) in my specific case the duality of my "UI thread" situation was caused by the fact that I was creating a dialog in response to a JavaScript interface method called from a WebView, basically: the WebView had its own "UI thread" and that was the one that i was currently running on..

from what i can tell (without really caring about or reading into it too much) it seems that the AsyncTask class' callback methods in general run off a single statically instantiated handler (see: http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/4.0.3_r1/android/os/AsyncTask.java#AsyncTask.0sHandler), which means that it is always going to execute on the "main thread" or "entry thread" which they incorrectly refer to as the "UI thread" (which is presumed as any thread where UI interactions take place, eg. multiple threads in this case) this is both shoddy craftsmanship and shoddy documentation from the android team... weak sauce, the sauce is weak

hope this helps you -ck

ckozl
  • 6,751
  • 3
  • 33
  • 50
  • Thanks for sharing you research! It's very interesting. – Amir Uval May 22 '12 at 20:04
  • wow. i apologize for the "thanks" comment spam, but really, thanks. in my case, i was running an async task in the shortcut creation activity, and it was failing will all sorts of message / handler related errors. i used an executor + handler in place of my async task, and all is good. – Jeffrey Blattman Jan 24 '13 at 01:31
  • do you know why WebView has its own thread pool? It seems like it's not completely Asynctask's fault in this case - the actual bug in your code was caused from webview's callback being on the wrong UI thread. – David T. Aug 20 '13 at 16:17
  • @DavidT. WebView has it's own resources because it runs as a separate process, essentially all the web support capabilities are a process onto themselves and are running outside your actual process space. AsyncTask should be locking on the calling thread or be parameterizing the use of a shared handler in the constructor... AsyncTask is not a person it is not "its fault," this is the fault of the documenters and architects that AsyncTask behaves confusingly under certain circumstances, the highly asynchronous nature of the tasks you want to perform in android deserve a better mechanism. – ckozl Aug 20 '13 at 18:04
  • @ckozl Erh... so i have an almost identical problem, and I would like to narrow down all possibilities. Basically, in your answer, you mentioned that you're creating a dialog response to a JavaScript Interface, which i'm guessing is where the error occurs. It seems that the webView decided to perform the Javascript callback (in your android code) on the wrong thread? (I agree that AsyncTask should not be doing what you just proved it to be wrongfully doing, but it seems that the webView call-back running on wrong UI Thread is the immediate culprit?) – David T. Aug 20 '13 at 18:52
  • @ckozl quick question - how did you get your looper to run on thread 16? whenever i make an activity it seems to always run on Thread 1 :'( – David T. Aug 20 '13 at 19:03
  • 1
    @DavidT. Well the callback will happen asynchronously, eg. since the WebView is running outside your process there is no synchronization. Could the implementation post a message on the appropriate handler so that the callback occurs on the starting UI thread? Sure, but it would slow down the process and disallow you to return back to the JavaScript from the callback, which is currently possible and very useful. So, no. The JavaScript interface is correct and behaves understandably. – ckozl Aug 20 '13 at 19:06
  • 1
    @DavidT. myLooper is the looper from the WebView, and that was running on 16 in my scenario (on a real device, over a year ago). I suggest you start a new SO question and I will see if I can help you more there, this comment section is getting too long... – ckozl Aug 20 '13 at 19:06
  • @ckozl Ah, interesting. I guess their implementation choice makes sense. Thanks for the insight. I've also posted up a question, if you have some time, will you please help? http://stackoverflow.com/questions/18434470/android-calledfromwrongthreadexception-from-webviews-shouldoverrideurlloading – David T. Aug 25 '13 at 22:44
  • Link to the bug in the tracker: https://code.google.com/p/android/issues/detail?id=20915&can=1&q=CalledFromWrongThreadException&colspec=ID%20Type%20Status%20Owner%20Summary%20Stars – Brian Melton-Grace - MSFT Sep 04 '13 at 15:42
  • @iambmelton yeah, that bug is relevant... however the situation i described above could still happen since it wasn't triggered by a lazy instantiation of the AsyncTask, but caused by a secondary event handler loop owned by the WebView... – ckozl Sep 04 '13 at 17:29
  • Thanks very much for sharing your knowledge. But Is there any solution for this ? Do we need to change the Asynctask implementation with Executors? – sha Oct 17 '13 at 12:46
  • The solution is to use a `Handler` created where you want the call to go from -> to. The fix for `AsyncTask` would be to have it lock on the thread that it was called on vs the thread that triggered the static constructor, I believe that in newer versions of android it gets auto locked on the entry thread. `AsyncTask` should always return to the thread it was called on, you don't necessarily want it to be on the entry thread. "UI thread locking" and "async tasks" shouldn't be confused like they are here, at the very least `AsyncTask` should have a parameterized ctor so that you can specify. – ckozl Oct 17 '13 at 17:29
10

Had the same issue. Solved in my case

Briefly explanation:

  1. Running AsynckTask for the very first time on non UI thread with looper leads to loading AsyncTask.class and initialization sHandler to handler constructed on that non UI looper.
  2. Now sHandler is connected to that non UI thread for ANY instance of AsyncTask subclasses and onPreExecute, onProgressUpdate and onPostExecute methods will be invoked on that non UI thread (unless AsyncTask.class will be unloaded)
  3. Any attempt to deal with UI inside any of the above methods will lead to crash with android.view.ViewRootImpl$CalledFromWrongThreadException
  4. To avoid such situation one should always run (at least for the very first time) AsyncTask on UI thread in order to let AsyncTask's sHandler-field be initialized with UI's looper

The story:

There were two production apps: A - main android app and B - some utilty app.

After integration app B ito app A we received a lot of crashes:

android.view.ViewRootImpl$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views.

for method running from AsynckTask.onPostExecute()

After some investigation it appeared that utility app B used AsyncTask within its HandlerThread

The traces was found in AsyncTask's source code:

private static final InternalHandler sHandler = new InternalHandler();

This is the handler which is used to send onPostExecute() to UI thread.

This handler is static and it will be initialized during class loading i.e. first new AsyncTask() appearance

It means that onPostExecute will always be posted to that thread where new AsyncTask() was called for the first time (unless AsyncTask.class will be unloaded and loaded again)

In my case the flow was something like this:

1 - starting app A
2 - initializing B form A
3 - B creates its own HandlerThread and launches AsyncTask <- now onPostExecute wil be posted to this HandlerThread no matter where from an instance of AsyncTask will be launched in future
4 - create AsyncTask in the app A for a long operation and update UI in its onPostExecute
5 - when executing onPostExecute() the CalledFromWrongThreadException is thrown

Then a friend of mine showed me related documentation from android.developers (Threading rules section):

The AsyncTask class must be loaded on the UI thread. This is done automatically as of JELLY_BEAN. The task instance must be created on the UI thread. execute(Params...) must be invoked on the UI thread.

Hope it can help to make clear the situation)

vir us
  • 9,920
  • 6
  • 57
  • 66
  • infinite thankyous, even though it shouldn't be, your explenation is perfect. I just set up a default blank AsyncTask and executed it from inside a onUiThread runnable. – WIllJBD Jul 03 '13 at 20:12
9

Maybe the reason is Flurry? I had this exception when I used Flurry 3.2.1. But when I went back to Flurry 3.2.0 I didn't have this exception

Use Flurry 3.2.2 and above.

valerybodak
  • 4,195
  • 2
  • 42
  • 53
3

Placing the following line of code in the Application onCreate should solve the problem:

     /**
     * Fixing AsyncTask Issue not called on main thread
     */
    try {
        Class.forName("android.os.AsyncTask");
    } catch (ClassNotFoundException e) {
        e.printStackTrace();
    }

It seems the issue is created when the AsyncTask class is first initiated on a different main Thread which is not our main Thread, I checked it by adding the code in the bottom, to my Application onCreate

    new Thread(new Runnable() {


        @Override
        public void run() {

            Log.i("tag","1.3onPostExecute ran ON: " + Thread.currentThread().getId());
            Looper.prepare();
            new AsyncTask<Void,Void,Void>(){
                @Override
                protected Void doInBackground(Void... params) {
                    Log.i("tag","2onPostExecute ran ON: " + Thread.currentThread().getId());
                    return null;
                }

                @Override
                protected void onPostExecute(Void aVoid) {
                    Log.i("tag","1.2onPostExecute ran ON: " + Thread.currentThread().getId());
                    super.onPostExecute(aVoid);
                }
            }.execute();
            Looper.loop();
            Looper.myLooper().quit();
        }
    }).start();

This code will init the AsynTask in a main Thread which is not the application main, and will cause the application to crash in any other AsyncTask which will do any UI on the post-execute. crashing with the CalledFromWrongThreadException

Hope it cleared things a little bit more.

Thanks all for the great help on this.

shimi_tap
  • 7,822
  • 5
  • 23
  • 23
  • Can you please explain why this line should help? (preferably by editing the answer). thanks! – Amir Uval May 18 '14 at 12:55
  • Thanks for editing, I understand that you force the AsyncTask class to be instantiated in the main thread prior to creating it inside the thread. I think you could achieve the same effect by creating the actual instance of the AsyncTask on the main thread, and just call the execute from the thread. But my +1 nevertheless.. – Amir Uval May 18 '14 at 20:41
  • I agree this will work to, Actually what mattered to me is testing the solution, which now i know works, Thanks ! – shimi_tap May 18 '14 at 21:02
1

Where is

runOnUiThread(new Runnable() {
    public void run() { /*code*/ } );

in your code

/*
             * runs on the ui thread
             */
            protected void onPostExecute(String result) {

                Activity a = getActivity();

                if (S.sHelpEnabled && a != null) {

                    in = new InstructionsView(a.getApplicationContext());

                    runOnUiThread(new Runnable() {
                       public void run() {

                    RelativeLayout mv = (RelativeLayout) a
                            .findViewById(R.id.main_place);

                    mv.addView(in.prepareView());
                   }
                }

            };

Try this code. I think this would fix the problem

Vinothkumar Arputharaj
  • 4,567
  • 4
  • 29
  • 36
0

I think the problem lies in the line Activity a = getActivity(); I think you should do that before going into the AsyncTask

thepoosh
  • 12,497
  • 15
  • 73
  • 132
  • Thanks for the idea. I'm not sure it's related, since getActivity() is not manipulating the UI. It did once (in some edge case test) return null, that's why I'm checking it before proceeding. – Amir Uval May 03 '12 at 06:28