975173 - HomeBanner CalledFromWrongThreadException

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

I know how to fix this, I'll take it.

02-20 18:19:01.088 E/GeckoEventDispatcher(16789): android.view.ViewRootImpl$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views.
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at android.view.ViewRootImpl.checkThread(ViewRootImpl.java:6006)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at android.view.ViewRootImpl.invalidateChildInParent(ViewRootImpl.java:855)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at android.view.ViewGroup.invalidateChild(ViewGroup.java:4320)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at android.view.View.invalidate(View.java:10919)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at android.view.View.setFlags(View.java:8900)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at android.view.View.setVisibility(View.java:6020)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at android.widget.ImageView.setVisibility(ImageView.java:1225)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at org.mozilla.gecko.home.HomeBanner.handleMessage(HomeBanner.java:138)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:97)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2333)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:357)
02-20 18:19:01.088 E/GeckoEventDispatcher(16789): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:177)

Comment 1

[:Margaret Leibovic]

Attachment: Set HomeBanner ImageView visibility on the main thread

This issue existed before bug 960359, but I built this patch on top of the patches for bug 960359 to avoid bit-rot.

Bailing early before calling BitmapUtils.getDrawable doesn't really save us much, since the first thing that method does is just do a null check:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/BitmapUtils.java#45

Comment 2

[Brian Nicholson (:bnicholson)]

Comment on attachment 8379508 [details] [diff] [review]
Set HomeBanner ImageView visibility on the main thread

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

::: mobile/android/base/home/HomeBanner.java
@@ -130,5 @@
>              return;
>          }
>  
>          final String iconURI = message.optString("iconURI");
>          final ImageView iconView = (ImageView) findViewById(R.id.icon);

findViewById() is also a method on View, so these should also be moved to the UI thread (here and above).

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 8379508 [details] [diff] [review]
Set HomeBanner ImageView visibility on the main thread

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

::: mobile/android/base/home/HomeBanner.java
@@ +139,2 @@
>                  // Update the banner icon on the UI thread.
>                  ThreadUtils.postToUiThread(new Runnable() {

Looking at all of our BitmapUtils#onBitmapFound callbacks throughout our code, we always seem to be assuming/wanting the callback to happen on the UI thread (which makes sense -- we get an image back, so we're likely going to update UI). Instead of having a postToUiThread() everywhere (which is actually missing in several places we handle this callback), we should probably just make sure that the callback always happens on the UI thread. I can file a follow-up if you don't want to do that here.

Comment 4

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #3)
> Comment on attachment 8379508 [details] [diff] [review]
> Set HomeBanner ImageView visibility on the main thread
> 
> Review of attachment 8379508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeBanner.java
> @@ +139,2 @@
> >                  // Update the banner icon on the UI thread.
> >                  ThreadUtils.postToUiThread(new Runnable() {
> 
> Looking at all of our BitmapUtils#onBitmapFound callbacks throughout our
> code, we always seem to be assuming/wanting the callback to happen on the UI
> thread (which makes sense -- we get an image back, so we're likely going to
> update UI). Instead of having a postToUiThread() everywhere (which is
> actually missing in several places we handle this callback), we should
> probably just make sure that the callback always happens on the UI thread. I
> can file a follow-up if you don't want to do that here.

Yeah, I definitely fell for that when originally writing this code (as did whoever reviewed it). I can address that in this bug.

Comment 5

[Josh Dover [:jdover]]

Comment on attachment 8379508 [details] [diff] [review]
Set HomeBanner ImageView visibility on the main thread

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

::: mobile/android/base/home/HomeBanner.java
@@ -130,5 @@
>              return;
>          }
>  
>          final String iconURI = message.optString("iconURI");
>          final ImageView iconView = (ImageView) findViewById(R.id.icon);

The findViewById() above to get textView also needs to be in the UI thread.

Ideally these wouldn't be called every time handleMessage() is called and would be stored in a instance var in the constructor.

Comment 6

[:Margaret Leibovic]

Attachment: Make sure HomeBanner only touches views on the main thread

Comment 7

[:Margaret Leibovic]

Attachment: (Part 2) Always call BitmapLoader.onBitmapFound on the UI thread

Sorry I keep giving you so many reviews! This isn't high priority, but a clean-up.

The `runOnBitmapFoundOnUiThread` method name seems kinda awkward, but I couldn't think of something better.

Comment 8

[:Margaret Leibovic]

Attachment: (Part 3) Declare local variables as final in getDrawable

Some more cleanup. I'll let jdover review this, since he's now the expert on final local variables.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8380042 [details] [diff] [review]
(Part 2) Always call BitmapLoader.onBitmapFound on the UI thread

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

Nice, this is much better.

::: mobile/android/base/gfx/BitmapUtils.java
@@ +41,5 @@
>          public void onBitmapFound(Drawable d);
>      }
>  
> +    private static void runOnBitmapFoundOnUiThread(final BitmapLoader loader, final Drawable d) {
> +        ThreadUtils.postToUiThread(new Runnable() {

We should also probably check ThreadUtils.isOnUiThread() first; that way, we can execute the callback immediately and save ourselves a Runnable if we're already on the UI thread.

Comment 10

[:Margaret Leibovic]

Try push for good measure:
https://tbpl.mozilla.org/?tree=Try&rev=f997a1b519b6

Comment 11

[:Margaret Leibovic]

Green on try.

https://hg.mozilla.org/integration/fx-team/rev/889527944470
https://hg.mozilla.org/integration/fx-team/rev/af0f055d4ca3
https://hg.mozilla.org/integration/fx-team/rev/eeda6a335f14

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/889527944470
https://hg.mozilla.org/mozilla-central/rev/af0f055d4ca3
https://hg.mozilla.org/mozilla-central/rev/eeda6a335f14

Comment 13

[:Margaret Leibovic]

Comment on attachment 8380027 [details] [diff] [review]
Make sure HomeBanner only touches views on the main thread

[Approval Request Comment]
Bug caused by (feature/regressing bug #): snippets and sync promo banner
User impact if declined: exceptions get thrown
Testing completed (on m-c, etc.): landed on m-c 2/23
Risk to taking this patch (and alternatives if risky): low-risk, moves some method calls to the ui thread
String or IDL/UUID changes made by this patch: none

Comment 14

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee34a1cb0543