Closed Bug 812347 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 19

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(1 file)

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).
Attachment #682202 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #682202 - Flags: review?(lucasr.at.mozilla) → review-
(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 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?
Attachment #682202 - Flags: review- → review?(lucasr.at.mozilla)
Attachment #682202 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/0490d36be8f0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.