Closed Bug 665217 Opened 10 years ago Closed 10 years ago

Login Manager can throw the wrong exception on failures

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: zpao, Assigned: jaws)

Details

Attachments

(1 file)

+++ 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?
Assignee: nobody → jwein
Status: NEW → ASSIGNED
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?
Attachment #540554 - Flags: review?(paul)
Comment on attachment 540554 [details] [diff] [review]
Patch for bug 665217

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

Looks good
Attachment #540554 - Flags: review?(paul) → review+
(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).
Paul can you land this for me?
Keywords: checkin-needed
If somebody doesn't get to it before me, I'll land it with some patches I have ready to go.
http://hg.mozilla.org/mozilla-central/rev/5c3cca200590
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.