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

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: handyman, Assigned: handyman)

Tracking

Trunk
mozilla39
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(e10sm5+, firefox39 fixed)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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!

Updated

4 years ago
tracking-e10s: ? → m5+

Updated

4 years ago
Crash Signature: [@ -[ChildView keyDown:]]
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1140517
(Assignee)

Comment 2

4 years ago
Making it clearer that this isn't password manager only (it happens e.g. with the LastPass addon).
Summary: Crash in [ChildView keyDown:] when dismissing password manager → Crash in [ChildView keyDown:] when dismissing password field in chrome content
(Assignee)

Comment 3

4 years ago
Created attachment 8576354 [details] [diff] [review]
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.
Attachment #8576354 - Flags: review?(masayuki)
(Assignee)

Comment 4

4 years ago
Created attachment 8576355 [details] [diff] [review]
Ignore NOTIFY_IME_OF_BLUR when establishing IME focus

*sigh*

(At the last minute, I noticed some screwed up indentation.)
Attachment #8576354 - Attachment is obsolete: true
Attachment #8576354 - Flags: review?(masayuki)
Attachment #8576355 - Flags: review?(masayuki)
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.
(Assignee)

Comment 6

4 years ago
Created attachment 8579759 [details] [diff] [review]
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
Attachment #8576355 - Attachment is obsolete: true
Attachment #8576355 - Flags: review?(masayuki)
Attachment #8579759 - Flags: review?(masayuki)
Attachment #8579759 - Flags: review?(masayuki) → review+
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.
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e8438b5f3d46
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Comment 11

3 years ago
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?
Flags: needinfo?(davidp99)
(Assignee)

Comment 12

3 years ago
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.
Flags: needinfo?(davidp99)
Duplicate of this bug: 1185126
You need to log in before you can comment on or make changes to this bug.