859855 - More DB access on main thread

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Geoff Brown [:gbrown]]

Robocop tests continue to intermittently timeout showing DB access on the main thread. 

From https://bugzilla.mozilla.org/show_bug.cgi?id=856129#c20:

    Thread[main,5,main]
    java.lang.Object.wait(Native Method)
    java.lang.Thread.parkFor(Thread.java:1535)
    java.lang.LangAccessImpl.parkFor(LangAccessImpl.java:48)
    sun.misc.Unsafe.park(Unsafe.java:317)
    java.util.concurrent.locks.LockSupport.park(LockSupport.java:131)
    java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:790)
    java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:823)
    java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1153)
    java.util.concurrent.locks.ReentrantLock$FairSync.lock(ReentrantLock.java:200)
    java.util.concurrent.locks.ReentrantLock.lock(ReentrantLock.java:261)
    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)
    android.widget.CursorAdapter.changeCursor(CursorAdapter.java:251)
    android.widget.SimpleCursorAdapter.changeCursor(SimpleCursorAdapter.java:321)
    android.widget.CursorFilter.publishResults(CursorFilter.java:67)
    android.widget.Filter$ResultsHandler.handleMessage(Filter.java:282)
    android.os.Handler.dispatchMessage(Handler.java:99)
    android.os.Looper.loop(Looper.java:123)
    android.app.ActivityThread.main(ActivityThread.java:4627)
    java.lang.reflect.Method.invokeNative(Native Method)
    java.lang.reflect.Method.invoke(Method.java:521)
    com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:868)
    com.android.internal.os.ZygoteInit.main(ZygoteInit.java:626)
    dalvik.system.NativeStart.main(Native Method) 

:mfinkle says that he thinks that changing the cursor will close the old cursor; he found this:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/awesomebar/BookmarksTab.java#381

Comment 1

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

The stack above has no Fennec/Gecko lines of code so it's a bit hard to find the cause. The full log shows that the Awesomebar has been created before the failure, so that is a hint.

Also, SimpleCursorAdapter.changeCursor is in the stack, so I looked for a changeCursor call. I found one here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/awesomebar/BookmarksTab.java#132

Called from:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/awesomebar/BookmarksTab.java#388

Which is run from the UI thread. Since changeCursor ends up closing a cursor, Android tries to lock the DB (Gingerbread and earlier) causing this ANR.

