Closed Bug 463445 Opened 12 years ago Closed 12 years ago

nsPrivateBrowsingService.removeDataFromDomain might throw if the user cancels the master password prompt

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.1b2

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(2 files)

Attached patch Patch (v1)Splinter Review
Followup from bug 460086 comment 26:

(In reply to comment bug 460086 #23)
> We really don't expect them to throw, and
> if they do, we should propagate that exception.

Well, getAllLogins() will throw if it triggers a master password prompt and the
user clicks cancels. That's probably something that should be caught and
ignored.



Dolske: is there a better way to detect this exception rather than this hacky comparison?
Attachment #346683 - Flags: review?(mconnor)
Attachment #346683 - Flags: review?(mconnor) → review+
We should probably add a test for this too
(In reply to comment #0)

> Dolske: is there a better way to detect this exception rather than this hacky
> comparison?

Not really... You could just unconditionally catch and ignore exceptions from it, doesn't seem like a big deal either way.

It might be syntactically cleaner to use |catch (e if e == "blah")| instead of having to rethrow the exception, though.
I wish password manager threw error codes like everything else :(
(In reply to comment #1)
> We should probably add a test for this too

Justin, can we force this exception in a unit test?

If not, we'd have to resort to catching the master password dialog and pressing cancel from the test (ugh).
(In reply to comment #2)
> Not really... You could just unconditionally catch and ignore exceptions from
> it, doesn't seem like a big deal either way.

Hmm, Shawn mentioned that the plan is to let removeDataFromDomain pass along any possible exception.  If that's not an issue, I can do this, Shawn.

> It might be syntactically cleaner to use |catch (e if e == "blah")| instead of
> having to rethrow the exception, though.

Oh, didn't know that syntax existed.  Will do based on Shawn's respone.
We should throw other exceptions.

I imagine the master password is prompted with something like nsIPrompt?  If so, you can create your own implementation of it, register it, and have it manage the canceling all in xpcshell.  An example of this is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_bug_412360.xul#69
Comment on attachment 346683 [details] [diff] [review]
Patch (v1)

we should get this in, since this would break the clear action in some cases.
Attachment #346683 - Flags: approval1.9.1b2+
Aaron: are you willing to consider writing a test for this?
Flags: in-testsuite?
Attached patch For checkinSplinter Review
http://hg.mozilla.org/mozilla-central/rev/b520f255b3a9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
You need to log in before you can comment on or make changes to this bug.