1044940 - Favicons in the bookmarks table are neither read correctly nor written

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Chris Kitching [:ckitching]]

While working on Bug 1044794, I went hunting for the code that updates the favicon_id field in the bookmarks database.

I didn't find any.
My bookmarks database is chock-full of nulls in that column.

Whoops.

Further investigation suggests that code that reads favicons doesn't exploit this field, either. When loading a favicon, we take the page URL, look in the history table to find the favicon_id, find that in the favicons table, and get the URL.

That works just fine, except for things we've bookmarked but never visited (or visited, but since deleted the history).

Comment 1

[Mark Capella [:capella]]

Attachment: bug1044940.diff

Keep in mind it's just a wip for you to review the approach ... you might have something radically different in mind already, knowing the codebase better than I do

Comment 2

[Mark Capella [:capella]]

Forgot to mention, this depends on bug 1016686

Comment 3

[Chris Kitching [:ckitching]]

Comment on attachment 8464204 [details] [diff] [review]
bug1044940.diff

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

Looks like something potentially interesting, but wrong bug.

Filed Bug 1045843 for this one, let's talk about it there.

Comment 4

[Mark Capella [:capella]]

Attachment: bug1044940.diff

maybe this is closer ?

Comment 5

[Mark Capella [:capella]]

Attachment: bug1044940.diff

meh - forgot a qref ...

This seems to fix kat's bug 1051544 also.

Comment 6

[Mark Capella [:capella]]

Nice! With the update of the pageURL->faviconURL memCache on page load, and pretty-much-100% chance of finding the favicon in the local DB (History/Bookmark table) I'm seeing a much snappier transition into reading mode, as we avoid a favicon load from network.

Comment 7

[Chris Kitching [:ckitching]]

Comment on attachment 8471715 [details] [diff] [review]
bug1044940.diff

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

Going in a much more sane direction this time: looks like it actually fixes the bug.

Is f=, in fact, a commit message field we use?

In general: This is doing all the right things, but in all the wrong ways. We're almost there!

::: mobile/android/base/db/BrowserDB.java
@@ +233,1 @@
>      }

