The default bug view has changed. See this FAQ.

Use of AsyncTask needs some cleanup.

RESOLVED FIXED

Status

()

Firefox for Android
General
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: dougt, Assigned: blassey)

Tracking

unspecified
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
Created attachment 572511 [details] [diff] [review]
patch
Assignee: nobody → blassey.bugs
Attachment #572511 - Flags: review?(doug.turner)
(Assignee)

Updated

6 years ago
OS: Mac OS X → Android
Hardware: x86 → ARM
(Reporter)

Updated

6 years ago
Priority: -- → P2
(Reporter)

Comment 2

6 years ago
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.
Attachment #572511 - Flags: review?(doug.turner) → review-
Created attachment 572650 [details] [diff] [review]
patch
Attachment #572511 - Attachment is obsolete: true
Attachment #572650 - Flags: review?(doug.turner)
(Assignee)

Updated

6 years ago
Attachment #572650 - Flags: review?(doug.turner) → review?(mark.finkle)
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.
Attachment #572650 - Flags: review?(mark.finkle) → review+
(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
Brad, did you land this?
yup, https://hg.mozilla.org/projects/birch/rev/13f5d3d70ecb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
tracking-fennec: --- → 11+
(Assignee)

Updated

5 years ago
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.