Closed Bug 1148196 Opened 5 years ago Closed 4 years ago

[e10s] Crash in [ChildView keyDown:] when using master password popup in e10s mode

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
e10s + ---
firefox41 --- wontfix
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: handyman, Assigned: smichaud)

References

Details

Crash Data

Attachments

(1 file)

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
NI the world
Flags: needinfo?(twalker)
Flags: needinfo?(lshapiro)
Flags: needinfo?(julian.viereck)
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)
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)
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)
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)
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
Duplicate of this bug: 1185126
(In reply to comment #6)

The crashes with 1Password are a different bug -- bug 1163339.
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
Attached patch FixSplinter Review
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)
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+
https://hg.mozilla.org/mozilla-central/rev/131e9ddc58bd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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
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)
> 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-
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.
Comment on attachment 8655128 [details] [diff] [review]
Fix

OK, let's take it for 42.
Attachment #8655128 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.