1040806 - Default bookmark favicons are not displayed after browser restart

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: screenshot.png

Currently, the default bookmark listing can get into a situation where their associated default Favicon are not shown in the bookmark view.

Steps to Reproduce

  i. Launch the browser under a new profile
  ii. Swipe to the bookmark listing, see bookmarks and their Favicon
  iii. Close the browser by swiping off the recent application listing
  iv. Re-launch the browser, swipe to the bookmark listing

See screenshot

--
Nightly (07/18)
LG Nexus 5 (Android 4.4.4)

Comment 1

[Aaron Train [:aaronmt]]

e38671421b22	Richard Newman — Bug 1016611 - Part 5: be resilient against failed insertions. r=margaret
7834fa7dbb95	Richard Newman — Bug 1016611 - Part 4: fix testBookmarksPanel. r=margaret
270e99e71b91	Richard Newman — Bug 1016611 - Part 3: basic thread safety in GeckoProfile. r=margaret
2b3bcade4155	Richard Newman — Bug 1016611 - Part 2: further cleanup. r=margaret
6460524ba1a9	Richard Newman — Bug 1016611 - Part 1: don't process distribution files for webapp profiles. r=margaret

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=724d46a1b00a&tochange=6da364bc0e3e

Comment 2

[Richard Newman [:rnewman]]

Hm, I thought that was another (older) bug surfacing. Let me take a look.

Comment 3

[Richard Newman [:rnewman]]

The issue *appears* to be simple: when we insert the default favicons, we don't set a creation time or modification time.

Logging confirms that we insert them.

Pulling the DB confirms that they're present.

Then we pause, and we clean up:

07-18 11:44:10.526 D/GeckoHealthRec(29883): Recording session end: P
07-18 11:44:10.526 I/LAB126  ( 1013): PhoneStatusBar hears intent. action: android.intent.action.CLOSE_SYSTEM_DIALOGS
07-18 11:44:10.586 V/GeckoHealthRec(29883): Recorded session entry for env 1, current is 1
07-18 11:44:10.586 D/GeckoSessInfo(29883): Recording session done: 1405708955266
07-18 11:44:10.586 V/DoNotDisturb(  827): Top component: com.amazon.kindle.otter/com.amazon.kindle.otter.Launcher
07-18 11:44:10.596 V/GeckoBrowserProvider(29883): Calling delete in transaction on URI: content://org.mozilla.fennec_rnewman.db.browser/history/old?profile=default&priority=NORMAL
07-18 11:44:10.596 D/GeckoBrowserProvider(29883): Expiring history.
07-18 11:44:10.596 D/GeckoBrowserProvider(29883): Not expiring history: only have 0 rows.
07-18 11:44:10.596 D/GeckoBrowserProvider(29883): Expiring thumbnails.
07-18 11:44:10.606 V/GeckoBrowserProvider(29883): Clear thumbs using query: DELETE FROM thumbnails WHERE url NOT IN (  SELECT url FROM combined ORDER BY (CASE WHEN bookmark_id > -1 THEN 100 ELSE 0 END) + visits * MAX(1, 100 * 225 / ((date - 1405709050611) / 86400000*(date - 1405709050611) / 86400000 + 225))  DESC LIMIT 15) AND url NOT IN (  SELECT url FROM bookmarks WHERE parent = -3)
07-18 11:44:10.606 D/GeckoBrowserProvider(29883): Deleting all unused favicons and thumbnails for URI: content://org.mozilla.fennec_rnewman.db.browser/history/old?profile=default&priority=NORMAL
07-18 11:44:10.606 D/GeckoBrowserProvider(29883): Deleting favicons for URI: content://org.mozilla.fennec_rnewman.db.browser/history/old?profile=default&priority=NORMAL
07-18 11:44:10.606 D/GeckoBrowserProvider(29883): Deleting thumbnails for URI: content://org.mozilla.fennec_rnewman.db.browser/history/old?profile=default&priority=NORMAL
07-18 11:44:10.606 D/GeckoBrowserProvider(29883): Deleted 0 rows for URI: content://org.mozilla.fennec_rnewman.db.browser/history/old?profile=default&priority=NORMAL



