721776 - Bookmark is removed from bookmark list only after Fennec restart

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Iacopo Benesperi [:iacchi]]

This happens both on current Aurora and Nightly, at least on my Galaxy SII (Android 2.3.4).

If you hold on a bookmark in the bookmark list and then remove it from the context menu, the menu entry doesn't disappear until you reboot the browser.

I'm using today's (2012-01-27) nightlies both for Aurora and Nightly with the Italian localisation.

Comment 1

[Aaron Train [:aaronmt]]

Nice catch. Thanks for filing Iacopo. CC'ing :lucasr

Comment 2

[Aaron Train [:aaronmt]]

As far as I can see, the bookmark is deleted except and we hit a strictmode policy violation:

D/GeckoBrowserProvider(25039): Deleted 1 rows for URI: content://org.mozilla.fennec.db.browser/bookmarks?profile=default
D/GeckoBrowserProvider(25039): Successful delete transaction: content://org.mozilla.fennec.db.browser/bookmarks?profile=default
D/StrictMode(25039): StrictMode policy violation; ~duration=110 ms: android.os.StrictMode$StrictModeDiskWriteViolation: policy=31 violation=1
D/StrictMode(25039): 	at android.os.StrictMode$AndroidBlockGuardPolicy.onWriteToDisk(StrictMode.java:1048)
D/StrictMode(25039): 	at android.database.sqlite.SQLiteStatement.acquireAndLock(SQLiteStatement.java:226)
D/StrictMode(25039): 	at android.database.sqlite.SQLiteStatement.executeUpdateDelete(SQLiteStatement.java:84)
D/StrictMode(25039): 	at android.database.sqlite.SQLiteDatabase.executeSql(SQLiteDatabase.java:1899)
D/StrictMode(25039): 	at android.database.sqlite.SQLiteDatabase.execSQL(SQLiteDatabase.java:1839)
D/StrictMode(25039): 	at android.database.sqlite.SQLiteDatabase.beginTransaction(SQLiteDatabase.java:661)
D/StrictMode(25039): 	at android.database.sqlite.SQLiteDatabase.beginTransaction(SQLiteDatabase.java:552)
D/StrictMode(25039): 	at org.mozilla.fennec.db.BrowserProvider.delete(BrowserProvider.java:636)

Comment 3

[Lucas Rocha (:lucasr)]

Strange. All bookmark DB operations are done inside an AsyncTask in a separate thread. Aaron, does that happen on all devices you tested?

Comment 4

[Aaron Train [:aaronmt]]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> Strange. All bookmark DB operations are done inside an AsyncTask in a
> separate thread. Aaron, does that happen on all devices you tested?

Galaxy SII, Nexus S, Galaxy Nexus so far.

Comment 5

[Michael Kohler [:mkohler]]

This happens on my HTC Desire HD too. It seems the bookmark is deleted instantly but we don't repaint the list. I think repainting is the correct solution here.

Comment 6

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

Is the bookmark after closing and reopening the awesomebar alone? No need to restart Fennec.

Comment 7

[Aaron Train [:aaronmt]]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Is the bookmark after closing and reopening the awesomebar alone? No need to
> restart Fennec.

No. Also the bookmark menu-item will indicate that the bookmark is still existant (check-marked).

Comment 8

[Michael Kohler [:mkohler]]

