Closed Bug 721776 Opened 8 years ago Closed 8 years ago

Bookmark is removed from bookmark list only after Fennec restart

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 13
Tracking Status
firefox11 --- affected
firefox12 --- affected
firefox13 --- verified
fennec + ---

People

(Reporter: iacchi, Assigned: bnicholson)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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.
Nice catch. Thanks for filing Iacopo. CC'ing :lucasr
tracking-fennec: --- → ?
Flags: in-litmus?(fennec)
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)
Strange. All bookmark DB operations are done inside an AsyncTask in a separate thread. Aaron, does that happen on all devices you tested?
(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.
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.
Assignee: nobody → lucasr.at.mozilla
Is the bookmark after closing and reopening the awesomebar alone? No need to restart Fennec.
tracking-fennec: ? → +
(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).
(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.
Whiteboard: [QA^]
Priority: -- → P2
Assignee: lucasr.at.mozilla → bnicholson
Attached patch patch (obsolete) — Splinter Review
Attachment #594076 - Flags: review?(mark.finkle)
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #594076 - Attachment is obsolete: true
Attachment #594076 - Flags: review?(mark.finkle)
Attachment #594078 - Flags: review?(mark.finkle)
Blocks: 724194
Summary: Bookmark is removed from bookmark list only after reboot when removed → Bookmark is removed from bookmark list only after Fennec restart
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.
Attachment #594078 - Flags: review?(mark.finkle)
Attachment #594078 - Flags: review?(blassey.bugs)
Attachment #594078 - Flags: review+
(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?
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.
Attachment #594977 - Flags: review?(mark.finkle)
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).
Attachment #594078 - Attachment is obsolete: true
Attachment #594078 - Flags: review?(blassey.bugs)
Attachment #594982 - Flags: review?(mark.finkle)
Attachment #594977 - Flags: review?(mark.finkle) → review+
Attachment #594982 - Flags: review?(mark.finkle) → review+
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)?
https://hg.mozilla.org/mozilla-central/rev/870a1156baf0
https://hg.mozilla.org/mozilla-central/rev/d9999729e539
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
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.
Attachment #594977 - Flags: approval-mozilla-beta?
Attachment #594977 - Flags: approval-mozilla-aurora?
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.
Attachment #594982 - Flags: approval-mozilla-beta?
Attachment #594982 - Flags: approval-mozilla-aurora?
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.
Attachment #594977 - Flags: approval-mozilla-beta?
Attachment #594977 - Flags: approval-mozilla-beta+
Attachment #594977 - Flags: approval-mozilla-aurora?
Attachment #594977 - Flags: approval-mozilla-aurora+
Attachment #594982 - Flags: approval-mozilla-beta?
Attachment #594982 - Flags: approval-mozilla-beta+
Attachment #594982 - Flags: approval-mozilla-aurora?
Attachment #594982 - Flags: approval-mozilla-aurora+
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.]
Attachment #594977 - Flags: approval-mozilla-beta+
Attachment #594977 - Flags: approval-mozilla-aurora+
Attachment #594982 - Flags: approval-mozilla-beta+
Attachment #594982 - Flags: approval-mozilla-aurora+
 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"
Status: RESOLVED → VERIFIED
Testcase updated in litmus: BFT - Bokmarks
https://litmus.mozilla.org/show_test.cgi?id=50508
Flags: in-litmus?(fennec) → in-litmus+
Whiteboard: [QA^]
You need to log in before you can comment on or make changes to this bug.