Last Comment Bug 750358 - AsyncTask onPostExecute sometimes runs on GeckoBackgroundThread instead of main thread
: AsyncTask onPostExecute sometimes runs on GeckoBackgroundThread instead of ma...
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 16
Assigned To: Brian Nicholson (:bnicholson)
:
Mentors:
Depends on:
Blocks: 746946
  Show dependency treegraph
 
Reported: 2012-04-30 11:00 PDT by :Margaret Leibovic
Modified: 2012-07-09 21:52 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
-
15+


Attachments
debug patch (2.86 KB, patch)
2012-07-06 21:43 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Review
patch (1.00 KB, patch)
2012-07-06 21:46 PDT, Brian Nicholson (:bnicholson)
blassey.bugs: review-
Details | Diff | Review
patch v2 (1.20 KB, patch)
2012-07-06 22:29 PDT, Brian Nicholson (:bnicholson)
blassey.bugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description :Margaret Leibovic 2012-04-30 11:00:07 PDT
I'm filing this as a follow-up to bug 746946. We landed a band-aid patch there, but a real solution involves finding out why this is happening, and perhaps using GeckoAsyncTask instead of AsyncTask to ensure onPostExecute always gets run on the main thread. However, to make this switch, we'd need to implement cancel() for GeckoAsyncTask.
Comment 1 Brian Nicholson (:bnicholson) 2012-07-06 21:43:21 PDT
Created attachment 639931 [details] [diff] [review]
debug patch

I just ran into this after landing patches in bug 769145. Here's the STR I'm using - they only seem to work on my Galaxy Nexus:

1) Go to Manage Applications > Fennec, and click "Clear data"
2) Open Fennec
3) Quickly tap the AwesomeBar
4) Wait a few seconds
5) If there's no crash, click back, then go to step 3 (if the favicon on about:home has loaded, it's too late; start over from step 1)

This crashes fairly reliably for me with the signature from bug 746946. I followed these STR using the attached patch, and here's the output I see when it crashes:

D/BRN     ( 1056): 1 calling from main thread? true
D/BRN     ( 1056): 2 calling from main thread? false
D/BRN     ( 1056): 3 calling from main thread? false

This seems like a bug in AsyncTask since, according to [1], onPostExecute() should be called on same thread as the AsyncTask (which is the UI thread here). This led me to [2], which validates that it is, in fact, a recognized Android bug. I tried the solution given by romainguy, and it fixes the problem.

[1] http://developer.android.com/reference/android/os/AsyncTask.html
[2] http://code.google.com/p/android/issues/detail?id=20915
Comment 2 Brian Nicholson (:bnicholson) 2012-07-06 21:46:13 PDT
Created attachment 639933 [details] [diff] [review]
patch

Here's the suggested fix. Hopefully this fixes other similar bugs.
Comment 3 Brian Nicholson (:bnicholson) 2012-07-06 21:51:33 PDT
(In reply to Brian Nicholson (:bnicholson) from comment #1)
> I just ran into this after landing patches in bug 769145. Here's the STR I'm
> using - they only seem to work on my Galaxy Nexus:

s/Galaxy Nexus/Galaxy S/

This is the same phone, and very similar steps, to what Aaron used to reproduce bug 769145.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-06 22:04:44 PDT
Comment on attachment 639933 [details] [diff] [review]
patch

Shouldn't this be in GeckoApplication instead of GeckoActivity? See:
http://code.google.com/p/android/issues/detail?id=20915#c7
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-07-06 22:22:13 PDT
Comment on attachment 639933 [details] [diff] [review]
patch

As Finkle said in comment 4, this should be done in Application::onCreate(). I'd also note that this doesn't guaranty to work around the problem, just makes it much less likely that we'll hit it. The sure fire way to fix this is to stop using AsyncTasks and use GeckoAsyncTasks.
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-06 22:27:39 PDT
(In reply to Brad Lassey [:blassey] from comment #5)
> The sure fire way to fix this is to stop using AsyncTasks and use GeckoAsyncTasks.

I think we needed a few more features in GeckoAsyncTask to be able to use it in more places.
Comment 7 Brian Nicholson (:bnicholson) 2012-07-06 22:29:21 PDT
Created attachment 639935 [details] [diff] [review]
patch v2

Moved to GeckoApplication
Comment 8 Brian Nicholson (:bnicholson) 2012-07-06 22:38:44 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/da718420b647
Comment 9 Brian Nicholson (:bnicholson) 2012-07-06 22:54:48 PDT
Comment on attachment 639935 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: potential to crash using AsyncTask
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): very low risk
String or UUID changes made by this patch: none
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-07-07 11:59:55 PDT
https://hg.mozilla.org/mozilla-central/rev/da718420b647
Comment 11 Alex Keybl [:akeybl] 2012-07-08 17:53:41 PDT
Comment on attachment 639935 [details] [diff] [review]
patch v2

[Triage Comment]
Given the low risk profile and the mobile team's tracking-fennec designation, let's uplift to Aurora 15.
Comment 12 Brian Nicholson (:bnicholson) 2012-07-09 21:52:05 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/6eb6a52e6f77

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