Closed Bug 1074787 Opened 10 years ago Closed 10 years ago

Unable to Remove bookmarks via context menu

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox34 unaffected, firefox35 verified, firefox36 verified, fennec35+)

VERIFIED FIXED
Firefox 36
Tracking Status
firefox34 --- unaffected
firefox35 --- verified
firefox36 --- verified
fennec 35+ ---

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(1 file)

Simple STR:
   1) Fresh install FF nightly
   2) Add Firefox Sync account and let bookmarks populate from desktop
      (My list is ~30 items)
   3) From Bookmarks panel, start Removing entries via long-tap Context menu

Results:
   4) Several (3-5) entries are removed, TwoLinePageRow listitem views are removed
   5) After several successful removals, listitem views stop updating / being removed from list

Further observations:
   6) Swipe close and restart FF
   7) Bookmarks that appeared to fail to be removed from list are now correctly "gone"
   8) Results repeat now from step 3)
Component: General → Android Sync
Product: Firefox for Android → Android Background Services
Any errors on attempt to remove dumped to console?
Tapping the "Remove" option in the failure case generates these LOGCAT messages:

D/Telemetry(32557): SendUIEvent: event = action.1 method = contextmenu timestamp = 188835564 extras = home_remove
D/audio_hw_primary(  181): select_devices: out_snd_device(2: speaker) in_snd_device(0: )
D/ACDB-LOADER(  181): ACDB -> send_afe_cal
W/InputMethodManagerService(  591): Window already focused, ignoring focus gain of: com.android.internal.view.IInputMethodClient$Stub$Proxy@42696358 attribute=null, token = android.os.BinderProxy@424ee3a0
D/SharedBrowserDatabaseProvider(32557): Cleaning up deleted records from bookmarks
D/dalvikvm(32557): GC_FOR_ALLOC freed 3231K, 25% free 17639K/23292K, paused 32ms, total 32ms
D/GeckoToolbar(32557): onTabChanged: MENU_UPDATED
D/GeckoBrowserApp(32557): BrowserApp.onTabChanged: 9: MENU_UPDATED
D/dalvikvm(32557): GC_FOR_ALLOC freed 453K, 16% free 19705K/23292K, paused 27ms, total 27ms
D/dalvikvm(32557): GC_FOR_ALLOC freed 3018K, 23% free 17962K/23292K, paused 25ms, total 26ms
This doesn't sound like it's Sync related at all.  It sounds like we don't update the UI/hit an error/race to update over time.

Disconnect your Sync account or otherwise populate ~30 bookmarks on your device.  Then do the same steps.  Do you see the same behaviour?  I will test locally.
Flags: needinfo?(markcapella)
I was testing in the area of sync when I found this so that's how I "quick-logged" the bug while in the middle of things. But you're correct it's not sync specific.

I installed a fresh FF, bookmarked 17 pages by hand then started deleting them, and ran into the same problem ... list UI not updated. Closing FF and re-starting displays the proper results.
Component: Android Sync → General
Flags: needinfo?(markcapella)
Product: Android Background Services → Firefox for Android
Summary: Unable to Remove bookmarks via context menu that were added through Sync → Unable to Remove bookmarks via context menu
After bisecting ...

The last good revision I have:

changeset:   204258:ed872b06ebbd
parent:      204257:27d8e45f0e0d
parent:      204169:6b8da5940f74
user:        Ryan VanderMeulen <ryanvm@gmail.com>
date:        Mon Sep 08 19:34:40 2014 -0400
description:
Merge m-c to fx-team. a=merge

The next / first bad revision where I can faithfully repro is:

changeset:   204259:f0edea26db81
user:        Chris Kitching <chriskitching@linux.com>
date:        Mon Sep 08 13:10:46 2014 -0700
files:       mobile/android/base/db/LocalBrowserDB.java
description:
Bug 1064506: Eradicate lazy cursor pattern. r=rnewman
I don't see anything in Bug 1064506 that would impact this.
Chenxia, any ideas what could be going on here?
Flags: needinfo?(liuche)
The change in bug 1064506 modifies desktopBookmarksExist() ... This seems to be the critical part ... 

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java?rev=1de6161e995b&mark=761-767#742

Somehow we're coming out of that block with mDesktopBookmarksExist == NULL (which then chokes on the return statement?)
Ah, invalidateCachedState() is being called from the main thread and clearing the value while desktopBookmarksExist() runs on ModernAsyncTask thread.
Marking both methods |synchronized| solves the issue for me, is this too naive a solution?
Flags: needinfo?(rnewman)
Is 34 affected?
tracking-fennec: --- → ?
Changeset 204259:f0edea26db81 isn't in my beta repo, and testing beta on playstore confirms "no".
One of the methods does DB access, so synchronizing -- especially in a manner that blocks the UI thread! -- is a bad idea. invalidateCachedState will block and jank the UI until desktopBookmarksExist returns.

The right fix here is this:

* Set mDesktopBookmarksExist as a side effect, but return the latest computed value directly, rather than reading it back out of the cache:

         try {
             boolean e = c.getCount() > 0;
             mDesktopBookmarksExist = e;
             return e;
         } finally {
             c.close();
         }

         return false;

* Make mDesktopBookmarksExist volatile.
Flags: needinfo?(rnewman)
Attached patch bug1074787.diffSplinter Review
Nice.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #8507547 - Flags: review?(rnewman)
Comment on attachment 8507547 [details] [diff] [review]
bug1074787.diff

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +759,5 @@
>                                    null);
>  
>          try {
> +            // Avoid returning mDesktopBookmarksExist as null
> +            // due to thread interaction.

I'd just phrase this as "Don't read back out of the cache to avoid races with invalidation."
Attachment #8507547 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/5aafff0e71c4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Flags: needinfo?(liuche)
Can we get an uplift approval request here?
tracking-fennec: ? → 35+
Comment on attachment 8507547 [details] [diff] [review]
bug1074787.diff

Approval Request Comment

[Feature/regressing bug #]: Bug 1064506: Eradicate lazy cursor pattern
[User impact if declined]: Poor UI interaction in Bookmarks Panel
[Describe test coverage new/current, TBPL]: Manual testing during fix locally and after push to nightly
[Risks and why]:  minor to none, small patch, code better approximates original, simple backout case, beta not affected affords time to bake in aurora
[String/UUID change made/needed]: none/no
Attachment #8507547 - Flags: approval-mozilla-aurora?
Attachment #8507547 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in
Builds:
Firefox for Android 36.0a1 (2014-11-17)
Firefox for Android 35.0a2 (2014-11-17)
Devices:
Asus Transformer Pad TF300T (Android 4.2.1)
Nexus 4 (Android 4.4.4)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.