750358 - AsyncTask onPostExecute sometimes runs on GeckoBackgroundThread instead of main thread

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

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)]

Attachment: 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)]

Attachment: patch

Here's the suggested fix. Hopefully this fixes other similar bugs.

Comment 3

[Brian Nicholson (:bnicholson)]

(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?)]

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?)]

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?)]

(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)]

Attachment: patch v2

Moved to GeckoApplication

Comment 8

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/da718420b647

Comment 9

[Brian Nicholson (:bnicholson)]

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]]

https://hg.mozilla.org/mozilla-central/rev/da718420b647

Comment 11

[Alex Keybl [:akeybl]]

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)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/6eb6a52e6f77