Closed Bug 926574 Opened 7 years ago Closed 7 years ago

ANR: SQLite access on main thread when using CursorAdapters

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox25 --- unaffected
firefox26 + fixed
firefox27 --- fixed
fennec 26+ ---

People

(Reporter: jchen, Assigned: sriram)

References

Details

(Whiteboard: [ANR])

Attachments

(1 file)

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)
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?
Flags: needinfo?(sriram)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(lucasr.at.mozilla)
Duplicate of this bug: 867251
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.
Flags: needinfo?(sriram)
(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.
I felt the same. We are having a observer in our SimpleCursorLoader.
Attached patch PatchSplinter Review
Attachment #816775 - Flags: review?(mark.finkle)
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.
Attachment #816775 - Flags: review?(mark.finkle) → review?(lucasr.at.mozilla)
Looks like Sriram has this under control. Clearing needinfo.
Flags: needinfo?(margaret.leibovic)
Assignee: nobody → sriram
tracking-fennec: --- → ?
(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.
Flags: needinfo?(lucasr.at.mozilla)
Comment on attachment 816775 [details] [diff] [review]
Patch

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

Yep.
Attachment #816775 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/7a6e64677565
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Since this caused ANRs, I feel like nominating this for 26.
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.
Sriram - Please request approval for uplift
tracking-fennec: ? → 26+
Blocks: 919234
Flags: needinfo?(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.
Attachment #816775 - Flags: approval-mozilla-aurora?
Attachment #816775 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.