Closed Bug 812805 Opened 8 years ago Closed 8 years ago

Awesomescreen: Favicon shown in Top Sites but not in Bookmarks for same url

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 20
Tracking Status
firefox19 --- fixed
firefox20 --- verified
firefox21 --- verified

People

(Reporter: aryx, Assigned: bnicholson)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached file logcat
Firefox for Android nightly 20121117, Android 4.1.2 (stock), Google Nexus S

I have http://www.computerbase.de/ in the 'Top Sites' of the awesomescreen where a favicon is shown. It has the star for being a bookmark and in the 'Bookmarks' tab the favicon is not shown, even not after visiting the page.

From the logcat:

I/ActivityManager(  247): START {flg=0x40010000 cmp=org.mozilla.fennec/org.mozilla.gecko.AwesomeBar (has extras) u=0} from pid 18592
D/GeckoAwesomeBar(18592): creating awesomebar
I/GeckoViewsFactory(18592): Creating custom Gecko view: BrowserToolbarBackground
I/GeckoViewsFactory(18592): Creating custom Gecko view: CustomEditText
D/GeckoViewsFactory(18592): Warning: unknown custom view: CustomEditText
I/GeckoViewsFactory(18592): Creating custom Gecko view: AwesomeBarTabs
D/GeckoAwesomeBarTabs(18592): Creating AwesomeBarTabs
I/GeckoViewsFactory(18592): Creating custom Gecko view: AwesomeBarTabs.Background
D/skia    (18592): --- SkImageDecoder::Factory returned null
I/ALL_PAGES(18592): exception while decoding bitmap: data:image/x-icon;base64,AAABAAEAEBAAAAEACABoBQAAFgAAACgAAAAQAAAAIAAAAAEACAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADAgMAFhcWACcmJwA3ODcARkVGAFZXVgBnZ2cAe3x7AIWEhQCWlZYAqKeoALe4twDDxcMA19fXAObn5gD7/fsAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAMAAAACBAAAAAAAAAAAAAsLAgAACQ0AAAAAAAAAAAUNDQUABA0NAwAAAAAAAAAHDAgFAggNBgcAAAAAAAAADQoBBQkMCQAHAQAAAAAABw8FAAIMDQQABgYAAAAAAAsLAgACDAsAAAIIAAAAAAQOBwAABg8KAAABBwQAAAAKDgMAAgsNBwUAAAQFAAACDgkBAAYOBQMJAAABCQAABw0EAAALDQIBBwYAAQkGBAoLBAAFCwgCAAgKAgIJCgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
I/ALL_PAGES(18592): java.lang.IllegalArgumentException: bad base-64
I/ALL_PAGES(18592): 	at android.util.Base64.decode(Base64.java:161)
I/ALL_PAGES(18592): 	at android.util.Base64.decode(Base64.java:136)
I/ALL_PAGES(18592): 	at android.util.Base64.decode(Base64.java:118)
I/ALL_PAGES(18592): 	at org.mozilla.gecko.AllPagesTab.getBitmapFromDataURI(AllPagesTab.java:580)
I/ALL_PAGES(18592): 	at org.mozilla.gecko.AllPagesTab.setSearchEngines(AllPagesTab.java:556)
I/ALL_PAGES(18592): 	at org.mozilla.gecko.AllPagesTab.access$1500(AllPagesTab.java:61)
I/ALL_PAGES(18592): 	at org.mozilla.gecko.AllPagesTab$8.run(AllPagesTab.java:665)
I/ALL_PAGES(18592): 	at android.os.Handler.handleCallback(Handler.java:615)
I/ALL_PAGES(18592): 	at android.os.Handler.dispatchMessage(Handler.java:92)
I/ALL_PAGES(18592): 	at android.os.Looper.loop(Looper.java:137)
I/ALL_PAGES(18592): 	at android.app.ActivityThread.main(ActivityThread.java:4745)
I/ALL_PAGES(18592): 	at java.lang.reflect.Method.invokeNative(Native Method)
I/ALL_PAGES(18592): 	at java.lang.reflect.Method.invoke(Method.java:511)
I/ALL_PAGES(18592): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:786)
I/ALL_PAGES(18592): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
I/ALL_PAGES(18592): 	at dalvik.system.NativeStart.main(Native Method)

There are also these errors:

D/StrictMode(18592): StrictMode policy violation; ~duration=58 ms: android.os.StrictMode$StrictModeDiskReadViolation: policy=31 violation=2
D/StrictMode(18592): 	at android.os.StrictMode$AndroidBlockGuardPolicy.onReadFromDisk(StrictMode.java:1107)
D/StrictMode(18592): 	at android.database.sqlite.SQLiteConnection.applyBlockGuardPolicy(SQLiteConnection.java:1034)
Wes/Brian/Lucas?
Hmm...I overlooked this one in the favicon schema change. updateFaviconForUrl() works fine for updating existing bookmark entries, but we have a problem if we add a bookmark once we've already fetched the favicon (like when we click the star icon in the menu).