As I mention elsewhere, it should be possible to aggregate many of these methods into one: we don't want to provide an API for updating ids in just a single field: that'll allow people to write code that moves the database into a less consistent state.
We want to hide the fact that our database is evil and denormalised, otherwise it's just asking for someone to come along and forget to update one of the tables, bringing this bug back (again).

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1147,5 @@
> +
> +        try {
> +            c = cr.query(mBookmarksUriWithProfile,
> +                         new String[] { Bookmarks.FAVICON_URL },
> +                         Combined.URL + " = ?",

Bookmarks.URL (yes, they're equal... for now). Use the constant from the table that you're querying.

@@ +1152,5 @@
> +                         new String[] { uri },
> +                         null);
> +
> +            if (c.moveToFirst())
> +                return c.getString(c.getColumnIndexOrThrow(Bookmarks.FAVICON_URL));

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Braces on ifs.

@@ +1155,5 @@
> +            if (c.moveToFirst())
> +                return c.getString(c.getColumnIndexOrThrow(Bookmarks.FAVICON_URL));
> +        } finally {
> +            if (c != null)
> +                c.close();

Braces on ifs.

@@ +1178,5 @@
> +                         Favicons.URL + " = ?",
> +                         new String[] { faviconUri });
> +    }
> +
> +    public Integer getIDForFaviconURL(ContentResolver cr, String faviconURL) {

Javadoc comments for new methods. What does it do? What does it assume? What should it *not* do?

Unless you need to handle nulls, never return Integer, return int. (and if you need to handle nulls, consider using a sentinel int value instead of an Integer).
This is due to the madness that occurs in bytecode if you take an Integer and start using it as an int in many situations (remember what happens when you do ++?). (identical advice applies for Float, Boolean, Long, etc.)

@@ +1181,5 @@
> +
> +    public Integer getIDForFaviconURL(ContentResolver cr, String faviconURL) {
> +        final Cursor c = cr.query(mFaviconsUriWithProfile,
> +                                  new String[] { Favicons._ID },
> +                                  Favicons.URL + " = ? AND " + Favicons.DATA + " IS NOT NULL",

Why the heck isn't data declared NOT NULL? Please file. It's sort of pointless for a row to exist here without data. (same goes for URL. I suggest we get this into the next db revision and drop all rows in old databases that have either as null: these rows are of no value).

@@ +1188,5 @@
> +
> +        try {
> +            final int col = c.getColumnIndexOrThrow(Favicons._ID);
> +            return (c.moveToFirst() && !c.isNull(col)) ?
> +                c.getInt(col) : null;

Expand ternary if to aid clarity: this takes a few seconds to figure out as it is.

@@ +1214,5 @@
> +
> +        return cr.update(mBookmarksUriWithProfile,
> +                         values,
> +                         Bookmarks.URL + " = ?",
> +                         new String[] { pageURL });

These two methods really should be one: It's dangeorus to permit update of just one of two things that should be kept consistent. Hide the denormalisation!

::: mobile/android/base/favicons/Favicons.java
@@ -52,1 @@
>      public static final int FLAG_SCALE   = 4;

Huh. Both PERSIST and SCALE have been moved inside LoadFaviconTask, so both of these can be deleted from here.

These flags also don't make a lot of sense to me. It's unclear that we'll ever want to make a non-persistent favicon load. All that means is "download this thing but don't store it in the DB". Do we ever do that? If you can't find an instance where FLAG_PERSIST is *not* set, please delete FLAG_PERSIST from LoadFaviconTask along with all related logic.

FLAG_SCALE seems never to be used. I think the original intent was to toggle the limited primary-upscaling logic in the favicon cache when satisfying a request, though that is both not implemented and never used, so let's get rid of the flag before someone tries to use it (we can always put it back along with its implementation if it turns out we need this later on).

@@ +148,5 @@
> +        if (cacheURL != null) {
> +            // Ensure pageURL->faviconURL mapping is updated when provided.
> +            pageURLMappings.put(pageURL, cacheURL);
> +            // Then ensure History and Boookmark favicon ID's are correct.
> +            updateFaviconIDsToDb(pageURL, cacheURL, persistFlags);

You're already updating the URLs in the database over in LoadFaviconTask after you've downloaded the favicon. 
This code will mean you update the URLs, then download the favicon, store it, then update the URLs again. 

You're using five database transactions and two background tasks when you could aggregate it all together and kill off this logic.

@@ +154,3 @@
>              cacheURL = pageURLMappings.get(pageURL);
> +            // If there's no favicon URL given, try and hit the cache with the default one.
> +            if (cacheURL == null)  {

Why have you restructured this method like this? Pushing these ifs inside each other just hurts readability... 

To quote Torvalds, "if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program." :P

@@ +166,4 @@
>          if (cachedIcon != null) {
>              return dispatchResult(pageURL, cacheURL, cachedIcon, listener);
> +        } else if (faviconsCache.isFailedFavicon(cacheURL)) {
> +            // Check if favicon has failed.

Again with the not-using-early-returns. What's the point of this change? It's purely cosmetic (and the opposite to one that rnewman's been pestering me about for months :P).

@@ +192,5 @@
> +                    BrowserDB.updateHistoryFaviconID(resolver, pageURL, id);
> +                    BrowserDB.updateBookmarksFaviconID(resolver, pageURL, id);
> +                }
> +            }
> +        });

This shouldn't exist. You already do this work in LoadFaviconTask, what's the point of doing it a second time?

@@ +274,5 @@
>          Tab theTab = Tabs.getInstance().getFirstTabForUrl(pageURL);
>          if (theTab != null) {
> +            faviconURL = theTab.getFaviconURL();
> +            if (faviconURL != null) {
> +                return faviconURL;

Do try and keep renaming variables and other assorted refactors in separate patches: makes it a little easier to review when I don't have to play spot the difference to check if you changed any logic while fixing up the naming conventions.

@@ -254,2 @@
>              }
>          }

Whoops. Might be worth refactoring this to use early-returns to reduce amount of nesting.

@@ +284,5 @@
> +        faviconURL = BrowserDB.getFaviconUrlForHistoryUrl(resolver, pageURL);
> +        if (faviconURL == null) {
> +            // If it's not in the History table, try to find it in the Bookmarks table.
> +            faviconURL = BrowserDB.getFaviconUrlForBookmarksUrl(resolver, pageURL);
> +            if (faviconURL == null) {

Again with the nesting.

::: mobile/android/base/favicons/LoadFaviconTask.java
@@ +111,5 @@
> +            if (id != null) {
> +                BrowserDB.updateHistoryFaviconID(resolver, pageURL, id);
> +                BrowserDB.updateBookmarksFaviconID(resolver, pageURL, id);
> +            }
> +        }

Here's the other place you're updating the favicon IDs. This is doing just the same as that updateFaviconIDsToDb method you added, and the two always seem to get called together...

Then, if you're only updating the ids here you don't need to add these two updateXFaviconID methods and can instead move their functionality into updateFaviconForUrl (as I suggested over in the other bug). I don't think it's ever useful to update *just* the id in *just* one table? We're always going to want to do all three things, so do them all together, all at once.

Comment 8

[Mark Capella [:capella]]

re: |Is f=, in fact, a commit message field we use?|
Yes :-) Feedback isn't as binding as review, but still deserves recognition.

Also, Thanks for the style nits that I let get past me due to cut'n'paste. I'll fix them up along with proper method JAVADOC. I tossed this all together for a rough WIP that I'm dog-fooding currently. 

re: |Unless you need to handle nulls, never return Integer, return int.|
I thought favicon_id's could be negative / positive, so I didn't think using -1 to mean "not found" would work. I'll actually verify this now.

re: |These flags also don't make a lot of sense to me. It's unclear that we'll ever want to make a non-persistent favicon load.|
iir ... Private Tabs don't / shouldn't persist to DB.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=efc6e6534451&mark=1657-1658#1652

re: |Why the heck isn't data declared NOT NULL? Please file|
Don't know, cut'n'paste, can do.

re: |Here's the other place you're updating the favicon IDs. This is doing just the same as that updateFaviconIDsToDb method you added|
Yes. I believe that one use called in getSizedFavicon() is for updating, and doesn't always trigger the second one saveFaviconToDb() use for new inserts.

I'll double check my homework, but this is why in this version, I didn't tie the update methods into updateFaviconForUrl() which is avoided in the first / updating case.

I'm ok with combining the updateHistoryFaviconID() with it's sister updateBookmarksFaviconID() in any case.

re: |Again with the not-using-early-returns.|
As Yourdan points out in "Techniques of Program Structure and Design", logical-cohesion groups functionally related processes into code blocks ... versus run-on if/exit, if/exit, if/exit.

It's logically equivalent, and I think easier to grok. However, I agree it can also be called cosmetic, and I can put it back if you'd prefer :)

Comment 9

[Chris Kitching [:ckitching]]

(In reply to Mark Capella [:capella] from comment #8)
> re: |Unless you need to handle nulls, never return Integer, return int.|
> I thought favicon_id's could be negative / positive, so I didn't think using
> -1 to mean "not found" would work. I'll actually verify this now.

AFAIK normal ids are always +ve (I think Richard did a clever thing at one point that made the bundled ones have negative ids. In which case, there's still plenty of -ve values for you to use).

Just a thought. It's a micro-optimisation, not worth going to too much trouble over.

> re: |These flags also don't make a lot of sense to me. It's unclear that
> we'll ever want to make a non-persistent favicon load.|
> iir ... Private Tabs don't / shouldn't persist to DB.

Ah, of course. More coffee needed.
In which case, just clean the flags up from Favicons.java and get rid of FLAG_SCALE.

> re: |Here's the other place you're updating the favicon IDs. This is doing
> just the same as that updateFaviconIDsToDb method you added|
> Yes. I believe that one use called in getSizedFavicon() is for updating, and
> doesn't always trigger the second one saveFaviconToDb() use for new inserts.
> 
> I'll double check my homework, but this is why in this version, I didn't tie
> the update methods into updateFaviconForUrl() which is avoided in the first
> / updating case.

Perhaps I wasn't clear enough.

You're never going to get a new favicon ID except when you've downloaded a new favicon.
That happens in LoadFaviconTask.
So update the favicon ids as part of the LoadFaviconTask instead of duplicating the logic all over the place (and doing it repeatedly in some cases).

> It's logically equivalent, and I think easier to grok. However, I agree it
> can also be called cosmetic, and I can put it back if you'd prefer :)

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

(Read it this time, please)

Comment 10

[Mark Capella [:capella]]

mmm? You'll have to link to something specific there ... not sure what I've missed that I haven't read before ... but thanks for the feedback.

re: |You're never going to get a new favicon ID except when you've downloaded a new favicon.
That happens in LoadFaviconTask.|
I mentioned I'd double-check this. I remember some situation in bug 1051615 where I found we can have the both the pageURL and the faviconURL, but the memory mapping isn't yet set.

Comment 11

[Mark Capella [:capella]]

Attachment: bug1044940.diff

Core / unfettered version of the patch that you're asking for. Let me know if I missed anything you asked for.

Comment 12

[Richard Newman [:rnewman]]

(In reply to Mark Capella [:capella] from comment #8)
> re: |Is f=, in fact, a commit message field we use?|
> Yes :-) Feedback isn't as binding as review, but still deserves recognition.

Chris is right, though: we don't use f= in commit messages for routine patches. Especially not when the f= matches the r=!


> re: |Unless you need to handle nulls, never return Integer, return int.|
> I thought favicon_id's could be negative / positive, so I didn't think using
> -1 to mean "not found" would work. I'll actually verify this now.

Historically they've always been positive autoincrementing integers, but we started using negatives in some very special cases: Bug 1040806. Integer.MIN_VALUE should be safe as a sentinel.


> re: |Again with the not-using-early-returns.|

Please stick with early returns. It's the style we use, it keeps things from nesting too far (a problem with four-space indents), and in our experience it makes it easier to analyze code flow and avoid errors.

I say that merely as someone who reviews a lot of code, and doesn't have a structured programming consulting business to promote!

Comment 13

[Mark Capella [:capella]]

Attachment: bug1044940v1.diff

Re-submitting the patch using Integer.MIN_VALUE as the "not found" sentinel, and changing feedback request to review request.

Comment 14

[Chris Kitching [:ckitching]]

Comment on attachment 8472806 [details] [diff] [review]
bug1044940v1.diff

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

Gave it a quick gander. Seems prettymuch on target, let's just take out the footguns.

::: mobile/android/base/db/BrowserDB.java
@@ +207,5 @@
>          return sDb.getFaviconForUrl(cr, faviconURL);
>      }
>  
> +    /**
> +     * Try to find a useable favicon URL in the bookmarks table.

Usable.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1122,5 @@
>          return FaviconDecoder.decodeFavicon(b);
>      }
>  
> +    /**
> +     * Try to find a useable favicon URL in the bookmarks table.

Usable.

@@ +1183,5 @@
> +                                      Favicons.URL + " = ?",
> +                                      new String[] { faviconUri });
> +
> +        // After writing the encodedFavicon, ensure the Bookmarks and/or History table's
> +        // favicon_id's are also up-to-date.

Invalid use of apostrophe: favicon_id's

@@ +1184,5 @@
> +                                      new String[] { faviconUri });
> +
> +        // After writing the encodedFavicon, ensure the Bookmarks and/or History table's
> +        // favicon_id's are also up-to-date.
> +        if (updated > 0) {

Invert if condition, return early.

@@ +1186,5 @@
> +        // After writing the encodedFavicon, ensure the Bookmarks and/or History table's
> +        // favicon_id's are also up-to-date.
> +        if (updated > 0) {
> +            final Integer id = getIDForFaviconURL(cr, faviconUri);
> +            if (id != Integer.MIN_VALUE) {

Might be worth making a named constant for this sentinel (with the same value).

@@ +1221,5 @@
> +
> +    /**
> +     * Update a history table entry's favicon ID after a new favicon table entry is added.
> +     */
> +    private void updateHistoryFaviconID(ContentResolver cr, String pageURL, int id) {

Would anyone ever want to call this method and *not* call updateBookmarksFaviconID immediately before (or after)?

Wouldn't that lead to database inconsistency?

It seems to make sense to combine these into a single method. That way, when someone writes new code that does favicon inserts they don't have to figure this out all over again.

More generally:
What we have here is database denormalisation: we're storing the same data in multiple places. That can be useful, but the cost is you have to keep all the copies up-to-date.
In that situation, it's generally a good idea to hide the denormalisation at the lowest abstraction level you can manage, instead of requiring higher-level code to remember to write to both copies.
Let's not build ourselves a convenient mechanism for shooting ourselves in the foot when some future developer gets a bit forgetful (or just didn't read this bug).

::: mobile/android/base/favicons/Favicons.java
@@ +259,3 @@
>          if (targetURL == null) {
> +            // If it's not in the History table, try to find it in the Bookmarks table.
> +            targetURL = BrowserDB.getFaviconURLForBookmarksURL(resolver, pageURL);

Again, let's hide the denormalisation. At the moment, application logic always has to remember to look in the bookmarks table, then look in the history table, then give up. Next month we will write new code and forget to do this properly.

Since nobody who's trying to resolve a page URL to a favicon URL is ever going to want to do half a job, you can make regressions less likely (and your code simpler!) by combining getFaviconURLForBookmarksURL and getFaviconUrlForHistoryUrl  into a single method in BrowserDB.

As an added bonus, that method might then be able to use just one query, not two, (a join of some sort with COALESCE seems like it might do the trick), which may well be faster.
What you want here is basically a (much, much) weaker version of the combined view. It's possible that querying that view might be the optimal approach for the time being (despite the fact it has a bunch of redundant stuff in it: by selecting only certain rows we can live in hope that the query parser will strip out most of the crap).

@@ +261,5 @@
> +            targetURL = BrowserDB.getFaviconURLForBookmarksURL(resolver, pageURL);
> +            if (targetURL == null) {
> +                // If we still can't find it, fall back to the default URL and hope for the best.
> +                targetURL = guessDefaultFaviconURL(pageURL);
> +            }

This if block doesn't need to be nested inside the other one: let's refactor this as something like:

targetURL = something;
// Did we get something sensible yet?
if (targetURL != null) {
  return targetURL
}

targetURL = somethingElse;
// How about now?
if (targetURL != null) {
   return targetURL;
}

etc.



You may also like to refactor the code earlier in this method to use a similar paradigm.

Comment 15

[Mark Capella [:capella]]

Attachment: bug1044940v1.diff

Comment 16

[Chris Kitching [:ckitching]]

Comment on attachment 8473392 [details] [diff] [review]
bug1044940v1.diff

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1189,5 @@
> +        }
> +
> +        // After writing the encodedFavicon, ensure that the favicon_id in both the bookmark and
> +        // history tables are also up-to-date.
> +        final Integer id = getIDForFaviconURL(cr, faviconUri);

You don't actually need to go query the database to get the ID of the favicon you just inserted. If you play your cards right, you can access it using last_insert_rowid:
http://www.sqlite.org/c3ref/last_insert_rowid.html

... which would perform rather a lot better, and mean you can throw away tons of code.

@@ +1234,5 @@
> +                  Bookmarks.URL + " = ?",
> +                  new String[] { pageURL });
> +
> +        final ContentValues historyValues = new ContentValues();
> +        historyValues.put(History.FAVICON_ID, id);

These two ContentValues objects are identical, but we can't reuse the first one because then we break if the constants are changed. Aww. :(

::: mobile/android/base/favicons/Favicons.java
@@ +261,4 @@
>          }
> +
> +        // If it's not in the History table, try to find it in the Bookmarks table.
> +        targetURL = BrowserDB.getFaviconURLForBookmarksURL(resolver, pageURL);

Again, let's hide the denormalisation. At the moment, application logic always has to remember to look in the bookmarks table, then look in the history table, then give up. Next month we will write new code and forget to do this properly.

Since nobody who's trying to resolve a page URL to a favicon URL is ever going to want to do half a job, you can make regressions less likely (and your code simpler!) by combining getFaviconURLForBookmarksURL and getFaviconUrlForHistoryUrl  into a single method in BrowserDB.

As an added bonus, that method might then be able to use just one query, not two, (a join of some sort with COALESCE seems like it might do the trick), which may well be faster.
What you want here is basically a (much, much) weaker version of the combined view. It's possible that querying that view might be the optimal approach for the time being (despite the fact it has a bunch of redundant stuff in it: by selecting only certain rows we can live in hope that the query parser will strip out most of the crap).

Comment 17

[Mark Capella [:capella]]

You want to replace the current getIDForFaviconURL() method with a new method that does basically the same thing?
http://www.davekb.com/browse_programming_tips:java_find_id_of_just_inserted_row:txt

Comment 18

[Mark Capella [:capella]]

I mean, I can do that if you see it as superior, or let me know if there's another method you have in mind. I only did a cursory google.

Comment 19

[Mark Capella [:capella]]

Attachment: bug1044940v1.diff

The current patch, with comment #18 not yet resolved.

Comment 20

[Mark Capella [:capella]]

review ping?

Comment 21

[Chris Kitching [:ckitching]]

Comment on attachment 8473414 [details] [diff] [review]
bug1044940v1.diff

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

(In reply to Mark Capella [:capella] from comment #17)
> You want to replace the current getIDForFaviconURL() method with a new
> method that does basically the same thing?
> http://www.davekb.com/browse_programming_tips:
> java_find_id_of_just_inserted_row:txt

The difference being that the function to get the last autoincrement id can usually be run without the need to actually interact with the database. It's generally substantially cheaper than doing a lookup using one of the regular indexes as you're doing with your other query.
r+ with comments, and ideally I'd like to see another patch that manages to delete most of getIDForFaviconURL, but this looks like it should do the trick.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1136,5 @@
>                           Combined.URL + " = ?",
>                           new String[] { uri },
>                           null);
>  
>              if (c.moveToFirst())

Feel free to bolt some braces onto this.

@@ +1147,5 @@
> +        // If that fails, check in the bookmarks table.
> +        try {
> +            c = cr.query(mBookmarksUriWithProfile,
> +                         new String[] { Bookmarks.FAVICON_URL },
> +                         Combined.URL + " = ?",

Bookmarks.URL

@@ +1186,5 @@
> +        // After writing the encodedFavicon, ensure that the favicon_id in both the bookmark and
> +        // history tables are also up-to-date.
> +        final Integer id = getIDForFaviconURL(cr, faviconUri);
> +        if (id == FAVICON_ID_NOT_FOUND) {
> +            return;

This should at least log an error: don't silently fail when a sanity check fails. If this condition is false we've got a bug.

@@ +1197,5 @@
> +     */
> +    private Integer getIDForFaviconURL(ContentResolver cr, String faviconURL) {
> +        final Cursor c = cr.query(mFaviconsUriWithProfile,
> +                                  new String[] { Favicons._ID },
> +                                  Favicons.URL + " = ? AND " + Favicons.DATA + " IS NOT NULL",

Since it's entirely nonsensical for favicons.data to be null (and it should be declared NOT NULL for this reason), we can probably leave out that check.

Comment 22

[Richard Newman [:rnewman]]

Comment on attachment 8473414 [details] [diff] [review]
bug1044940v1.diff

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

Quick drive-by.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1197,5 @@
> +     */
> +    private Integer getIDForFaviconURL(ContentResolver cr, String faviconURL) {
> +        final Cursor c = cr.query(mFaviconsUriWithProfile,
> +                                  new String[] { Favicons._ID },
> +                                  Favicons.URL + " = ? AND " + Favicons.DATA + " IS NOT NULL",

That it's not declared NOT NULL is why this needs the check, no?

Let's not remove the protective goggles while the blade is still spinning.

@@ +1206,5 @@
> +            final int col = c.getColumnIndexOrThrow(Favicons._ID);
> +            if (c.moveToFirst() && !c.isNull(col)) {
> +                return c.getInt(col);
> +            }
> +            // ID's can be negative, so we return a sentinel value indicating "not found".

IDs, newline before comment.

@@ +1210,5 @@
> +            // ID's can be negative, so we return a sentinel value indicating "not found".
> +            return FAVICON_ID_NOT_FOUND;
> +
> +        } finally {
> +            if (c != null) {

If you're going to null check, do it after calling cr.query, not after you failed to dereference `c` on line 1206!

Personally I wouldn't null check at all.

Comment 23

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #22)
> ::: mobile/android/base/db/LocalBrowserDB.java
> @@ +1197,5 @@
> > +     */
> > +    private Integer getIDForFaviconURL(ContentResolver cr, String faviconURL) {
> > +        final Cursor c = cr.query(mFaviconsUriWithProfile,
> > +                                  new String[] { Favicons._ID },
> > +                                  Favicons.URL + " = ? AND " + Favicons.DATA + " IS NOT NULL",
> 
> That it's not declared NOT NULL is why this needs the check, no?

Ah, I thought I'd already got my patch for that landed. Apparently it's still sitting blithely in my queue :/

> Let's not remove the protective goggles while the blade is still spinning.

But it's so much fun!

> @@ +1210,5 @@
> > +            // ID's can be negative, so we return a sentinel value indicating "not found".
> > +            return FAVICON_ID_NOT_FOUND;
> > +
> > +        } finally {
> > +            if (c != null) {
> 
> If you're going to null check, do it after calling cr.query, not after you
> failed to dereference `c` on line 1206!

Good spot.
To clarify, the common pattern of null-checking inside finally like this is useful when the thing we're checking is assigned inside the try. In that case, where we might throw before we assign the thing, we need to null-check before we close it in finally, because it might still be null.
Since you're doing the assignment before the try, this check isn't helping.

> Personally I wouldn't null check at all.

That's not like you :P

Comment 24

[Richard Newman [:rnewman]]

> > You want to replace the current getIDForFaviconURL() method with a new

> The difference being that the function to get the last autoincrement id...

Note that:

* It's only meaningful to use the last inserted ID if you just did an insertion. You could replace some _calls_ to getIDForFaviconURL, but you couldn't replace the definition of that method, because it's not always called immediately after the relevant insertion.

* It's only safe to use sqlite3_last_insert_rowid if you can guarantee that nobody else has inserted into this table on this connection since your insert. That's why we typically *return the value from insert*, rather than querying for it explicitly. You'd need to guard this with synchronization.

* last_insert_rowid only applies to INSERT. The code in question is an UPDATE, so this is all moot.

Comment 25

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #23)

> To clarify, the common pattern of null-checking inside finally like this is
> useful when the thing we're checking is assigned inside the try.

Yeah, and I always encourage people not to do that. This is lazy:

  Foo x;
  try {
    x = bar();
  } finally {
    if (x != null) {
      x.close();
    }
  }

because either bar() threw, and you don't need to close, or it didn't, and you always need to. You can eliminate the decision by moving one line of code.

Comment 26

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #25)
> (In reply to Chris Kitching [:ckitching] from comment #23)
> 
> > To clarify, the common pattern of null-checking inside finally like this is
> > useful when the thing we're checking is assigned inside the try.
> 
> Yeah, and I always encourage people not to do that. This is lazy:
> 
>   Foo x;
>   try {
>     x = bar();
>   } finally {
>     if (x != null) {
>       x.close();
>     }
>   }
> 
> because either bar() threw, and you don't need to close, or it didn't, and
> you always need to. You can eliminate the decision by moving one line of
> code.

I agree, but the situation gets more awkward when you have multiple resources you want to use together and close in the same finally. That's pretty unusual, though. I miss try-with-resources.

(In reply to Richard Newman [:rnewman] from comment #24)
> * It's only meaningful to use the last inserted ID if you just did an
> insertion. You could replace some _calls_ to getIDForFaviconURL, but you
> couldn't replace the definition of that method, because it's not always
> called immediately after the relevant insertion.
> 
> * It's only safe to use sqlite3_last_insert_rowid if you can guarantee that
> nobody else has inserted into this table on this connection since your
> insert. That's why we typically *return the value from insert*, rather than
> querying for it explicitly. You'd need to guard this with synchronization.
> 
> * last_insert_rowid only applies to INSERT. The code in question is an
> UPDATE, so this is all moot.

Ah.
I had wondered why we never seemed to use this.

Comment 27

[Mark Capella [:capella]]

Heh, *5* updates while I was typing my single reply ... it's hard to get a word in, when ya'll are on a tear :-D 

I was going to say:

---- A)
I know you mentioned |Bookmarks.URL| before, but that doesn't work. (I've tried it) I'm not sure why you think it does?

---- B)
Regarding, removing getIDForFaviconURL(), my java seems to be letting me down. Wouldn't we have to add a method to BrowserProvider.java like the below, called via |int foo = cr.getLastInsertRowId(mFaviconsUriWithProfile)| or somesuch?

I tried that back a week or so ago and ran into plumbing problems. But again, it was back a bit and I may have been in a hurry and got tired of looking at it.

   public Cursor getLastInsertRowId(Uri uri) {
       SQLiteDatabase db = getReadableDatabase(uri);
       return db.rawQuery("SELECT last_insert_rowid()", null);
   }


But it likes like we've mooted B) and are ok with the current method if I've understood through the fast read.

Comment 28

[Chris Kitching [:ckitching]]

(In reply to Mark Capella [:capella] from comment #27)
> Heh, *5* updates while I was typing my single reply ... it's hard to get a
> word in, when ya'll are on a tear :-D 
> 
> I was going to say:
> 
> ---- A)
> I know you mentioned |Bookmarks.URL| before, but that doesn't work. (I've
> tried it) I'm not sure why you think it does?

Defining class (inherited from URLColumns)
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserContract.java#142

Example usage:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserContract.java#393

You need to add an import. Consider using a different editor if this is nontrivial.

Comment 29

[Mark Capella [:capella]]

Well the issue in this bit, isn't a failure to compile. It just doesn't work. The two fields are not the same.

Comment 30

[Chris Kitching [:ckitching]]

(In reply to Mark Capella [:capella] from comment #29)
> Well the issue in this bit, isn't a failure to compile. It just doesn't
> work. The two fields are not the same.

... Yes. Yes they are.

Comment 31

[Mark Capella [:capella]]

mmm .. then I'll have to ask you to post a patch for me to see what I'm missing.

Comment 32

[Chris Kitching [:ckitching]]

The constant is defined here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserContract.java#86

And inherited by both classes:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserContract.java#213
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserContract.java#142

Comment 33

[Mark Capella [:capella]]

Attachment: bug1044940v1.diff

I'll ask for a final r? to be sure I finally present it as you'd like, and that I caught all the final nits in the quick back and forth.

Comment 34

[Chris Kitching [:ckitching]]

Comment on attachment 8485353 [details] [diff] [review]
bug1044940v1.diff

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

Basically fine.

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1162,5 @@
> +        } finally {
> +            if (c != null) {
> +                c.close();
> +            }
> +        }

Address Richard's complaint from Comment 25 about this pattern.

... I hadn't realised how annoying this was. Wow.

So what you want is something of the form:


Cursor c = ...
try {
   // Stuff that uses c
} finally {
    c.close();
}

No null check required!

Comment 35

[Mark Capella [:capella]]

mmm ... I did address the item as pointed out below here in comment #22 in getIDForFaviconURL() (iirc)

The (two) patterns you point to here getFaviconURLFromPageURL() still require that check don't they?

Forgive me if I'm confused, I get that way :)

Comment 36

[Chris Kitching [:ckitching]]

Nah, you can still evade the check:

// Check first in the history table.
Cursor c = cr.query(mHistoryUriWithProfile,
                          new String[] { History.FAVICON_URL },
                          Combined.URL + " = ?",
                          new String[] { uri },
                          null);
         try { 
            if (c.moveToFirst()) {
                 return c.getString(c.getColumnIndexOrThrow(History.FAVICON_URL));
            }
         } finally {
             c.close();
         }
 
        // If that fails, check in the bookmarks table.
        c = cr.query(mBookmarksUriWithProfile,
                         new String[] { Bookmarks.FAVICON_URL },
                         Bookmarks.URL + " = ?",
                         new String[] { uri },
                         null);
        try {
            if (c.moveToFirst()) {
                return c.getString(c.getColumnIndexOrThrow(Bookmarks.FAVICON_URL));
            }
        } finally {
            c.close();
        }


... Get it?

Comment 37

[Mark Capella [:capella]]

Attachment: bug1044940v1.diff

Got it! Copy/pasta and some quick tests looks good to me. Haven't run it by try-server yet, but I always do that before heading off to fx-team.

Comment 38

[Mark Capella [:capella]]

fyi - TRY push https://tbpl.mozilla.org/?tree=Try&rev=5253960fe18c

Comment 39

[Mark Capella [:capella]]

Attachment: bug1044940v1.diff

This should be the same as what I pushed to TRY

Comment 40

[Chris Kitching [:ckitching]]

Comment on attachment 8486610 [details] [diff] [review]
bug1044940v1.diff

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +1099,5 @@
> +
> +        // After writing the encodedFavicon, ensure that the favicon_id in both the bookmark and
> +        // history tables are also up-to-date.
> +        final Integer id = getIDForFaviconURL(cr, faviconUri);
> +        if (id == FAVICON_ID_NOT_FOUND) {

Just spotted a particularly interesting subtle bug here.

`id` is a boxed Integer, FAVICON_ID_NOT_FOUND is an int. 

In general, comparing one to the other with == isn't a good idea, because this code gets desugared to:

final Integer id = getIDForFaviconURL(cr, faviconUri);
if (id == Integer.valueOf(FAVICON_ID_NOT_FOUND)) {
...
}

Notice how we're now doing an Object reference comparison, not an integer comparison. Unless both sides are the *same object* (instead of equivalent objects) this will return false (even if they represent the same number...).

Interestingly, your code might work anyway. This is because when you return FAVICON_ID_NOT_FOUND from getIDForFaviconURL it, too, is subjected to autoboxing and wrapped in a call to Integer.valueOf(). So what we're checking for is `Integer.valueOf(FAVICON_ID_NOT_FOUND) == Integer.valueOf(FAVICON_ID_NOT_FOUND)`.

Integer.valueOf is ridiculous. It provides the "boxing conversion" defined in section 5.1.7 of the Java Language Specification ( http://docs.oracle.com/javase/specs/jls/se7/html/jls-5.html#jls-5.1.7 ).
From the spec, we find this truly hilarious quote:

"If the value p being boxed is true, false, a byte, or a char in the range \u0000 to \u007f, or an int or short number between -128 and 127 (inclusive), then let r1 and r2 be the results of any two boxing conversions of p. It is always the case that r1 == r2.

Ideally, boxing a given primitive value p, would always yield an identical reference. In practice, this may not be feasible using existing implementation techniques. The rules above are a pragmatic compromise. The final clause above requires that certain common values always be boxed into indistinguishable objects. The implementation may cache these, lazily or eagerly. For other values, this formulation disallows any assumptions about the identity of the boxed values on the programmer's part. This would allow (but not require) sharing of some or all of these references.

This ensures that in most common cases, the behavior will be the desired one, without imposing an undue performance penalty, especially on small devices. Less memory-limited implementations might, for example, cache all char and short values, as well as int and long values in the range of -32K to +32K."

That is to say, `Integer.valueOf(x) == Integer.valueOf(x)` provided x is between -128 and 127 (inclusive), but the rest of the time the result is undefined. So, the result of your comparison is undefined (since your flag is outside that range).


Things get even more fun when we look at how Integer.valueOf actually implements this. There's a private inner class of Integer which holds a cache of Integer objects for values in that range. valueOf simply checks the cache and returns the existing value (if any), before constructing a new one and storing it if it needs to.
But... the Reflection API exists. This means we can modify the Integer cache.
This means that, for any class operating in the same ClassLoader as we are, we can redefine the value of integers in the range -128 to 127 inclusive under the boxing transformation. *cackles insanely*

Oh, Java.


In your case, the solution is simply to redefine `id` to be an int, because null is not a possible value.

This provides another interesting lesson: when a function that wants to be able to return "no value" is otherwise returning an integer, using a sentinel is much better than using `null` (provided an appropriate sentinel value exists) because it avoids the need to use autoboxing. (which is bad for performance as well as because it's COMPLETELY INSANE).


Anyhoo, no need for a new patch, I'll fix this in my copy before I land it, but I thought you'd find the discussion amusing.

Comment 41

[Chris Kitching [:ckitching]]

Slight correction to the above:

Yes, you declared FAVICON_ID_NOT_FOUND as an Integer:

private static final int FAVICON_ID_NOT_FOUND = Integer.MIN_VALUE;

But *this* desugars to:

private static final int FAVICON_ID_NOT_FOUND = Integer.valueOf(Integer.MIN_VALUE);

and from there everything behaves equivalently to how I explained it above (sorry for any confusion...)

Comment 42

[Chris Kitching [:ckitching]]

Actually, a final comment because I appear to be failing pretty hard at making sense:

* It's also necessary to retype FAVICON_ID_NOT_FOUND as an int so we use integer comparison.
* It's now possible (and encouraged) to retype getIDForFaviconURL as an int.

In general, avoid using boxed types wherever possible. Sometimes they are necessary, but a good approach is to try and unbox as soon as possible.
This is because boxed types:

* Have the completely pants-on-head crazy comparison semantics I discuss above.
* Generate appallingly inefficient bytecode.


Anyway, thanks for the patch, sorry I took so long to review it (and made very little sense while doing so).

Comment 43

[Mark Capella [:capella]]

It seems that since I posted the patch in comment #37, then re-based, pushed to try and reposted as requested in comment #38 to comment #39, that the patch needs another rebase.

I'd then retest locally, push to try again, and repost for your review for my own comfort level.

I can do this if you see this code being stable for the next day or so, but as there is work you'd like to touch-up as outlined in comment #40 to comment #42, it might make sense here for you to just incorporate the patch and move it for us both (?)

Comment 44

[Chris Kitching [:ckitching]]

Don't worry about rebasing, I'll handle it.

Also, I'm the reason it needs rebasing. :P

Comment 45

[Mark Capella [:capella]]

(hehe) I got as far as re-basing locally, tested locally (looks fine), and pushed to try this:
https://tbpl.mozilla.org/?tree=Try&rev=2ddf34e605da

I'll let you take things from here and complete it for us, as best as you see fit.

Comment 46

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/54ca5ea4de1d

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54ca5ea4de1d