926574 - ANR: SQLite access on main thread when using CursorAdapters

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

I'm not familiar with the code, but it looks like we are using the deprecated CursorAdapter constructor [1] in our code which "results in Cursor queries being performed on the application's UI thread and thus can cause poor responsiveness or even Application Not Responding errors". And we do see a number of these ANRs in the wild [2].

[1] http://developer.android.com/reference/android/widget/CursorAdapter.html#CursorAdapter%28android.content.Context,%20android.database.Cursor%29

[2] http://darchons.github.io/anr-analyzer/?files=c/cb/Anr-20130903.jgz,3/3f/Anr-20130910.jgz,b/bd/Anr-20130917.jgz,d/df/Anr-20130924.jgz#f:anr-20130924,l:28 (requires intranet access)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Look slike all of our HomePage Adapters use this constructor, even the MultiTypeCursorAdapter:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/MultiTypeCursorAdapter.java#28

ReadingListAdapter:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/ReadingListPage.java#200
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/ReadingListPage.java#139

BookmarksListAdapter:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BookmarksListAdapter.java#43
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BookmarksPage.java#80

Lucas, Margaret, Sriram: Any reason we are using this constructor over the "recommended" constructor:
http://developer.android.com/reference/android/widget/CursorAdapter.html#CursorAdapter(android.content.Context,%20android.database.Cursor,%20int)

The docs also talk about using a LoaderManager and CursorLoader, but I see we are using LoadrManagers and CursorLoaders. Do we just need to switch CursorAdapter constructors?

Comment 3

[Sriram Ramasubramanian [:sriram]]

I guess we could use the third constructor. The first one was used because we weren't using support v4 library's CursorAdapter earlier. Now we've started using the support v4's CursorAdapter and won't have to worry about API 11+.

(Remember the issue with swapCursor() and not available in old phones (11+) and we reverting everything to use support library's? We shall go with the third option now safely).

We should register a content observer though, with the flag: FLAG_REGISTER_CONTENT_OBSERVER

I'll post a patch today.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)

> We should register a content observer though, with the flag:
> FLAG_REGISTER_CONTENT_OBSERVER

I saw this in the Javadocs:
"This flag is not needed when using a CursorAdapter with a CursorLoader."

So maybe we could just pass in 0 when we are using a cursor loader.

Comment 5

[Sriram Ramasubramanian [:sriram]]

I felt the same. We are having a observer in our SimpleCursorLoader.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 816775 [details] [diff] [review]
Patch

This is what I would have assumed we'd need to do, but I want more eyes. Handing to Lucas.

Comment 8

[:Margaret Leibovic]

Looks like Sriram has this under control. Clearing needinfo.

Comment 9

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> 
> > We should register a content observer though, with the flag:
> > FLAG_REGISTER_CONTENT_OBSERVER
> 
> I saw this in the Javadocs:
> "This flag is not needed when using a CursorAdapter with a CursorLoader."
> 
> So maybe we could just pass in 0 when we are using a cursor loader.

Yep, our custom cursor loader (SimpleCursorLoader) registers a content observer and forces re-load as appropriate.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 816775 [details] [diff] [review]
Patch

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

Yep.

Comment 11

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/fx-team/rev/7a6e64677565

Comment 12

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/7a6e64677565

Comment 13

[Sriram Ramasubramanian [:sriram]]

Since this caused ANRs, I feel like nominating this for 26.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The patch in this bug appears to have solved the issues in bug 919234, which is tracking 26. Since a patch there would likely have been uplifted to 26, I would +1 this nomination.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Sriram - Please request approval for uplift

Comment 16

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 816775 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Using old version of the constructor.
User impact if declined: ANRs randomly.
Testing completed (on m-c, etc.): 10/16
Risk to taking this patch (and alternatives if risky): None.
String or IDL/UUID changes made by this patch: None.

Comment 17

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5bd7c307b8a5