The bookmarks and history table each keep track of favicon_ids separately, so when adding a bookmark, we need to make sure the bookmarks entry has a favicon_id assigned to it. This patch does it by pulling favicon_id from the history table. This is one reason why normalizing our DB further would be useful; with favicon_id being tracked for URLs in two different places, it's possible for things to get out of sync.
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
Attachment #683324 - Flags: review?(lucasr.at.mozilla)
Blocks: 784086
Depends on: 813346
No longer depends on: 813346
The error in comment 0 is from a separate, unrelated bug - I've filed bug 813346.
Comment on attachment 683324 [details] [diff] [review]
Pull favicon ID from history when adding bookmarks

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +545,5 @@
> +        if (c.moveToFirst()) {
> +            int columnIndex = c.getColumnIndexOrThrow(History.FAVICON_ID);
> +            if (!c.isNull(columnIndex)) {
> +                faviconId = c.getLong(columnIndex);
> +            }

No need for the {}'s

@@ +555,5 @@
>          values.put(Browser.BookmarkColumns.TITLE, title);
>          values.put(Bookmarks.URL, uri);
>          values.put(Bookmarks.PARENT, folderId);
>          values.put(Bookmarks.DATE_MODIFIED, now);
> +        values.put(Bookmarks.FAVICON_ID, faviconId);

Shouldn't the faviconId only be set if you're inserting a new bookmark? I guess you don't want to overwrite whatever favicon id is already set in an existing bookmark entry.
Attachment #683324 - Flags: review?(lucasr.at.mozilla) → review+
I wonder if we should instead do this in BrowserDB. Otherwise, we'll still be missing for any insert operations that don't go through LocalBrowserDB; e.g., http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidImport.java#73
(In reply to Brian Nicholson (:bnicholson) from comment #5)
> I wonder if we should instead do this in BrowserDB. Otherwise, we'll still
> be missing for any insert operations that don't go through LocalBrowserDB;
> e.g.,
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> AndroidImport.java#73

Actually, that was a bad example since we update the favicon right after that. But here's a place where we don't: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ProfileMigrator.java#967
I guess I'll just stick with this patch; moving it to the BrowserDB is really only a band-aid fix that doesn't cover all cases anyway (it only works if the bookmark being added is still in the history). The real way to fix this issue is to further normalize the database by having bookmarks refer to IDs in a places/history table.

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Comment on attachment 683324 [details] [diff] [review]
> Pull favicon ID from history when adding bookmarks
> 
> Review of attachment 683324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shouldn't the faviconId only be set if you're inserting a new bookmark? I
> guess you don't want to overwrite whatever favicon id is already set in an
> existing bookmark entry.

When does addBookmarkItem() not insert a new bookmark? I thought the update() was only there to unmark entries that have been deleted, if applicable - and in that case, shouldn't the bookmark have the updated favicon if it's changed?

Setting r? again for question and minor changes in the patch.
Attachment #683324 - Attachment is obsolete: true
Attachment #683688 - Flags: review?(lucasr.at.mozilla)
Attachment #683688 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #7)
> When does addBookmarkItem() not insert a new bookmark? I thought the
> update() was only there to unmark entries that have been deleted, if
> applicable - and in that case, shouldn't the bookmark have the updated
> favicon if it's changed?

You're right. r+
Comment on attachment 683688 [details] [diff] [review]
Pull favicon ID from history when adding bookmarks, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 784086
User impact if declined: new bookmarks will be missing favicons
Testing completed (on m-c, etc.): just landed m-i
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #683688 - Flags: approval-mozilla-aurora?
Keywords: regression
https://hg.mozilla.org/mozilla-central/rev/abe54b7b33ec
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Duplicate of this bug: 814668
Comment on attachment 683688 [details] [diff] [review]
Pull favicon ID from history when adding bookmarks, v2

Firefox 19 regression. Approving for Aurora.
Attachment #683688 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Favicons are displayed everywhere in Awesomescreen on the latest Nightly build. Leaving the bug open until the patch will land on m-a.

--
Firefox 20.0a1 (2012-11-28)
Device: Galaxy Nexus
OS: Android 4.1.1
I cannot reproduce it on the latest Aurora either. Closing bug as verified fixed on:

Firefox for Android
Version: 20.0a2 (2013-01-30)
Device: Galaxy R
OS: Android 2.3.4
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.