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

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[David Parks [:handyman]]

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

Comment 1

[David Parks [:handyman]]

NI the world

Comment 2

[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.

Comment 3

[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.

Comment 4

[David Parks [:handyman]]

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.

Comment 5

[Tracy Walker [:tracy]]

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.

Comment 6

[Robin Whittleton]

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

Comment 8

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #6)

The crashes with 1Password are a different bug -- bug 1163339.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

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).

Comment 11

[Steven Michaud [:smichaud] (Retired)]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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!!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/131e9ddc58bd

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/131e9ddc58bd

Comment 15

[Steven Michaud [:smichaud] (Retired)]

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

Comment 16

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Steven, is this an e10s only bug? e10s is disabled by default in 41 and therefore this does not meet the bar. Please confirm.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

> 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.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

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 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8655128 [details] [diff] [review]
Fix

OK, let's take it for 42.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f24900228e4d

Comment 23

[Steven Michaud [:smichaud] (Retired)]

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).