Closed Bug 962025 Opened 8 years ago Closed 7 years ago

favicon of default bookmarks changes after upgrading firefox

Categories

(Firefox for Android Graveyard :: General, defect)

27 Branch
ARM
Android
defect
Not set
normal

Tracking

(fennec-)

RESOLVED WONTFIX
Tracking Status
fennec - ---

People

(Reporter: angelc04, Assigned: rnewman)

Details

(Keywords: regression, reproducible)

Attachments

(3 files)

This is a regression bug.

1. Install Firefox V26.0
2. Launch Firefox, you will see favicons for default bookmarks show correct icon.
3. Install firefox v27.0
4. Launch Firefox and check Bookmarks
   --> favicons for addons.mozilla.org and support.mozilla.org changed.

upgrading from v26.0 to v26.0.1 has no problem.
BTW, only default bookmarks has this problem. User added bookmarks shows favicon as expected.
Attached image screenshot.png
I see no difference whatsoever.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Aaron, I can consistently reproduce this. Here are the tests I have done.

Release versions:
fennec 25.0 -> fennec 26.0			Pass
fennec 26.0 -> fennec 26.0.1			Pass
fennec 26.0 -> fennec 27.0 (local compiled)	Fail

beta versions:
fennec 25.0b8 -> fennec 26.0b10			Pass
fennec 26.0b2 -> fennec 26.0b8			Pass
fennec 26.0b10 -> fennec 27.0b8			Fail
fennec 26.0b10 -> fennec 27.0b1			Fail
fennec 27.0b6 -> fennec 27.0b8			Pass

So the conclusion is:
1. This is a regression bug. 
2. It only appears when upgrading from 26.0 to 27.0 build.
3. It cannot be reproduced by upgrading between 27 beta builds.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
The device I used. 
Samsung GT-P7510, Android 4.0.4
MIUI 2S, android 4.1.1
Summary: favicon of default bookmarks changes after upgrading firefox → favicon of default bookmarks changes after upgrading firefox (non en-US)
Aaron, why "non en-US" was added to the summary? I dont't think this is related to locale bcz the behavior is the same to en-us locale.

I think this might relate to Favicon Revamp in v27.0 (Bug 914296). But still need to do some test to narrow down the cause.
Flags: needinfo?(aaron.train)
I haven't been able to reproduce this at all. If you have steps, they would be much appreciated.
Flags: needinfo?(aaron.train)
Keywords: steps-wanted
Sorry for the late reply. I'm currently on holiday.

Here are the steps I used.
0. prepare a clean device
1. install any v26.0 beta build
2. Launch firefox, you will see all bookmark icons shows correctly
3. upgrade firefox to any v27.0 beta build
4. Launch firefox again
   --> favicons for addons.mozilla.org and support.mozilla.org changed
Keywords: qawanted
Thanks.

Using the steps in comment #9 with 

http://ftp.mozilla.org/pub/mozilla.org/mobile/candidates/26.0b10-candidates/build1/android/multi/fennec-26.0b10.multi.android-arm.apk

Launched, and then upgrading to the first 27 beta candidate 
http://ftp.mozilla.org/pub/mozilla.org/mobile/candidates/27.0b1-candidates/build1/android/multi/fennec-27.0b1.multi.android-arm.apk

I was able to reproduce with only an upgraded profile.

On startup with the same dirty profile, I see:

D/LoadFaviconTask(15263): Fetching favicon from JAR.
D/GeckoFavicons(15263): Cancelling favicon load (4)

Perhaps our resident favicon experts might know what happened between 26 and 27 in this timeframe.
tracking-fennec: --- → ?
Flags: needinfo?(rnewman)
Flags: needinfo?(chriskitching)
Summary: favicon of default bookmarks changes after upgrading firefox (non en-US) → favicon of default bookmarks changes after upgrading firefox
I wonder if this is fallout from bug 941868
Flags: needinfo?
Flags: needinfo?
We touched how some distributed icons were handled (the old way was dumb from an I/O standpoint), but that should only affect about:home.

Icons for built-in bookmarks should still be in the DB from the prior install (filled from the distribution file), and should pass through the cache as normal when subsequently viewed.

Why test against the *first* 27 candidate, not the last?

What happens if you now visit one of those bookmarks?
Flags: needinfo?(rnewman)
In our latest candidate (26.0b10 to 27.0b10) on an upgraded profile, launch:

D/FaviconCache(19661): Favicon cache fullness: 0/524288
D/GeckoTabs(19661): Setting about: tab favicon inline.
D/GeckoToolbar(19661): setFavicon(android.graphics.Bitmap@43476730)
D/GeckoToolbar(19661): setFavicon(android.graphics.Bitmap@43476730)
D/GeckoToolbar(19661): Ignoring favicon set: new favicon is identical to previous favicon.
D/GeckoToolbar(19661): onTabChanged: FAVICON
D/GeckoToolbar(19661): setFavicon(android.graphics.Bitmap@435c6468)
D/GeckoBrowserApp(19661): BrowserApp.onTabChanged: 0: FAVICON
D/GeckoToolbar(19661): setFavicon(android.graphics.Bitmap@435c6468)
D/GeckoToolbar(19661): Ignoring favicon set: new favicon is identical to previous favicon.
D/GeckoToolbar(19661): onTabChanged: FAVICON
D/GeckoToolbar(19661): setFavicon(android.graphics.Bitmap@43476730)
D/GeckoBrowserApp(19661): BrowserApp.onTabChanged: 0: FAVICON
D/GeckoToolbar(19661): setFavicon(android.graphics.Bitmap@43476730)
D/GeckoToolbar(19661): Ignoring favicon set: new favicon is identical to previous favicon.