IMO, BookmarksQueryTask and getCursorAdpater are kinda broken:
* BookmarksQueryTask uses AsyncTask, which breaks one of our current rules. Use UiAsyncTask.
* BookmarksQueryTask forces a UI thread execution in onPostExecute because AsyncTask runs on whatever thread creates it. UiAsyncTask does not.
* BookmarksQueryTask passes a Cursor from the background to the UI thread. This is usually a bad sign, and here is no different.
* getCursorAdapter does too damn much! It plays with the cursor and the view. We should split those apart.
* BookmarksQueryTask should use an onPreExecute to handle any ListView pre-DB changes, like setting the Adpater to null (or fixing the header if that's the real issue)
* BookmarksQueryTask should do the DB and set the new cursor to the adapter in the background. This is why getCursorAdapter should not do any view work.

Comment 2

[Lucas Rocha (:lucasr)]

Just some quick comments.

(In reply to Mark Finkle (:mfinkle) from comment #1)
> The stack above has no Fennec/Gecko lines of code so it's a bit hard to find
> the cause. The full log shows that the Awesomebar has been created before
> the failure, so that is a hint.
> 
> Also, SimpleCursorAdapter.changeCursor is in the stack, so I looked for a
> changeCursor call. I found one here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/awesomebar/
> BookmarksTab.java#132
> 
> Called from:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/awesomebar/
> BookmarksTab.java#388
> 
> Which is run from the UI thread. Since changeCursor ends up closing a
> cursor, Android tries to lock the DB (Gingerbread and earlier) causing this
> ANR.
> 
> IMO, BookmarksQueryTask and getCursorAdpater are kinda broken:
> * BookmarksQueryTask uses AsyncTask, which breaks one of our current rules.
> Use UiAsyncTask.
> * BookmarksQueryTask forces a UI thread execution in onPostExecute because
> AsyncTask runs on whatever thread creates it. UiAsyncTask does not.
> * BookmarksQueryTask passes a Cursor from the background to the UI thread.
> This is usually a bad sign, and here is no different.

This is not really bad sign. It's quite common practice to load a cursor in a background thread (via AsyncTask, CursorLoader, etc) and use it on the UI thread. Cursor are used in the UI thread most of the time (e.g. adapters). What exactly looks bad about it?

> * getCursorAdapter does too damn much! It plays with the cursor and the
> view. We should split those apart.
> * BookmarksQueryTask should use an onPreExecute to handle any ListView
> pre-DB changes, like setting the Adpater to null (or fixing the header if
> that's the real issue)
> * BookmarksQueryTask should do the DB and set the new cursor to the adapter
> in the background. This is why getCursorAdapter should not do any view work.

Ok but let's make sure we're framing the problem accurately here. Setting the new cursor to the adapter in the UI thread is only a problem because of this closing-cursor-locks-DB bug on Gingerbread and earlier.

Let's not turn all this into a general guideline as if it's generally not ok to set a new cursor to the adapter or closing a cursor in the UI thread. Quite the contrary, I'd say the natural and correct thing to do is actually to do all these things in the UI thread.

FWIW, we should *not* call changeCursor()/swapCursor() in a background thread because it will internally call notifyDataSetChanged() which must be called in the UI thread.

Any workaround for the changeCursor() problem should probably involve replacing changeCursor() with swapCursor() (which doesn't automatically close the previously set cursor) and close the previous cursor ourselves off UI thread if needed.

Comment 3

[Mark Capella [:capella]]

I'm going to hack at this for a bit. I think I understand it, and have already started, but if this becomes a critical priority and someone needs to steal it back, just let me know.

I'll yell if I have any questions.

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v1)

Ok, I've been testing this for a short time and will continue to do so, but I thought I could @ least post the rough patch for you both to have an advance look, and so I can also see the side by side DIFF.

Most if not all of what mfinkle mentions has been taken into account.

* BookmarksQueryTask uses AsyncTask, which breaks one of our current rules. Use UiAsyncTask.
>> I Can do this, but with the rest of his suggested changes done, wonder if you see a benefit to plug 'n playing the UiAsyncTask() here ... if seems we're now using the existing AsyncTask properly, and that it will continue to match the logic in the HistoryTabs.java module as-is ...(?)

* BookmarksQueryTask forces a UI thread execution in onPostExecute because AsyncTask runs on whatever thread creates it. UiAsyncTask does not.
>> No longer happens.

* BookmarksQueryTask passes a Cursor from the background to the UI thread. This is usually a bad sign, and here is no different.
>> If you refer here to doInBackground() returning a Cursor to onPostExecute() then yah, no, that's gone.

* getCursorAdapter does too damn much! It plays with the cursor and the view. We should split those apart.
>> I just deleted the whole thing. *bonus points*

* BookmarksQueryTask should use an onPreExecute to handle any ListView pre-DB changes, like setting the Adpater to null (or fixing the header if that's the real issue)
>> Yep, did it.

* BookmarksQueryTask should do the DB and set the new cursor to the adapter in the background. This is why getCursorAdapter should not do any view work.
>> Also did it. This leads to the question I have for lucasr regarding his comment |we should *not* call changeCursor()/swapCursor() in a background thread because it will internally call notifyDataSetChanged() which must be called in the UI thread| ...

>> Can you elaborate? This patch seems proper, as the DB access is done in the background, and the CursorAdapter.java's swapCursor() method handles the old-to-new cursor swap as designed (Content and DataSet Observers are unregistered / re-registered, etc...) ... We safeguard the UI thread by disconnecting the cursor from the UI (null-ing) prior to changing in the background, then reconnect it after returning to the main thread from there.


Other than that, whoever was closest to the the original code will notice some changes / optimizations where streamlining allowed us to remove what I figured were redundant checks, most noticably in the handleItemClick() method. So far my testing hasn't suffered for the changes, but I trust you'll let me know if you see something flagrant that I didn't.

Comment 5

[Mark Capella [:capella]]

Forgot this https://tbpl.mozilla.org/?tree=Try&rev=9f9cd68c7df1

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 759736 [details] [diff] [review]
Patch (v1)

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

Given that we're rewriting the bookmarks tab in fig, I wonder if it's still worth tweaking the current code.

Comment 7

[Mark Capella [:capella]]

Yah :) Unfortunately I tuned into the new stuff just after pursuing this ...

Comment 8

[Mark Capella [:capella]]

releasing ... new about:home will re-write all of this and obviate the patch