856739 - AwesomeBar.onDestroy does DB access on the main thread

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

From bug 856011 comment 0:

android.database.sqlite.SQLiteDatabase.lock(SQLiteDatabase.java:375)
android.database.sqlite.SQLiteProgram.close(SQLiteProgram.java:291)
android.database.sqlite.SQLiteQuery.close(SQLiteQuery.java:133)
android.database.sqlite.SQLiteCursor.close(SQLiteCursor.java:502)
android.database.CursorWrapper.close(CursorWrapper.java:45)
android.content.ContentResolver$CursorWrapperInner.close(ContentResolver.java:1355)
android.database.CursorWrapper.close(CursorWrapper.java:45)
org.mozilla.gecko.AllPagesTab.destroy(AllPagesTab.java:158)
org.mozilla.gecko.AwesomeBarTabs.destroy(AwesomeBarTabs.java:299)
org.mozilla.gecko.AwesomeBar.onDestroy(AwesomeBar.java:488)

Similar stacks are in bug 856015 and bug 856129.

Comment 1

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

Attachment: patch

I found that the BookmarksTab code did the same thing, so I changed that too. In AllPagesTab.destroy, I rearranged the code so the cursor closed happened last. There is a null adapter check that could return early, which made the message clean up code not be called.

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 732306 [details] [diff] [review]
patch

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

The code reordering looks fine but moving code to close the cursor to the background thread looks suspicious: you're postponing an operation on the state of an object that is being destroyed. AFAIK, it's totally fine to close the cursor on main thread. I'd be more confident to r+ this patch if we knew for sure what's causing this bug. Have you confirmed that this patch fixes the crash?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Based on what sriram found in bug 856767 I think here also we should disconnect the adapter and/or cursor from the view before doing the close.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 732306 [details] [diff] [review]
patch

Minusing; see comment 3.

Comment 5

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

Attachment: patch v2

This patch disconnects the adapter from the view before closing the cursor. It's a bit tricky though. I had to stop using "getCursorAdapter" since that method never returns null. Same for "getListView".

Otherwise this should be just like Sriram's patch.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 734692 [details] [diff] [review]
patch v2

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

I'm mixed about this patch. I really don't like the fact that part of of the destroy logic is now done asynchronously. But it seems like the best possible thing to try for now. Please add a comment with more background info about why closing a cursor in UI thread might cause DB lock issues (on pre-ICS it seems).

::: mobile/android/base/awesomebar/BookmarksTab.java
@@ +95,5 @@
>      public void destroy() {
> +        // Can't use getters for adapter. It will create one if null.
> +        if (mCursorAdapter != null && mView != null) {
> +            ListView list = (ListView)mView;
> +            list.setAdapter(null);

Don't you have to set mCursorAdapter to null somewhere?

Comment 7

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

Pushed with comments added:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39fbc0bcfa1f

We don't seem to need to null out mCursorAdapter. Just close the cursor.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39fbc0bcfa1f

Comment 9

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

Comment on attachment 734692 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Can cause DB hangs and locks
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): some risk, but we should be able to manage it
String or IDL/UUID changes made by this patch: none

Comment 10

[bhavana bajaj [:bajaj]]

Approving per, https://bugzilla.mozilla.org/show_bug.cgi?id=843005#c10

Comment 11

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/1221bb6223ef
https://hg.mozilla.org/releases/mozilla-beta/rev/91f56eedcea8