1138678 - Crash in [ChildView keyDown:] when dismissing password field in chrome content

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[David Parks [:handyman]]

This bug is a spinoff of bug 1124408 (also a spinoff).  This bug has the same crash signature but different STR and is not fixed by the patch in that bug.  This bug is the result of messing around with the password manager, trying to get it to crash.  Its not clear how general the issue this STR reveals is.

STR:

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

Expected result:
The letter appears in the textbox or the password field disappears (if pressed <return>)

Actual result:
CRASH!

Comment 2

[David Parks [:handyman]]

Making it clearer that this isn't password manager only (it happens e.g. with the LastPass addon).

Comment 3

[David Parks [:handyman]]

Attachment: Ignore NOTIFY_IME_OF_BLUR when establishing IME focus

We keep proper tabs on IME focus with SetInputContext (we dont really even count EnableSecureInputs anymore... its really just a toggle).  This code unset the password config in the STR.  Its the nsChildView routine that the bug hits but I'm pretty sure the nsCocoaWindow version has the same property.

Comment 4

[David Parks [:handyman]]

Attachment: Ignore NOTIFY_IME_OF_BLUR when establishing IME focus

*sigh*

(At the last minute, I noticed some screwed up indentation.)

Comment 5

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

Comment on attachment 8576355 [details] [diff] [review]
Ignore NOTIFY_IME_OF_BLUR when establishing IME focus

>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
>--- a/widget/cocoa/nsChildView.mm
>+++ b/widget/cocoa/nsChildView.mm
>@@ -1600,20 +1600,16 @@ nsChildView::NotifyIMEInternal(const IME
>       } else {
>         TextInputHandler::EnsureSecureEventInputDisabled();
>       }
> 
>       NS_ENSURE_TRUE(mTextInputHandler, NS_ERROR_NOT_AVAILABLE);
>       mTextInputHandler->OnFocusChangeInGecko(true);
>       return NS_OK;
>     case NOTIFY_IME_OF_BLUR:
>-      // When we're going to be deactive, we must disable the secure event input
>-      // mode, see the Carbon Event Manager Reference.
>-      TextInputHandler::EnsureSecureEventInputDisabled();
>-

Sorry for the delay.

I guess that this cases we fail to disable secure input mode for other application when we're deactivated.

E.g.,

1. Show keyboard viewer.
2. Set focus to our password field.
3. Activate another application's window
4. Type a character

Then, pressed key should be animated in the keyboard viewer. IIRC, without this code, we had this bug. See bug 807893 comment 7.

Comment 6

[David Parks [:handyman]]

Attachment: Ignore NOTIFY_IME_OF_BLUR when establishing IME focus

Good catch.  This fixes that use case by disabling IME when the window key status is lost.

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2cce3170321

Comment 7

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

Ah, could you check following steps before landing?

1. Open two windows and both of them have password field and focused.
2. Focus one of them.
3. Switch to the other.
4. Type a character.

So, I'd like you to check focus move from a password field on a window to the other password field on the other window.

Comment 8

[David Parks [:handyman]]

I've tried it switching between all combinations of pairs of e10s/non-e10s windows.  It works properly in all cases.  The nature of the bug was wrt the master password login in chrome and two windows won't show the master password field at the same time so I tried with combinations where one or both password fields were in page content.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8438b5f3d46

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e8438b5f3d46

Comment 11

[Julian Viereck]

Sigh, I am still getting crashes when entering my master password in the master-password popup dialog:

  https://crash-stats.mozilla.com/report/index/9d42bca1-6005-46a2-9424-c44572150326

Should this bug be reopened?

Comment 12

[David Parks [:handyman]]

No, the STR in this bug has been fixed.  I'll open a new bug and NI you to fill it in with any info you have on the crash.  The pattern has been that no one can determine conditions under which they crash so I stress it until I can find an STR... which may or may not match any particular crash.  Of course, any conditions you can think of that are relevant to the crash could focus attention, but any crash is a legitimate bug worth fixing anyway.

I'm also going to try to find QA resources to try to find a set of reliable reproduction steps.  This might give us some depth on the crash.