807893 - Get rid of nsIWidget::BeginSecureKeyboardInput() and nsIWidget::EndSecureKeyboardInput()

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

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

Bug 394107 added the methods 5 years ago. However, other platforms don't use them. And now, nsIWidget::OnIMEFocusChange() is always called when our editable editor gets or loses focus. So, cocoa widget can switch the secure event input mode in it instead of nsIWidget::BeginSecureKeyboardInput() and nsIWidget::EndSecureKeyboardInput().

Comment 1

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

Attachment: Patch

I'm thinking about adding automated tests for this if it's possible.

Comment 2

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

Attachment: Patch

oops...

Comment 3

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

roc:

Can we dispatch custom DOM event from widget? If so, the test would be simple. We can fire custom DOM events on the DOM window when the secure input mode is changed. If it's impossible, should I made a new widget event which would cause firing DOM custom event? Or, do you have better idea?

Comment 4

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

Ah, MOZ_ASSERT() in the patch could be useful for the test.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> Can we dispatch custom DOM event from widget? If so, the test would be
> simple. We can fire custom DOM events on the DOM window when the secure
> input mode is changed.

I don't think we should fire custom DOM events from widget.

Comment 6

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

Attachment: part.1 Remove nsIWidget::BeginSecureKeyboardInput() and nsIWidget::EndSecureKeyboardInput()

These methods are only used on Mac. And current input context has the information if the focused element is password field or not. Therefore, these methods are not necessary.

Comment 7

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

Attachment: part.2 Cocoa widget should manage secure input mode with input context at changing the context and changing focus

This patch manages the secure input mode at setting input context and changing focus.

On current trunk build, we don't exit from secure input mode if a password field has focus and we're deactivated. Then, the secure input mode is enabled on all other applications. This patch fixes this bug too.

Comment 8

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

Attachment: part.3 Add tests for secure input mode of Cocoa widget

Adding tests for this. However, unfortunately, the bug mentioned in the previous comment cannot test with our test framework, though.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 752758 [details] [diff] [review]
part.2 Cocoa widget should manage secure input mode with input context at changing the context and changing focus

This basically looks fine to me.  But I'm puzzled that you have to implement NotifyIME() and SetInputContext() in nsCocoaWindow (in addition to nsChildView).  As I understand it, text input (of any kind) can only take place when an nsChildView is focused, so why pay attention to calls that focus or unfocus an nsCocoaWindow object?

Also, some of the comments need to be clarified:

+  /**
+   * EnableSecureEventInput() and DisableSecureEventInput() warp the same
+   * name APIs.  They manage count of calls in our process.
+   * API's IsSecureEventInputEnabled() returns if the mode is enabled at
+   * that time, whereas this class's IsSecureEventInputEnabled() returns
+   * if we've made secure event input mode enabled. 
+   */

This should be something like:

  EnableSecureEventInput() and DisableSecureEventInput() wrap the Carbon
  Event Manager APIs with the same names.  In addition they keep track of
  how many times we've called them (in the same process) -- unlike the
  Carbon Event Manager APIs, which only keep track of how many times they've
  been called from any and all processes.

  The Carbon Event Manager's IsSecureEventInputEnabled() returns whether
  secure event input mode is enabled (in any process).  This class's
  IsSecureEventInputEnabled() returns whether we've made any calls to
  EnableSecureEventInput() that are not (yet) offset by the calls we've
  made to DisableSecureEventInput().

+  /**
+   * EnsrueSecureEventInputDisabled() calls DisableSecureEventInput() until
+   * our call count becomes 0.
+   */

Ensrue... -> Ensure

+  NS_ASSERTION(!!sSecureEventInputCount == !!::IsSecureEventInputEnabled(),
+    "Something other process(es) has made secure event input enabled");

The comment should be something like:

  "Some other process has enabled secure event input"

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 752760 [details] [diff] [review]
part.3 Add tests for secure input mode of Cocoa widget

This looks fine to me.

But of course "Not crahsed" should be "Not crashed" :-)

Comment 11

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

Thank you!
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c6bb40c217
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e3eb662e4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/53afe9fba5c0

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e26d50054025

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c4c6bb40c217
https://hg.mozilla.org/mozilla-central/rev/b6e3eb662e4e
https://hg.mozilla.org/mozilla-central/rev/53afe9fba5c0