Default bookmark favicons are not displayed after browser restart

VERIFIED FIXED in Firefox 33

Status

()

Firefox for Android
Favicon Handling
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: aaronmt, Assigned: rnewman)

Tracking

(Blocks: 1 bug, {regression, reproducible})

Trunk
Firefox 34
All
Android
regression, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 unaffected, firefox32 unaffected, firefox33+ verified, firefox34 verified, fennec33+)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 8458709 [details]
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)
(Reporter)

Updated

4 years ago
status-firefox31: --- → unaffected
status-firefox32: ? → unaffected
tracking-firefox33: --- → ?
(Reporter)

Comment 1

4 years ago
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
Blocks: 1016611
Keywords: regressionwindow-wanted
(Assignee)

Comment 2

4 years ago
Hm, I thought that was another (older) bug surfacing. Let me take a look.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Summary: Default bookmark Favicon's are not displayed after browser restart → Default bookmark favicons are not displayed after browser restart
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
... 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.
(Assignee)

Comment 6

4 years ago
Created attachment 8459011 [details] [diff] [review]
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.
Attachment #8459011 - Flags: review?(margaret.leibovic)
tracking-firefox33: ? → +

Comment 7

4 years ago
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?
Attachment #8459011 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 8

4 years ago
> 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

4 years ago
(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 :)
(Assignee)

Comment 11

4 years ago
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!
Flags: needinfo?(rnewman)
Hardware: ARM → All
Target Milestone: --- → Firefox 34
https://hg.mozilla.org/mozilla-central/rev/bf683354136b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Comment 13

4 years ago
Verified FIXED on trunk (34.0a1); needed on Aurora (33.0a2)
Status: RESOLVED → VERIFIED
status-firefox34: --- → verified
(Assignee)

Comment 14

4 years ago
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.
Attachment #8459011 - Flags: approval-mozilla-aurora?
Flags: needinfo?(rnewman)
Attachment #8459011 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

4 years ago
Blocks: 1042736
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)
status-firefox33: fixed → verified
tracking-fennec: ? → 33+
You need to log in before you can comment on or make changes to this bug.