1074787 - Unable to Remove bookmarks via context menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Capella [:capella]]

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)

Comment 1

[Aaron Train [:aaronmt]]

Any errors on attempt to remove dumped to console?

Comment 2

[Mark Capella [:capella]]

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

Comment 3

[Nick Alexander :nalexander [he/him]]

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.

Comment 4

[Mark Capella [:capella]]

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.

Comment 5

[Mark Capella [:capella]]

After bisecting ...

The last good revision I have:

changeset:   204258:ed872b06ebbd
parent:      204257:27d8e45f0e0d
parent:      204169:6b8da5940f74
user:        Ryan VanderMeulen <[e-mail]>
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 <[e-mail]>
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

Comment 6

[Richard Newman [:rnewman]]

I don't see anything in Bug 1064506 that would impact this.

Comment 7

[:Margaret Leibovic]

Chenxia, any ideas what could be going on here?

Comment 8

[Mark Capella [:capella]]

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?)

Comment 9

[Mark Capella [:capella]]

Ah, invalidateCachedState() is being called from the main thread and clearing the value while desktopBookmarksExist() runs on ModernAsyncTask thread.

Comment 10

[Mark Capella [:capella]]

Marking both methods |synchronized| solves the issue for me, is this too naive a solution?

Comment 11

[Aaron Train [:aaronmt]]

Is 34 affected?

Comment 12

[Mark Capella [:capella]]

Changeset 204259:f0edea26db81 isn't in my beta repo, and testing beta on playstore confirms "no".

Comment 13

[Richard Newman [:rnewman]]

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.

Comment 14

[Mark Capella [:capella]]

Attachment: bug1074787.diff

Nice.

Comment 15

[Richard Newman [: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."

Comment 16

[Mark Capella [:capella]]

try push:
https://tbpl.mozilla.org/?tree=Try&rev=4915cf214888

Comment 17

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/5aafff0e71c4

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5aafff0e71c4

Comment 19

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

Can we get an uplift approval request here?

Comment 20

[Mark Capella [:capella]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0b91fda29efc

Comment 22

[Cristina Madaras, QA [:CristinaM]]

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)