Closed Bug 1188425 Opened 5 years ago Closed 5 years ago
Add a pref to allow insecure text input, thereby avoiding crashes at [Child
View key Down:] in non-release builds
Since bug 807893 landed, we've had code in Cocoa Widgets that crashes whenever it detects that the user is entering a password insecurely -- when "A password editor has focus, but not in secure input mode". This is to allow us to find and fix the bugs that allow this to happen. We've fixed some of them: bug 1051842 bug 1124408 bug 1138678 But others remain unfixed, including the following and (probably) others that haven't yet been opened: bug 1148196 Though the bugs we're trying to fix also happen in release builds, we don't crash there -- that would be too disruptive and annoying for users who aren't also testers. But these crashes can also be disruptive/annoying for those who regularly trigger them -- for example those who use 3rd party password managers. So we should add an about:config pref to allow people to disable the tests for insecure input in non-release builds, if they so choose.
Here's another of these bugs that's still open: bug 893973
Summary: Add a pref to allow disabling tests for insecure input, which can crash at [ChildView keyDown:] in non-release builds → Add a pref to allow insecure input, thereby avoiding crashes at [ChildView keyDown:] in non-release builds
Summary: Add a pref to allow insecure input, thereby avoiding crashes at [ChildView keyDown:] in non-release builds → Add a pref to allow insecure text input, thereby avoiding crashes at [ChildView keyDown:] in non-release builds
Crash Signature: [@ -[ChildView keyDown:]]
Here's yet another open bug, which concerns crashes with the 1password extension: bug 1163339
Jamie, it's at bug 1163339 that I'll be working on the problem(s) specific to 1Password -- why these crashes happen with it. You should probably repeat your comment there. That'll be the best place to continue our discussion.
Comment on attachment 8640140 [details] [diff] [review] Fix Review of attachment 8640140 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpref/init/all.js @@ +4858,5 @@ > +#if !defined(RELEASE_BUILD) || defined(DEBUG) > +// In non-release builds we crash by default on insecure text input (when a > +// password editor has focus but secure event input isn't enabled). The > +// following pref, when turned on, disables this behavior. See bug 1188425. > +pref("intl.allow-insecure-text-input", false); This seems like a temporary pref for developers working with and/or building non-release Firefox builds. Again, I'm not sure this warrants a permanently visible pref in about:config. I'll let you make the call, but I'd be perfectly fine if you removed it from this file.
Attachment #8640140 - Flags: review?(spohl.mozilla.bugs) → review+
I think we want this patch's functionality to be a little more visible than the one for bug 1186158. Also note that the pref is only permanently visible (in about:config) in non-release builds.
This bug's patch is now in today's mozilla-central nightly. I just tested with the STR from bug 1148196 comment #9, with intl.allow-insecure-text-input set to 'true' and 'false'. With it set to 'false' the crashes still happen. With it set to 'true' they don't. I think this counts as verification.
Status: RESOLVED → VERIFIED
Comment on attachment 8640140 [details] [diff] [review] Fix Approval Request Comment [Feature/regressing bug #]: bug 807893, sort of [User impact if declined]: Users of password managers may crash consistently in the Dev Edition [Describe test coverage new/current, TreeHerder]: Manual testing, as per comment #10 [Risks and why]: Very low [String/UUID change made/needed]: None
Attachment #8640140 - Flags: approval-mozilla-aurora?
Steven, is this fix only a temporary fix so we don't crash and move on to fix other bugs in this area? Do we plan to keep this bug open so we remove this temporary fix in a few weeks? Thanks!
This fix won't *prevent* these crashes. It will just allow those few who consistently see lots of them to turn off the security check that causes them to happen (in non-release builds). It's meant as a palliative until we can actually fix all cases of the underlying security bug. It's been a long time since we started working on this issue, and frankly it's taken us too long to resolve it. I *hope* it won't take too much longer, but I really don't know. Once the underlying security bug has been fixed, this patch can come out -- in that sense it *is* temporary. But I don't know how long "temporary" will be :-)
Comment on attachment 8640140 [details] [diff] [review] Fix Approved. Seems safe as this is only limited to non-release builds.
Attachment #8640140 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora uplift.
Comment on attachment 8640140 [details] [diff] [review] Fix Landed (after rebasing) on mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/5c14f816e9da
You need to log in before you can comment on or make changes to this bug.