On access, both tabs open they both do not update their favicons (see screenshot)

D/GeckoBrowserApp(19661): BrowserApp.onTabChanged: 5: FAVICON
D/GeckoToolbar(19661): setFavicon(android.graphics.Bitmap@4389b578)
D/GeckoToolbar(19661): Ignoring favicon set: new favicon is identical to previous favicon.
The fact this is only reproducible when you update from 26 to 27 suggests it might be a bug in one of the many updateDatabaseFromXToY functions in BrowserProvider.java.
But, the latest of those is upgradeDatabaseFrom16to17, added in July 2013 (that's before I came along and started poking favicons with a stick). It's deleting favicons with zero-length payloads - that shouldn't cause a problem here.

The not-updating-on-load-despite-valid-icon-existing-on-the-internet problem suggests whatever failure caused them to not load in the first place had them stuck into the failed cache. (Which never, ever clears during a session. There's a bug about that. (Although possibly not a bug..? Another story for another time.)).

This view uses getSizedFaviconForPageFromLocal to load favicons. This sets the mOnlyFromLocal switch in LoadFaviconTask. This switch incorrectly causes the task to abort after checking the database but before loading from the jar. RNewman - did you do this on purpose, or is that a bug?
That certainly would cause favicons that are in jars but not the database to fail to be loaded. It's also against the spirit of the mOnlyFromLocal flag - so I suggest the flag either wants renaming to reflect its actual semantics, or that the jar should be checked in these cases.

I hope some of the above is, at worst, almost but not quite entirely unuseful in resolving this bug.
Flags: needinfo?(chriskitching)
My speculation: the favicon URI for the distribution bookmarks changed, but of course we don't re-process the distribution file.
(In reply to Chris Kitching [:ckitching] from comment #14)

> But, the latest of those is upgradeDatabaseFrom16to17, added in July 2013
> (that's before I came along and started poking favicons with a stick). It's
> deleting favicons with zero-length payloads - that shouldn't cause a problem
> here.

This looks like it might be the problem. I just looked in my own Nightly profile's DB and immediately found an entry with a broken favicon foreign key relation. (Not one of these...)

sqlite> select favicon_id from bookmarks where title = 'FAQ''s & Support';
favicon_id
----------------------------------------
1109

sqlite> select count(*) from favicons where _id = 1109;
count(*)
----------------------------------------
0



> This view uses getSizedFaviconForPageFromLocal to load favicons. This sets
> the mOnlyFromLocal switch in LoadFaviconTask. This switch incorrectly causes
> the task to abort after checking the database but before loading from the
> jar. RNewman - did you do this on purpose, or is that a bug?

These icons are data: URIs.  It's nothing to do with JAR loading.
Aaron, could you grab browser.db from each profile before and after upgrade? You can use 

https://addons.mozilla.org/en-US/android/addon/copy-profile/

to do it.
Flags: needinfo?(aaron.train)
Thanks, Aaron.

Here's the difference.

26: bookmarks table has no favicon_ids or favicon_urls. favicons table is empty.
27: bookmarks table has favicon_ids 1, 2, 3. favicons table contains ids 1, 2, 3, 5, each apparently PNGs. Only one has a URL, which I deduce is from Aaron visiting SUMO.

I have no idea how 26 is able to display these favicons at all, 'cos the data ain't in the DB.

In 27, the data seems to be there, so the implication is that one of these things is occurring:

* The displayed bookmark somehow isn't being found in the DB.
* The join between bookmark and favicon isn't working.
* We're failing to decode the retrieved icon.
* Something else.
Assignee: nobody → rnewman
tracking-fennec: ? → 27+
Status: REOPENED → ASSIGNED
It's vanishingly unlikely that I'll make progress on this before 27 ships. If that bugs y'all, let me know (and ideally find another assignee).

If this applies to user-added bookmarks I'd be much more concerned. pcheng, Aaron, does it?
Flags: needinfo?(pcheng)
(In reply to Richard Newman [:rnewman] from comment #20)
> It's vanishingly unlikely that I'll make progress on this before 27 ships.
> If that bugs y'all, let me know (and ideally find another assignee).
> 
> If this applies to user-added bookmarks I'd be much more concerned. pcheng,
> Aaron, does it?

User-added bookmarks does not have this problem.
Flags: needinfo?(pcheng)
Thanks for clarifying. Re-noming.
tracking-fennec: 27+ → ?
Assume you mean 28, which ships week of the 18th.
Yes, thanks for the catch. This bug was tracking 27+, which twisted my brain.

Given that this bug affects upgrades from 26 to 27, we might just RESOLVED OHWELL this...
rnewman, up to you if you want to WONTFIX this.
tracking-fennec: ? → -
Horse is gone, won't spend time bolting the barn door.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → WONTFIX
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.