Closed
Bug 1148196
Opened 10 years ago
Closed 9 years ago
[e10s] Crash in [ChildView keyDown:] when using master password popup in e10s mode
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
VERIFIED
FIXED
mozilla43
People
(Reporter: handyman, Assigned: smichaud)
References
Details
Crash Data
Attachments
(1 file)
(deleted),
patch
|
masayuki
:
review+
Sylvestre
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
It is still possible to hit this assertion when using the password manager, as reported by Julien in bug 1138678 comment 11. The crash is forced by a check of internal IME state consistency (i.e. password vs. non-password). There have been a number of ways that the internal state would be corrupted that led to this crash. At this point, we want to try to build a catalog of ways that this can happen. I'm NIing Tracy and Larissa so that QA can consider options for finding reliable and complete conditions for the bug. Tracy/Larissa, let me know what you might need from me wrt this.
In case it helps, some past STRs (they have been fixed) from previous bugs:
-----------
bug 1051842 comment 86:
Steps:
* Open an e10s window
* Go to a page with a password text field
* Select the password text field and type characters
* Command-Tab or click away from the browser, then click on the web search bar (or any chrome text field). Do not interact with the dock or anything like that as part of this step.
* Type something
-----------
bug 1124408 comment 7:
* Activate password manager for bugzilla.mozilla.org
* Open an e10s window
* Navigate to a bug — for example: https://bugzilla.mozilla.org/show_bug.cgi?id=1124408
* Cancel the password manager popup (do not log in)
* Select the search box in the bugzilla page (not the browser’s search box). Dont type anything.
* Command-Tab away from the browser, then command-tab back
* Press Command-r to reload the page. The password manager popup will appear again.
* Type a letter.
-----------
bug 1138678 comment 0:
* Setup firefox so that the password manager requests the master password in order to log you into google.
* Open an e10s window
* Go to https://accounts.google.com/ServiceLogin?hl=en&continue=https://www.google.com/. The password manager dropdown will appear.
* Press <return> on the empty password field. It will rollback and then immediately reappear.
* Press any letter or <return>.
-----------
I'm also NIing Julien so that he can fill in any STR details that might help narrow down his particular crash.
Julien's crashstats: https://crash-stats.mozilla.com/report/index/9d42bca1-6005-46a2-9424-c44572150326
Reporter | ||
Comment 1•10 years ago
|
||
NI the world
Flags: needinfo?(twalker)
Flags: needinfo?(lshapiro)
Flags: needinfo?(julian.viereck)
Comment 2•10 years ago
|
||
Looking a little bit closer it seems I get the crash mostly when I log into the student account of our university. The following STR allow me to reproduce the crash in 100% of my tries.
Here is the exact STR:
1. Created a fresh Firefox Profile
2. Setup a master password to "abcdefg" in about:preferences#security (!!! DO NOT USE THIS AS YOUR NORMAL MASTER PASSWORD !!!)
3. Allow to execute JS in the chrome context by toggeling "devtools.chrome.enabled=true" in about:config
4. Open the Browser Console from Tools -> Web Developer -> Browser Console (at least that's the path under OSX)
5. Execute the following JS lines in the Browser Console's prompt to create a dummy user/password entry for the site we will try to log in later:
var pwmgr = Components.classes["@mozilla.org/login-manager;1"].
getService(Components.interfaces.nsILoginManager);
var newLogin = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
newLogin.init("https://aai-logon.ethz.ch", "https://aai-logon.ethz.ch", null,
"aUser", "aPassword", "j_username", "j_password");
pwmgr.addLogin(newLogin);
6. The master password prompt will ask you for the master password. Insert the password "abcdefg" correclty to store the password.
7. Close and reopen Firefox using the same profile. This is required to close the master password protected keychain again.
8. Go to the homepage: https://www.lehrbetrieb.ethz.ch/myStudies/
9. Press the "Start" button displayed on the page. You will be redirected to a new page.
10. The master password prompt will show up and ask you for the master password as it tires to use the password already stored in your Firefox keychain as stored during step 5. Enter an INCORRECT password first - e.g. "1234"
11. Press the enter key on your password to try to unlock the Firefox keychain using the incorrect master password
12. Start entering the master password a second time. After typing a few keystrokes the browser will crash.
Flags: needinfo?(julian.viereck)
Comment 3•10 years ago
|
||
Maybe this output on the OS terminal is useful as well:
[Child 91673] ###!!! ABORT: Aborting on channel error.: file /builds/slave/m-cen-m64-ntly-000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 1597
[Child 91673] ###!!! ABORT: Aborting on channel error.: file /builds/slave/m-cen-m64-ntly-000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 1597
@handyman - let me know if I can help with this any further.
Flags: needinfo?(davidp99)
Reporter | ||
Comment 4•10 years ago
|
||
Thanks Julien. That looks super helpful. This sounds similar to the bug 1138678 STR... maybe we'll get lucky and it won't be much different. I should have time to look at this next week.
Flags: needinfo?(davidp99)
Comment 5•10 years ago
|
||
This had been broken for me and was really blocking my usage of Nightly on a regular basis. However, something in the past week or two fixed it and I am no longer crashing in attempts to enter master password in the dialog prompt. I've been using Nightly as my primary browser since.
Flags: needinfo?(twalker)
Flags: needinfo?(lshapiro)
Updated•10 years ago
|
Comment 6•10 years ago
|
||
I might have just hit this using 1Password (e10s, latest Nightly, 10.10): https://crash-stats.mozilla.com/report/index/699f6c2d-a262-41f5-b8ce-598552150429
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to comment #6)
The crashes with 1Password are a different bug -- bug 1163339.
Assignee | ||
Comment 9•9 years ago
|
||
Thanks, Julian, for your STR!
I can reproduce the crashes with a simpler STR on a page where I already have an account. I only crash with e10s on.
1) Run a recent Firefox trunk/mozilla-central nightly and make sure "Enable multi-process Nightly" is selected (in Preferences : General).
2) Visit https://github.com/login and ensure your account and login are stored by the browser.
3) Under Preferences : Security, check "Use a master password" and choose one.
4) Restart the browser.
5) Visit https://github.com/login again.
6) When you are prompted, enter an incorrect master password.
7) When the master password prompt comes up again, type exactly one character.
Crash.
Assignee: davidp99 → smichaud
Summary: Crash in [ChildView keyDown:] when using master password popup → [e10s] Crash in [ChildView keyDown:] when using master password popup in e10s mode
Assignee | ||
Comment 10•9 years ago
|
||
This patch fixes bug 1148196 (this bug) and bug 1163339, and hopefully also *all* secure-input related crashes in -[ChildView keyDown:].
I simplified existing code, according to the following rules:
1) *Never* enable or disable secure input except when we actually have keyboard focus.
2) Only do this in the context of nsChildView/ChildView objects. Secure input mode can only be implemented using these objects. And in fact I doubt it makes sense for nsCocoaWindow/BaseWindow objects to have an input context at all -- though I didn't remove the code that gets and sets input contexts for those objects.
3) As the only exception to rule 2, we need to turn secure input mode off and on whenever a browser window gains or loses keyboard focus (whether or not our app also gains or loses focus).
Attachment #8655128 -
Flags: review?(masayuki)
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8655128 [details] [diff] [review]
Fix
I've started a tryserver run for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b11a75d633e3
Comment 12•9 years ago
|
||
Comment on attachment 8655128 [details] [diff] [review]
Fix
Looks okay. Let's try this approach.
r=masayuki, if tryserver won't detect any problems. Thank you for your work!!
Attachment #8655128 -
Flags: review?(masayuki) → review+
Comment 13•9 years ago
|
||
Comment 14•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee | ||
Comment 15•9 years ago
|
||
These crashes have disappeared in m-c nightly builds made since my patch landed on trunk. I think that counts as verification.
Hooray, we seem to have finally killed them! And gotten secure keyboard input working correctly.
But I think we should keep the debugging code in place (which triggered the crashes) for a while longer, just in case I've missed a few very edgy edge cases.
https://hg.mozilla.org/mozilla-central/annotate/b23b2fa33a9d/widget/cocoa/nsChildView.mm#l5502
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8655128 [details] [diff] [review]
Fix
Approval Request Comment
[Feature/regressing bug #]: Very old bug
[User impact if declined]: Nasty crashes, and secure keyboard input won't work correctly
[Describe test coverage new/current, TreeHerder]: Baked on m-c for a week
[Risks and why]: Low risk -- simple, well-designed patch
[String/UUID change made/needed]: None
This bug's crashes don't effect "release" builds (beta and up). But my patch also makes secure keyboard input work correctly, and so potentially fixes one or more security bugs.
Attachment #8655128 -
Flags: approval-mozilla-beta?
Attachment #8655128 -
Flags: approval-mozilla-aurora?
Steven, is this an e10s only bug? e10s is disabled by default in 41 and therefore this does not meet the bar. Please confirm.
Flags: needinfo?(smichaud)
Assignee | ||
Comment 18•9 years ago
|
||
> Steven, is this an e10s only bug?
This bug may be, but the general problems aren't (the ones fixed by my patch):
The crashes at -[ChildView keyDown:] don't just happen in e10s mode, and neither do the problems with secure keyboard input that they trace.
Flags: needinfo?(smichaud)
Comment on attachment 8655128 [details] [diff] [review]
Fix
Every STR that I see in this bug has e10s enabled step in it. That is not convincing enough to uplift this to Beta41. We are a few days away from 41 RC and without a release-blocking criteria, this uplift does not meet the bar. In 41, e10s is disabled by default.
Attachment #8655128 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Assignee | ||
Comment 20•9 years ago
|
||
There's also bug 1163339, which was fixed by this bug's patch and happens with or without e10s.
But the most important thing about my patch is that it fixes problems with secure keyboard input. It's only incidentally that it fixes crashes at -[ChildView keyDown:] -- since we crash there deliberately when secure keyboard input is off when it should be on (or vice versa), on non-release branches (trunk and aurora).
Maybe it's too late for 41 branch. Its implementation of secure keyboard input is still buggy ... but those bugs have existed for years.
But we really should get this patch onto the 42 branch.
Updated•9 years ago
|
status-firefox41:
--- → wontfix
status-firefox42:
--- → affected
Comment 21•9 years ago
|
||
Comment on attachment 8655128 [details] [diff] [review]
Fix
OK, let's take it for 42.
Attachment #8655128 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 23•9 years ago
|
||
For the record, these crashes have now also disappeared on the 42 branch (in builds later than firefox-2015-09-10-00-40-36-mozilla-aurora).
You need to log in
before you can comment on or make changes to this bug.
Description
•