and after that the favicon table is empty.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #3)
> The issue *appears* to be simple: when we insert the default favicons, we
> don't set a creation time or modification time.

I misspoke. We do set those, but they apparently don't make a difference.

Comment 5

[Richard Newman [:rnewman]]

... because we now batch-insert favicons and bookmarks, there's no relationship between the two, so the cleanup step deletes them for being unused. Well, that makes sense.

Comment 6

[Richard Newman [:rnewman]]

Attachment: Link bookmarks to their favicon IDs during bulk insertion. v1

Bulk insertion is enough of a win that I want to keep it.

We're inserting the favicons and the bookmarks at about the same time. So here's a trick: we select the IDs for the favicons, rather than having them be allocated by sqlite. And rather than trusting that the DB is empty (it should be safe to do so, but why risk it?), we pick IDs from a different number line: the negative numbers.

This patch adds code to allocate each favicon a unique negative ID, paying attention to the bookmark offset to keep distribution and default bookmarks from colliding.

Testing produces log output like this:

 Calling insert in transaction on URI: content://org.mozilla.fennec_rnewman.db.browser/favicons?profile=default
 Insert on FAVICONS: content://org.mozilla.fennec_rnewman.db.browser/favicons?profile=default
 Inserting favicon for URL: null
 Inserted ID in database: -1
 Calling insert in transaction on URI: content://org.mozilla.fennec_rnewman.db.browser/favicons?profile=default
 Insert on FAVICONS: content://org.mozilla.fennec_rnewman.db.browser/favicons?profile=default
 Inserting favicon for URL: null
 Inserted ID in database: -2
 Calling insert in transaction on URI: content://org.mozilla.fennec_rnewman.db.browser/favicons?profile=default
 Insert on FAVICONS: content://org.mozilla.fennec_rnewman.db.browser/favicons?profile=default
 Inserting favicon for URL: null
 Inserted ID in database: -3
 Calling insert in transaction on URI: content://org.mozilla.fennec_rnewman.db.browser/favicons?profile=default
 Insert on FAVICONS: content://org.mozilla.fennec_rnewman.db.browser/favicons?profile=default
 Inserting favicon for URL: null
 Inserted ID in database: -4
 Calling insert in transaction on URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?profile=default
 Insert on BOOKMARKS: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?profile=default
 Inserting bookmark in database with URL: about:firefox
 Query is on bookmarks: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?profile=default&limit=6
 Using sort order position ASC.
 Running built query.
 Inserted ID in database: 6
 Calling insert in transaction on URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?profile=default
 Insert on BOOKMARKS: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?profile=default
 Inserting bookmark in database with URL: https://addons.mozilla.org/android/
 Inserted ID in database: 7
 Calling insert in transaction on URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?profile=default
 Insert on BOOKMARKS: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?profile=default
 Inserting bookmark in database with URL: https://marketplace.firefox.com/
 Inserted ID in database: 8
 Calling insert in transaction on URI: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?profile=default
 Insert on BOOKMARKS: content://org.mozilla.fennec_rnewman.db.browser/bookmarks?profile=default
 Inserting bookmark in database with URL: https://support.mozilla.org/products/mobile
 Inserted ID in database: 9

Manual testing confirms that this fixes the problem.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8459011 [details] [diff] [review]
Link bookmarks to their favicon IDs during bulk insertion. v1

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

This seems reasonable to me. I just have a few questions about the details of how this works. Maybe inspiration for some inline comments!

