580839 - Satchel can throw the wrong exception on failures

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Justin Dolske [:Dolske]]

From bug 552828 comment 12:

> > >         } catch (e) {
> > >             this.log("getExistingEntryID failed: " + e);
> > >             throw e;
> > >         } finally {
> > >-            stmt.reset();
> > >+            if (stmt)
> > >+                stmt.reset();
> > >         }
> > 
> > What's this change for?
> 
> dbCreateStatement managed to return null or throw once for me and I saw an
> error message, so I added the check.

Indeed, if an exception occurs in the |finally|, callers see that instead of the intended |throw e| from the |catch|. We should fix all the cases in satchel where this pattern exists (and in pwmgr, if it's there?) to test |stmt| before trying to reset it.

This won't be a functional change, because there shouldn't be any callers that depend on specific exceptions (afaik), but it would be helpful for debugging.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

I'll take this bug

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 580839

Fix for bug 580839. Switched the finally blocks to check for a truthy |stmt| before invoking a member function on it.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #0)
> Indeed, if an exception occurs in the |finally|, callers see that instead of
> the intended |throw e| from the |catch|. We should fix all the cases in
> satchel where this pattern exists (and in pwmgr, if it's there?) to test
> |stmt| before trying to reset it.

Filed bug 665217 for pwmgr.

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 539881 [details] [diff] [review]
Patch for bug 580839

Looks good! woo!

Comment 5

[Justin Dolske [:Dolske]]

Push http://hg.mozilla.org/mozilla-central/rev/1d962ae5a4b6

Comment 6

[Mihaela Velimiroviciu (:mihaelav)]

Can anyone please provide some guidelines to verify this fix?

Thank you!

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Mihaela Velimiroviciu from comment #6)
> Can anyone please provide some guidelines to verify this fix?
> 
> Thank you!

Not really needed. These errors are very hard to hit and have no user facing result.