Closed Bug 975173 Opened 6 years ago Closed 6 years ago

HomeBanner CalledFromWrongThreadException

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
fennec 29+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(3 files, 1 obsolete file)

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)
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
Attachment #8379508 - Flags: review?(bnicholson)
Attachment #8379508 - Flags: feedback?(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);

findViewById() is also a method on View, so these should also be moved to the UI thread (here and above).
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.
(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.
tracking-fennec: --- → ?
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.
Attachment #8379508 - Flags: feedback?(jdover) → feedback+
Attachment #8379508 - Attachment is obsolete: true
Attachment #8379508 - Flags: review?(bnicholson)
Attachment #8380027 - Flags: review?(bnicholson)
Attachment #8380027 - Flags: feedback?(jdover)
Attachment #8380027 - Flags: review?(bnicholson) → review+
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.
Attachment #8380042 - Flags: review?(bnicholson)
Some more cleanup. I'll let jdover review this, since he's now the expert on final local variables.
Attachment #8380048 - Flags: review?(jdover)
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.
Attachment #8380042 - Flags: review?(bnicholson) → review+
Attachment #8380048 - Flags: review?(jdover) → review+
Attachment #8380027 - Flags: feedback?(jdover) → feedback+
Try push for good measure:
https://tbpl.mozilla.org/?tree=Try&rev=f997a1b519b6
Depends on: 977383
tracking-fennec: ? → 29+
tracking-fennec: ? → 29+
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
Attachment #8380027 - Flags: approval-mozilla-aurora?
Attachment #8380027 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.