970372 - Synced folders never appear until after browser restart

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Seemingly this has always been an issue. We don't update the ListView with Synced folders ever. Only on browser restart do my Synced folders show up.

This only happens after initial Sync. Idling in the bookmark ListView will not show my Synced folders. Closing and re-opening will not either. I need to manually close and restart the browser (swipe off recent application listing) to show my Synced folders in the bookmarks pane.

Just reproduced this on my Sony Xperia Tablet Z and my LG Nexus 5 w/Nightly (02/10).

Comment 1

[Aaron Train [:aaronmt]]

By 'Closing and re-opening will not either' I mean re: my single about:home tab

Comment 2

[Richard Newman [:rnewman]]

Something, somewhere, doesn't have a live cursor or a content observer.

Nothing to do with Sync; this is a pure frontend thing.

Comment 3

[Aaron Train [:aaronmt]]

For record (in-case this is a 'because you have a single folder as a root' issue or something)

Before Sync

* Firefox: About your Browser
* Firefox: Customize with Add'ons
* Firefox Marketplace
* Firefox Support

After Sync (and after browser restart)

* Desktop Bookmarks (**new**)
* Firefox: About your Browser
* Firefox: Customize with Add'ons
* Firefox Marketplace
* Firefox Support

Comment 4

[:Margaret Leibovic]

Without looking at the code, I suspect this has to do with the fact that the "Desktop Bookmarks" folder isn't a real folder, but rather a UI hack. So even if the cursor is updated to have your desktop bookmarks, we need to do something to update the UI.

Comment 5

[Aaron Train [:aaronmt]]

> "Desktop Bookmarks" folder isn't a real folder

Mind = blown

Comment 6

[:Margaret Leibovic]

Looking through the code, it seems like the main problem is that we cache the value of mDesktopBookmarksExist:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#427

So even if we perform another query, this will continue to be false.

However, I thought that we had fixed this once long ago in bug 727449. Indeed, we do still call this invalidateCachedState method:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BookmarksPanel.java#128

Maybe BookmarksPanel doesn't get recreated as often as the AwesomeBar activity did, so this fix isn't good enough anymore.

Comment 7

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #6)

> Maybe BookmarksPanel doesn't get recreated as often as the AwesomeBar
> activity did, so this fix isn't good enough anymore.

Certainly true -- the old activity used to be paused and resumed every single time you switched to or from web content.

Comment 8

[:Margaret Leibovic]

I couldn't reproduce this exactly as described. If I navigate away from about:home, then come back to it, the desktop bookmarks folder has appeared. Maybe when this bug was filed there was just some delay with bookmarks actually syncing?

It is true that we invalidate this cached state when we create the BookmarksPanel fragment, which happens when about:home is loaded. One potential improvement could be to move that invalidate call to somewhere like the BookmarksLoader constructor, so that we don't invalidate until we're about to actually load the panel contents. This would fix the scenario where the sync happens before the user visits the bookmarks panel, but it still would not fix the situation where the panel is already loaded.

I added some logging, and I noticed that while the first sync is happening, BookmarksLoader.loadCursor is called a bunch of times in a row (in my logs, almost 10 times), presumably because of the SimpleCursorLoader content observer. Perhaps we should consider registering a different content observer to invalidate the cache, since invalidating the cache when the bookmarks table changes makes more sense than invalidating it when we load the bookmarks panel.

Comment 9

[:Margaret Leibovic]

Attachment: Register content observer for bookmarks cursor

This is actually a nice improvement because we won't invalidate the cache when we don't need to.

I tested setting up sync while on the bookmarks panel, and the desktop bookmarks folder appeared as expected.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8376573 [details] [diff] [review]
Register content observer for bookmarks cursor

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

::: mobile/android/base/home/BookmarksPanel.java
@@ +223,5 @@
>  
>          @Override
>          public Cursor loadCursor() {
> +            final Cursor c = BrowserDB.getBookmarksInFolder(getContext().getContentResolver(), mFolderInfo.id);
> +            c.registerContentObserver(mContentObserver);

My one concern is whether I need to unregister this content observer somewhere. Will closing the cursor will unregister any content observers, or do I need to look for a place to do this explicitly?

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8376573 [details] [diff] [review]
Register content observer for bookmarks cursor

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

::: mobile/android/base/home/BookmarksPanel.java
@@ +223,5 @@
>  
>          @Override
>          public Cursor loadCursor() {
> +            final Cursor c = BrowserDB.getBookmarksInFolder(getContext().getContentResolver(), mFolderInfo.id);
> +            c.registerContentObserver(mContentObserver);

The SimpleCursorLoader already registers a content observer to be able to trigger auto-reloads by calling the Loader's onContentChanged(). Furthermore, this approach is a bit fragile as it assumes this observer will be called before the loader re-runs the query.

So here's a simpler approach: just override the BookmarksLoader's onContentChanged():

@Override
public void onContentChanged() {
   BrowserDB.invalidateCachedState();
   super.onContentChanged();
}

I haven't tested this but it should work. This also ensures the invalidateCachedState() call is done before the query is executed again.

Comment 12

[:Margaret Leibovic]

Attachment: Invalidate cache in onContentChanged

Yup, that works. Sorry I didn't think of that myself!

Comment 13

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/56d8d12f89a4

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56d8d12f89a4

Comment 15

[:Margaret Leibovic]

Comment on attachment 8377874 [details] [diff] [review]
Invalidate cache in onContentChanged

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this issue has existed since the new about:home (Fx26), but we care about it more now that we're going to promote new sync
User impact if declined: desktop bookmarks won't appear until about:home is closed and re-opened
Testing completed (on m-c, etc.): tested locally, just landed on m-c
Risk to taking this patch (and alternatives if risky): low risk, just changes when we invalidate a cached value
String or IDL/UUID changes made by this patch: none

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/06c41a48e27a

Comment 17

[Aaron Train [:aaronmt]]

\o/