Last Comment Bug 700354 - Use of AsyncTask needs some cleanup.
: Use of AsyncTask needs some cleanup.
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P2 normal (vote)
: ---
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-07 10:02 PST by Doug Turner (:dougt)
Modified: 2012-01-09 10:24 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
patch (6.31 KB, patch)
2011-11-07 10:04 PST, Brad Lassey [:blassey] (use needinfo?)
doug.turner: review-
Details | Diff | Splinter Review
patch (17.83 KB, patch)
2011-11-07 15:40 PST, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2011-11-07 10:02:28 PST
What we are seeing is that the completion callback is happening on the wrong thread.  Many users are creating AsyncTasks from the Gecko thread, and then in the completion callback accessing UI elements that must only be accessed from the java thread.
Comment 1 Brad Lassey [:blassey] (use needinfo?) 2011-11-07 10:04:25 PST
Created attachment 572511 [details] [diff] [review]
patch
Comment 2 Doug Turner (:dougt) 2011-11-07 14:43:09 PST
Comment on attachment 572511 [details] [diff] [review]
patch

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

::: embedding/android/GeckoApp.java
@@ +340,4 @@
>          if (!checkAndSetLaunchState(LaunchState.Launching, LaunchState.Launched))
>              return false;
>  
> +        class GeckoThread extends Thread {

put in its own file.

::: embedding/android/Tab.java
@@ +57,5 @@
>  
> +// AsyncTask runs onPostExecute on the thread it is constructed on
> +// We construct these off of the main thread, and we want that to run 
> +// on the main UI thread, so this is a convenience class to do that
> +abstract class GeckoAsyncTask<Params, Progress, Result> {

move into its own file.
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2011-11-07 15:40:26 PST
Created attachment 572650 [details] [diff] [review]
patch
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-07 19:13:20 PST
Comment on attachment 572650 [details] [diff] [review]
patch

Not sure why you wanted to move GeckoThread out of GeckoApp. I kinda liked it better as an inner class. Easier access to GeckoApp innards and less stuff exposed from GeckoApp.

Otherwise looks fine.
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2011-11-07 19:31:36 PST
(In reply to Mark Finkle (:mfinkle) from comment #4)
> Comment on attachment 572650 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Not sure why you wanted to move GeckoThread out of GeckoApp. I kinda liked
> it better as an inner class. Easier access to GeckoApp innards and less
> stuff exposed from GeckoApp.
me too, just doing what doug asked for.

Although, with the landing of pcwalton's patch queue, nothing needs to be made any more public for this change
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-28 19:51:15 PST
Brad, did you land this?
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2011-11-28 20:15:26 PST
yup, https://hg.mozilla.org/projects/birch/rev/13f5d3d70ecb

Note You need to log in before you can comment on or make changes to this bug.