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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: 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)

Comment 1

[Aaron Train [:aaronmt]]

Wes/Brian/Lucas?

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Pull favicon ID from history when adding bookmarks

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.

Comment 3

[Brian Nicholson (:bnicholson)]

The error in comment 0 is from a separate, unrelated bug - I've filed bug 813346.

Comment 4

[Lucas Rocha (:lucasr)]

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.

Comment 5

[Brian Nicholson (:bnicholson)]

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

Comment 6

[Brian Nicholson (:bnicholson)]

(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

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Pull favicon ID from history when adding bookmarks, v2

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.

Comment 8

[Lucas Rocha (:lucasr)]

(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 9

[Brian Nicholson (:bnicholson)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 683688 [details] [diff] [review]
Pull favicon ID from history when adding bookmarks, v2

Firefox 19 regression. Approving for Aurora.

Comment 13

[Cristian Nicolae (:xti)]

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

Comment 14

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/1a365608dd7d

Comment 15

[Cristian Nicolae (:xti)]

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