::: mobile/android/base/db/LocalBrowserDB.java
@@ +197,2 @@
>                  if (iconValue != null) {
> +                    final int faviconID = faviconIDs.get(name);

Under what circumstance would we call get twice for the same name? The term "name" here is causing me a bit of confusion - it's a representation of the whole bookmark, right?

I'm just wondering why we can't just use a simpler counter that just returns successive IDs whenever it is called.

Also, do we ever attempt to insert duplicate favicons? What happens if two default or distribution bookmarks have the same icon?

Comment 8

[Richard Newman [:rnewman]]

> Also, do we ever attempt to insert duplicate favicons? What happens if two
> default or distribution bookmarks have the same icon?

Default bookmarks are read out of the resource class using Java reflection.

Their icons are either files on disk, or a path. Here are two entries from strings.xml:

  <string name="bookmarkdefaults_favicon_aboutfirefox">chrome/chrome/content/branding/favicon64.png</string>
  <!-- Icon is automatically generated from R.drawable.bookmarkdefaults_favicon_addons -->

We don't offer a way to use a resource (file) twice. If two default bookmarks have the same icon resource, it appears twice on disk and in the DB.

We do allow the same chrome/content stuff to be used more than once, but we insert it into the DB twice, because addDefaultBookmarks is simple: it grabs a handle to a Bitmap, rather than introspecting.

None of this is a problem right now, because each bookmark has a different icon.


Distribution bookmarks contain favicons as data: URIs. These might collide during insertion, but we don't handle this particularly elegantly.

We don't offer a way to have distribution bookmarks point at resources (though this would be easy to do) or file paths.


I want to fix all this, which is why I plowed on with the approach in this patch: counters don't work if different bookmarks share icons. But perhaps I should save that for a follow-up, and drop back to counters for now.

Do you have an opinion?

Comment 9

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #8)
> > Also, do we ever attempt to insert duplicate favicons? What happens if two
> > default or distribution bookmarks have the same icon?
> 
> Default bookmarks are read out of the resource class using Java reflection.
> 
> Their icons are either files on disk, or a path. Here are two entries from
> strings.xml:
> 
>   <string
> name="bookmarkdefaults_favicon_aboutfirefox">chrome/chrome/content/branding/
> favicon64.png</string>
>   <!-- Icon is automatically generated from
> R.drawable.bookmarkdefaults_favicon_addons -->
> 
> We don't offer a way to use a resource (file) twice. If two default
> bookmarks have the same icon resource, it appears twice on disk and in the
> DB.
> 
> We do allow the same chrome/content stuff to be used more than once, but we
> insert it into the DB twice, because addDefaultBookmarks is simple: it grabs
> a handle to a Bitmap, rather than introspecting.
> 
> None of this is a problem right now, because each bookmark has a different
> icon.
> 
> 
> Distribution bookmarks contain favicons as data: URIs. These might collide
> during insertion, but we don't handle this particularly elegantly.
> 
> We don't offer a way to have distribution bookmarks point at resources
> (though this would be easy to do) or file paths.
> 
> 
> I want to fix all this, which is why I plowed on with the approach in this
> patch: counters don't work if different bookmarks share icons. But perhaps I
> should save that for a follow-up, and drop back to counters for now.
> 
> Do you have an opinion?

I generally favor the simplest approach, since we don't necessarily know what this future duplicate icon fix will look like.

However, the abstraction you introduced in this patch is pretty simple, so I think it would be fine to land it as-is, with some comments explaining about why things are the way they are. Also, file a bug for these future improvements. Maybe it could be a mentor bug :)

Comment 10

[Richard Newman [:rnewman]]

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

Comment 11

[Richard Newman [:rnewman]]

Added commenting, and also removed the possibility of duplicate insertion of distribution icons while I was in the area. Will file a follow-up for the rest, and request uplift if this looks good in Nightly.

Thanks, Margaret!

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bf683354136b

Comment 13

[Aaron Train [:aaronmt]]

Verified FIXED on trunk (34.0a1); needed on Aurora (33.0a2)

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8459011 [details] [diff] [review]
Link bookmarks to their favicon IDs during bulk insertion. v1

Approval Request Comment
[Feature/regressing bug #]:
  See dependency. 

[User impact if declined]:
  Default bookmark favicons will be deleted after first use of the browser. 

[Describe test coverage new/current, TBPL]:
  QA verified. 

[Risks and why]: 
  Low risk, totally necessary. 

[String/UUID change made/needed]:
  None.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89dbad79b0b3

Comment 16

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Samsung Galaxy Nexus
OS: Android 4.2.1
Default bookmark favicons are displayed after browser restart, so:
Verified fixed on:
Build: Firefox for Android 33.0a2 (2014-07-24)