843029 - Add more retries to ensureDatabaseIsNotLocked

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

If things go badly and the DB remains locked, the user is in trouble. Let's try a little harder to make sure the zombies have cleared up and any open DB connections have been closed.

Comment 1

[Lucas Rocha (:lucasr)]

Comment on attachment 716016 [details] [diff] [review]
patch

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

Looks good to me otherwise.

::: mobile/android/base/db/DBUtils.java
@@ +67,5 @@
> +        for (int retries = 0; retries < 5; retries++) {
> +            try {
> +                // Try a simple test and exit the loop
> +                dbHelper.getWritableDatabase();
> +                break;

I guess you meant 'return' here? Otherwise the log call you added below will always happen.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #1)

> > +                dbHelper.getWritableDatabase();
> > +                break;
> 
> I guess you meant 'return' here? Otherwise the log call you added below will
> always happen.

Good catch. That's what I get for adding the Log and not looking at the rest of the patch. Fixed locally.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 716016 [details] [diff] [review]
patch

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

as Lucas already pointed out the only issue I see. I also wonder what other useful information we can log on failure, but that can be a follow up.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 716016 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): not a lot of risk. if this code is hit, we are in trouble already. the patch tries a little harder to bail us out.
String or UUID changes made by this patch: none

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 716016 [details] [diff] [review]
patch

Due to time constraints on getting FF20b1 for mobile builds kicked off, approving for uplift though it will still need to land on trunk/aurora.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b79b4213e3b2

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d609a1cf7be
https://hg.mozilla.org/releases/mozilla-beta/rev/cf2437d4159e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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