(In reply to Aaron Train [:aaronmt] from comment #7)
> (In reply to Mark Finkle (:mfinkle) from comment #6)
> > Is the bookmark after closing and reopening the awesomebar alone? No need to
> > restart Fennec.
> 
> No. Also the bookmark menu-item will indicate that the bookmark is still
> existant (check-marked).

which is bug 722413.

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: patch

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: patch v2

It looks like requery() is deprecated. This patch asynchronously gets and sets a new cursor as suggested in http://developer.android.com/reference/android/database/Cursor.html#requery%28%29.

Comment 11

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

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

Looks OK to me, but I want Brad to check the use of AsyncTask. We want the onPostExecute to not be run on the UI thread. We assume that this should put the code on a background thread, but it might be the Gecko background thread.

GeckoAsyncTask seems to put onPostExecute on the UI thread, which we do not want here.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> Comment on attachment 594078 [details] [diff] [review]
> patch v2
> 
> Looks OK to me, but I want Brad to check the use of AsyncTask. We want the
> onPostExecute to not be run on the UI thread. We assume that this should put
> the code on a background thread, but it might be the Gecko background thread.
> 
> GeckoAsyncTask seems to put onPostExecute on the UI thread, which we do not
> want here.

What thread is this being called on? Since Toast.makeText() is called there, I'm going to assume it is the main UI thread, in which case, why not just use GeckoAsyncTask?

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: (1/2) Don't run the remove bookmark DB command on main thread

The StrictMode exception is happening because we're running the bookmark delete command on DB in the UI thread. This patch moves the DB operation to the background thread and then runs the UI bits in main thread.

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: (2/2) Refresh bookmarks after a bookmark is deleted

This is Brian's patch rebased on top of patch 1/2. This ensures that the AsyncTask to refresh the bookmarks is started in the main thread and hence its onPostExecute() runs on the main thread as well (which is what we want here).

Comment 15

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/870a1156baf0
http://hg.mozilla.org/integration/mozilla-inbound/rev/d9999729e539

Comment 16

[Brian Nicholson (:bnicholson)]

Comment on attachment 594977 [details] [diff] [review]
(1/2) Don't run the remove bookmark DB command on main thread

+                GeckoAppShell.getHandler().post(new Runnable() {
+                    public void run() {
+                        ContentResolver resolver = Tabs.getInstance().getContentResolver();
+                        BrowserDB.removeBookmark(resolver, url);
+
+                        GeckoApp.mAppContext.mMainHandler.post(new Runnable() {
+                            public void run() {
+                                Toast.makeText(this, R.string.bookmark_removed,
+                                        Toast.LENGTH_SHORT).show();
+                            }
+                        });
+                    }
+                });

How come you didnt use GeckoAsyncTask here? It looks like you're doing a background operation, then posting the result on the UI thread. Isn't that what GeckoAsyncTask is a wrapper for (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAsyncTask.java)?

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/870a1156baf0
https://hg.mozilla.org/mozilla-central/rev/d9999729e539

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 594977 [details] [diff] [review]
(1/2) Don't run the remove bookmark DB command on main thread

[Approval Request Comment]
User impact if declined: User might get crashes due to IO operations being used in UI thread.
Risk to taking this patch (and alternatives if risky): Low risk.
String changes made by this patch: None.

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 594982 [details] [diff] [review]
(2/2) Refresh bookmarks after a bookmark is deleted

[Approval Request Comment]
User impact if declined: User might get confused because we keep the removed bookmark in the list.
Risk to taking this patch (and alternatives if risky): Low risk.
String changes made by this patch: None.

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 594977 [details] [diff] [review]
(1/2) Don't run the remove bookmark DB command on main thread

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.

Comment 21

[Matt Brubeck (:mbrubeck)]

Comment on attachment 594977 [details] [diff] [review]
(1/2) Don't run the remove bookmark DB command on main thread

Clearing approval for Aurora 12 and Beta 11 because we are not currently planning a Native Fennec release of these versions.  If this changes in the future, we will likely do a mass uplift of all native fennec changes.  For now, let's get these bugs off the channel triage radar.

[Filter on the string "mbrubeck-bugspam" if you want to delete all of these emails at once.]

Comment 22

[Camelia Urian]

 Nightly 13.0a1 (2012-02-28)
Device: Samsung Nexus S - Android 2.3.6

Verified fixed: bookmark is correctly removed after long tap on a bookmark and selecting "remove"

Comment 23

[Camelia Urian]

Testcase updated in litmus: BFT - Bokmarks
https://litmus.mozilla.org/show_test.cgi?id=50508