Closed Bug 1044940 Opened 5 years ago Closed 5 years ago

Favicons in the bookmarks table are neither read correctly nor written

Categories

(Firefox for Android :: Favicon Handling, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: ckitching, Assigned: capella, Mentored)

References

Details

Attachments

(1 file, 9 obsolete files)

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).
See Also: → 1044794
Attached patch bug1044940.diff (obsolete) — Splinter Review
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
Attachment #8464204 - Flags: feedback?(chriskitching)
Forgot to mention, this depends on bug 1016686
See Also: → 1045843
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.
Attachment #8464204 - Attachment is obsolete: true
Attachment #8464204 - Flags: feedback?(chriskitching)
Attached patch bug1044940.diff (obsolete) — Splinter Review
maybe this is closer ?
Attachment #8471640 - Flags: feedback?(chriskitching)
Attached patch bug1044940.diff (obsolete) — Splinter Review
meh - forgot a qref ...

This seems to fix kat's bug 1051544 also.
Attachment #8471640 - Attachment is obsolete: true
Attachment #8471640 - Flags: feedback?(chriskitching)
Attachment #8471715 - Flags: feedback?(chriskitching)
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.
Assignee: chriskitching → markcapella
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.
Attachment #8471715 - Flags: feedback?(chriskitching)
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 :)
(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)
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.
Attached patch bug1044940.diff (obsolete) — Splinter Review
Core / unfettered version of the patch that you're asking for. Let me know if I missed anything you asked for.
Attachment #8471715 - Attachment is obsolete: true
Attachment #8472213 - Flags: feedback?(chriskitching)
(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!
Mentor: chriskitching
Hardware: ARM → All
Summary: Favicons in the bookmarks table are neither read correctly nor written. → Favicons in the bookmarks table are neither read correctly nor written
Attached patch bug1044940v1.diff (obsolete) — Splinter Review
Re-submitting the patch using Integer.MIN_VALUE as the "not found" sentinel, and changing feedback request to review request.
Attachment #8472213 - Attachment is obsolete: true
Attachment #8472213 - Flags: feedback?(chriskitching)
Attachment #8472806 - Flags: review?(chriskitching)
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.
Attachment #8472806 - Flags: review?(chriskitching) → feedback+
Attached patch bug1044940v1.diff (obsolete) — Splinter Review
Attachment #8473392 - Flags: review?(chriskitching)
Attachment #8472806 - Attachment is obsolete: true
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).
Attachment #8473392 - Flags: review?(chriskitching) → feedback+
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
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.
Attached patch bug1044940v1.diff (obsolete) — Splinter Review
The current patch, with comment #18 not yet resolved.
Attachment #8473392 - Attachment is obsolete: true
Attachment #8473414 - Flags: review?(chriskitching)
review ping?
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.
Attachment #8473414 - Flags: review?(chriskitching) → review+
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.
(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
> > 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.
(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.
(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.
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.
(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.
Well the issue in this bit, isn't a failure to compile. It just doesn't work. The two fields are not the same.
(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.
mmm .. then I'll have to ask you to post a patch for me to see what I'm missing.
Attached patch bug1044940v1.diff (obsolete) — Splinter Review
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.
Attachment #8473414 - Attachment is obsolete: true
Attachment #8485353 - Flags: review?(chriskitching)
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!
Attachment #8485353 - Flags: review?(chriskitching) → feedback+
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 :)
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?
Attached patch bug1044940v1.diff (obsolete) — Splinter Review
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.
Attachment #8485353 - Attachment is obsolete: true
Attachment #8486003 - Flags: review?(chriskitching)
This should be the same as what I pushed to TRY
Attachment #8486003 - Attachment is obsolete: true
Attachment #8486003 - Flags: review?(chriskitching)
Attachment #8486610 - Flags: review?(chriskitching)
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.
Attachment #8486610 - Flags: review?(chriskitching) → review+
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...)
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).
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 (?)
Don't worry about rebasing, I'll handle it.

Also, I'm the reason it needs rebasing. :P
(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.
https://hg.mozilla.org/mozilla-central/rev/54ca5ea4de1d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.