Closed
Bug 1040806
Opened 9 years ago
Closed 9 years ago
Default bookmark favicons are not displayed after browser restart
Categories
(Firefox for Android Graveyard :: Favicon Handling, defect)
Tracking
(firefox31 unaffected, firefox32 unaffected, firefox33+ verified, firefox34 verified, fennec33+)
VERIFIED
FIXED
Firefox 34
Tracking | Status | |
---|---|---|
firefox31 | --- | unaffected |
firefox32 | --- | unaffected |
firefox33 | + | verified |
firefox34 | --- | verified |
fennec | 33+ | --- |
People
(Reporter: aaronmt, Assigned: rnewman)
References
Details
(Keywords: regression, reproducible)
Attachments
(2 files)
154.58 KB,
image/png
|
Details | |
9.30 KB,
patch
|
Margaret
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Reporter | ||
Comment 1•9 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•9 years ago
|
||
Hm, I thought that was another (older) bug surfacing. Let me take a look.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 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•9 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•9 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•9 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•9 years ago
|
||
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)
Updated•9 years ago
|
Comment 7•9 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•9 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•9 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 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/bf683354136b
Assignee | ||
Comment 11•9 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
Closed: 9 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 13•9 years ago
|
||
Verified FIXED on trunk (34.0a1); needed on Aurora (33.0a2)
Status: RESOLVED → VERIFIED
status-firefox34:
--- → verified
Assignee | ||
Comment 14•9 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)
Updated•9 years ago
|
Attachment #8459011 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 16•9 years ago
|
||
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)
Updated•9 years ago
|
tracking-fennec: ? → 33+
Updated•2 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
•