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)
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)
2.44 KB,
patch
|
rnewman
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Updated•10 years ago
|
Component: General → Android Sync
Product: Firefox for Android → Android Background Services
Comment 1•10 years ago
|
||
Any errors on attempt to remove dumped to console?
Assignee | ||
Comment 2•10 years ago
|
||
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•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Summary: Unable to Remove bookmarks via context menu that were added through Sync → Unable to Remove bookmarks via context menu
Assignee | ||
Comment 5•10 years ago
|
||
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
Comment 6•10 years ago
|
||
I don't see anything in Bug 1064506 that would impact this.
Assignee | ||
Comment 8•10 years ago
|
||
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?)
Assignee | ||
Comment 9•10 years ago
|
||
Ah, invalidateCachedState() is being called from the main thread and clearing the value while desktopBookmarksExist() runs on ModernAsyncTask thread.
Assignee | ||
Comment 10•10 years ago
|
||
Marking both methods |synchronized| solves the issue for me, is this too naive a solution?
Flags: needinfo?(rnewman)
Comment 11•10 years ago
|
||
Is 34 affected?
tracking-fennec: --- → ?
status-firefox34:
--- → ?
status-firefox35:
--- → affected
status-firefox36:
--- → affected
Assignee | ||
Comment 12•10 years ago
|
||
Changeset 204259:f0edea26db81 isn't in my beta repo, and testing beta on playstore confirms "no".
Updated•10 years ago
|
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
Nice.
Comment 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
try push: https://tbpl.mozilla.org/?tree=Try&rev=4915cf214888
Assignee | ||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5aafff0e71c4
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5aafff0e71c4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•10 years ago
|
Flags: needinfo?(liuche)
Assignee | ||
Comment 20•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8507547 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 22•10 years ago
|
||
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)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•