812347 - Improve robustness of favicon_urls.db handling in 12->13 upgrade

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Attachment: Don't fail upgrade if favicon_urls.db cannot be read

Wes reported that his Nightly is crashing because the 12->13 upgrade is failing. Apparently it's failing because of a missing favicon_urls.db file (though I don't exactly know what would cause that to happen). Since importing the favicon URLs isn't a critical part of the upgrade, we should be able to ignore any exceptions thrown.

I also moved the deletion of favicon_urls.db to the end of the upgrade steps in case a later upgrade were to fail (as discussed in IRC).

Comment 1

[Lucas Rocha (:lucasr)]

Comment on attachment 682202 [details] [diff] [review]
Don't fail upgrade if favicon_urls.db cannot be read

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

Giving r- just to get an answer to why deletion is done inside the transaction and why we throw if deletion fails.

::: mobile/android/base/db/BrowserProvider.java.in
@@ +1530,5 @@
> +            if (oldVersion < 13 && newVersion >= 13
> +                                && mContext.getDatabasePath(Obsolete.FAVICON_DB).exists()
> +                                && !mContext.deleteDatabase(Obsolete.FAVICON_DB)) {
> +                throw new SQLException("Could not delete " + Obsolete.FAVICON_DB);
> +            }

If we don't care about failing to import favicon urls, why would we care that much if the file got deleted or not? So, we should probably just ignore if the deletion fails and move this code *after* the endTransaction() call, no?

Comment 2

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #1)
> Comment on attachment 682202 [details] [diff] [review]
> Don't fail upgrade if favicon_urls.db cannot be read
> 
> Review of attachment 682202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Giving r- just to get an answer to why deletion is done inside the
> transaction and why we throw if deletion fails.
> 
> ::: mobile/android/base/db/BrowserProvider.java.in
> @@ +1530,5 @@
> > +            if (oldVersion < 13 && newVersion >= 13
> > +                                && mContext.getDatabasePath(Obsolete.FAVICON_DB).exists()
> > +                                && !mContext.deleteDatabase(Obsolete.FAVICON_DB)) {
> > +                throw new SQLException("Could not delete " + Obsolete.FAVICON_DB);
> > +            }
> 
> If we don't care about failing to import favicon urls, why would we care
> that much if the file got deleted or not? So, we should probably just ignore
> if the deletion fails and move this code *after* the endTransaction() call,
> no?

If we don't delete it here, it will stick around forever. This checks for the existence of the database file first, so it only throws if it knows it exists *and* it fails to delete it. I don't think we'll see this happen much, but if we do, we'd probably want to investigate why deleting the database fails rather than ignoring and leaving it on the user's phone.

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 682202 [details] [diff] [review]
Don't fail upgrade if favicon_urls.db cannot be read

Does this make sense, or is there something else we should try?

Comment 4

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0490d36be8f0

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0490d36be8f0