Closed
Bug 463445
Opened 16 years ago
Closed 16 years ago
nsPrivateBrowsingService.removeDataFromDomain might throw if the user cancels the master password prompt
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 3.1b2
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(2 files)
(deleted),
patch
|
mconnor
:
review+
mconnor
:
approval1.9.1b2+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | 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)
Updated•16 years ago
|
Attachment #346683 -
Flags: review?(mconnor) → review+
Comment 2•16 years ago
|
||
(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.
Assignee | ||
Comment 4•16 years ago
|
||
(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).
Assignee | ||
Comment 5•16 years ago
|
||
(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.
Comment 6•16 years ago
|
||
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 7•16 years ago
|
||
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+
Assignee | ||
Comment 8•16 years ago
|
||
Aaron: are you willing to consider writing a test for this?
Flags: in-testsuite?
Assignee | ||
Comment 9•16 years ago
|
||
Assignee | ||
Comment 10•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b520f255b3a9
Status: ASSIGNED → RESOLVED
Closed: 16 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.
Description
•