665217 - Login Manager can throw the wrong exception on failures

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

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

+++ This bug was initially created as a clone of Bug #580839 +++

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.

- - -

This is all over login manager (storage-mozStorage.js) too. Jared, want to tackle that too?

Comment 1

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

Attachment: Patch for bug 665217

I've replaced all the unchecked stmt.reset() lines with the extra guard around them.

There are quite a few other files that might need this similar change. Should we go ahead and just create bugs for all of those?

Comment 2

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

Try submission: http://tbpl.mozilla.org/?tree=Try&rev=5f940047dbf1

Comment 3

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

Comment on attachment 540554 [details] [diff] [review]
Patch for bug 665217

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

Looks good

Comment 4

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

(In reply to comment #1)
> There are quite a few other files that might need this similar change.
> Should we go ahead and just create bugs for all of those?

Yea, you can get those filed in the right components if you'd like. It doesn't seem like the most pressing issue (it's been ~3 years since password manager started using sqlite without problems that we know of).

Comment 5

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

Paul can you land this for me?

Comment 6

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

If somebody doesn't get to it before me, I'll land it with some patches I have ready to go.

Comment 7

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/5c3cca200590