1337259 - Don't show password autocomplete upon a right click into the password field

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Matthew N. [:MattN]]

STR:
1) Save 2 logins for a site
2) Reload the login page
3) When the password field is not focused, right-click in the password field to trigger the context menu (e.g. if you want to use the Fill Login option).

Expected result:
The context menu opens, autocomplete does not.

Actual result:
The first time you right-click in the password field both autocomplete and the context menu appear (overlapping each other).

Bug 376668 was supposed to address this using `mContextMenuFiredBeforeFocus` but it doesn't seem to work for me since the `focus` event gets dispatched before the `contextmenu` event in my testing (even though the event.timeStamp says the opposite). This was fixed for the username field in bug 1330111 attachment 8833235 [details] and the comments there are relevant.

There are two obvious solutions:
A) Implement the same approach as bug 1330111 attachment 8833235 [details] in nsFormFillController.cpp. 
** This doesn't require all password fields to be tracked in LoginManagerContent.jsm
B) Move the auto-show logic to LoginManagerContent.jsm re-using the username field code paths as much as possible.
** This makes sense if pwmgr is the only consumer who wants this behaviour.

Comment 1

[Tanvi Vyas[:tanvi]]

Matt, is this a P1 for 52?  From your description, it sounds like it.

Adding Johann and jkt to see if they have time to look at it.

Comment 2

[Matthew N. [:MattN]]

It's between P1 and P2 for me. I would say P1 if we had more time because it's not going to be a trivial patch and will touch C++.

Comment 3

[Johann Hofmann [:johannh]]

I'll take it. Option A) sounds less hacky, I haven't worked with Timers in our native code before, but I'm sure I can figure something out.

Comment 4

[Johann Hofmann [:johannh]]

Attachment: Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

Review commit: https://reviewboard.mozilla.org/r/111510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111510/

Comment 5

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c2f11ab41cb

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8835972 [details]
Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111510/diff/1-2/

Comment 7

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65e5e22d97fe

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8835972 [details]
Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

https://reviewboard.mozilla.org/r/111510/#review112856

::: toolkit/components/satchel/nsFormFillController.cpp:72
(Diff revision 2)
>  nsFormFillController::nsFormFillController() :
>    mFocusedInput(nullptr),
>    mFocusedInputNode(nullptr),
>    mListNode(nullptr),
> +  mFocusEventTimer(nullptr),
> +  mFocusAfterContextMenuTreshold(250),

Typo: "Threshold"

::: toolkit/components/satchel/nsFormFillController.cpp:1013
(Diff revision 2)
>      StartControllingInput(aInput);
>    }
>  }
>  
> +void
> +nsFormFillController::FocusEventCallback(nsITimer *aTimer, void *aClosure)

Nit: `FocusEventCallback` doesn't clearly enough convey that it's after a timeout. I guess the nsITimer can convey that but it seems easy to miss. Maybe `FocusEventDelayedCallback`?

::: toolkit/components/satchel/nsFormFillController.cpp:1017
(Diff revision 2)
> +  nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(self->mFocusedInputNode);
> +
> +  if (formControl) {

You're missing an important part of the JS code which ensures that the focused input is still the one from the Focus event after the timer:
```js
if (this._formFillService.focusedInput == focusedField) {
```

::: toolkit/components/satchel/nsFormFillController.cpp:1019
(Diff revision 2)
> +{
> +  nsFormFillController *self = (nsFormFillController *) aClosure;
> +
> +  nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(self->mFocusedInputNode);
> +
> +  if (formControl) {

```
if (!formControl) {
  return;
}
```

::: toolkit/components/satchel/nsFormFillController.cpp:1020
(Diff revision 2)
> +  nsFormFillController *self = (nsFormFillController *) aClosure;
> +
> +  nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(self->mFocusedInputNode);
> +
> +  if (formControl) {
> +    uint64_t timeDiff = (TimeStamp::Now() - self->mLastContextMenuEventTimeStamp).ToMilliseconds();

You're also missing taking the absolute value. See the comment in the JS about that. I'm trying to handle the event order changing like it seemed to based on comparing Dale's investigation and mine.

::: toolkit/components/satchel/nsFormFillController.cpp:1043
(Diff revision 2)
> -  nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(mFocusedInputNode);
> -  MOZ_ASSERT(formControl);
> +  if (!mFocusEventTimer) {
> +    mFocusEventTimer = do_CreateInstance("@mozilla.org/timer;1");
> -
> -  // If this focus doesn't immediately follow a contextmenu event then show
> -  // the autocomplete popup for all password fields.
> -  if (!mContextMenuFiredBeforeFocus
> -      && formControl->GetType() == NS_FORM_INPUT_PASSWORD) {
> -    ShowPopup();
>    }
> +  nsresult rv = mFocusEventTimer->InitWithFuncCallback(FocusEventCallback, this, 0,

I think we only need NS_DispatchToCurrentThread[1] here instead of a timer. I used a setTimeout in JS because I forgot about being about to dispatch a runnable in JS with `Services.tm.currentThread.dispatch(…)`.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Cross-thread_calls_using_runnables

::: toolkit/components/satchel/nsFormFillController.cpp:1043
(Diff revision 2)
> -  nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(mFocusedInputNode);
> -  MOZ_ASSERT(formControl);
> +  if (!mFocusEventTimer) {
> +    mFocusEventTimer = do_CreateInstance("@mozilla.org/timer;1");
> -
> -  // If this focus doesn't immediately follow a contextmenu event then show
> -  // the autocomplete popup for all password fields.
> -  if (!mContextMenuFiredBeforeFocus
> -      && formControl->GetType() == NS_FORM_INPUT_PASSWORD) {
> -    ShowPopup();
>    }
> +  nsresult rv = mFocusEventTimer->InitWithFuncCallback(FocusEventCallback, this, 0,

This patch isn't too complex but I kinda prefer option (B) so we can consolidate the logic in password manager and have nsFormFillController go back to being more generic. It would be interesting to see the option B approach.

Comment 9

[Dale Harvey (:daleharvey)]

Comment on attachment 8835972 [details]
Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

Clearing review as Matt has this covered and I couldnt see anything past Matts comments, yeh I really wanted to avoid both timers or hardcoding the event order in the original patch but its tricky, I still find it strange that there is no |originalTarget| or some flag telling us that the focus event originally came from a contextmenu press, I couldnt find it in my investigation however

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8835972 [details]
Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111510/diff/2-3/

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8835972 [details]
Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

https://reviewboard.mozilla.org/r/111510/#review114580

::: toolkit/components/satchel/nsFormFillController.cpp
(Diff revision 3)
> -
> -  // If this focus doesn't immediately follow a contextmenu event then show
> -  // the autocomplete popup for all password fields.
> -  if (!mContextMenuFiredBeforeFocus
> -      && formControl->GetType() == NS_FORM_INPUT_PASSWORD) {
> -    ShowPopup();

Doesn't this break the warning if pwmgr is disabled? The field won't be marked in that case but we want the warning to auto-appear. What about keeping a showpopup call here for unmarked pw fields but still remove the mcontext... since I don't think it worked anyways? That would mean the bug still happens for unmarked but that's better than the current situation. Otherwise maybe we should take your first approach.

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8835972 [details]
Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111510/diff/3-4/

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8835972 [details]
Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

https://reviewboard.mozilla.org/r/111510/#review115992

Seems okay to me, minus the below comments. Thanks!

::: toolkit/components/satchel/nsFormFillController.h:24
(Diff revision 4)
>  #include "nsIDOMHTMLInputElement.h"
>  #include "nsILoginManager.h"
>  #include "nsIMutationObserver.h"
>  #include "nsTArray.h"
>  #include "nsCycleCollectionParticipant.h"
> +#include "nsIFormControl.h"

Instead of adding another header here, we can forward declare, like we're doing with nsINode and nsPIDOMWindowOuter on lines 32-33. Reducing the number of includes helps to reduce build time.

::: toolkit/components/satchel/nsFormFillController.h:116
(Diff revision 4)
>    nsString mLastSearchString;
>  
>    nsDataHashtable<nsPtrHashKey<const nsINode>, bool> mPwmgrInputs;
>    nsDataHashtable<nsPtrHashKey<const nsINode>, bool> mAutofillInputs;
>  
> +  void FocusEventDelayedCallback(nsCOMPtr<nsIFormControl> formControl);

Generally, when defining functions like these that take pointers, we define it to take a pointer instead of a nsCOMPtr, so this should be:

```
void FocusEventDelayedCallback(nsIFormControl* formControl);
```

Not 100% sure why. Just always been that way. Some of the old-timers might know exact rationale.

::: toolkit/components/satchel/nsFormFillController.cpp:1012
(Diff revision 4)
>      StartControllingInput(aInput);
>    }
>  }
>  
> +void
> +nsFormFillController::FocusEventDelayedCallback(nsCOMPtr<nsIFormControl> aFormControl)

As in the definition, this should probably be:

```
nsFormFillController::FocusEventDelayedCallback(nsIFormControl* aFormControl)
```

::: toolkit/components/satchel/nsFormFillController.cpp:1021
(Diff revision 4)
> +  if (!formControl || formControl != aFormControl) {
> +    return;
> +  }
> +
> +  uint64_t timeDiff = fabs((TimeStamp::Now() - mLastContextMenuEventTimeStamp).ToMilliseconds());
> +  // If this focus doesn't immediately follow a contextmenu event then show

"immediately follow a contextmenu event" <-- should also mention our threshold and why we're checking that window of time.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8835972 [details]
Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

https://reviewboard.mozilla.org/r/111510/#review116300

::: toolkit/components/satchel/nsFormFillController.h:116
(Diff revision 4)
>    nsString mLastSearchString;
>  
>    nsDataHashtable<nsPtrHashKey<const nsINode>, bool> mPwmgrInputs;
>    nsDataHashtable<nsPtrHashKey<const nsINode>, bool> mAutofillInputs;
>  
> +  void FocusEventDelayedCallback(nsCOMPtr<nsIFormControl> formControl);

Nit: This should be defined above "// members /////////////…" with the other methods.

::: toolkit/components/satchel/nsFormFillController.cpp:1042
(Diff revision 4)
>    nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(mFocusedInputNode);
> -  MOZ_ASSERT(formControl);

Why are you removing this assertion?

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8835972 [details]
Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111510/diff/4-5/

Comment 16

[Johann Hofmann [:johannh]]

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

Comment 17

[Johann Hofmann [:johannh]]

> Why are you removing this assertion?

Good catch, that was because I originally removed both lines and then re-added the first line :)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5ba9006ad70
Don't show password autocomplete upon a right click into the password field. r=mconley

Comment 19

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

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

Comment 20

[Julien Cristau [:jcristau]]

Do you want to get this in beta53, or is the target for this change firefox 54?

Comment 21

[Johann Hofmann [:johannh]]

We should get this into 53, I wanted to get this some bake time on Nightly but then lost track of it. I suppose I can already make the uplift request. Thanks!

Comment 22

[Johann Hofmann [:johannh]]

Comment on attachment 8835972 [details]
Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

Approval Request Comment
[Feature/Bug causing the regression]: Bug in the original implementation (in 52)
[User impact if declined]: Users will get both the insecure password warning as well as the context menu on right click into an insecure input field, which looks really broken. This is broken in 52 but we considered the patch to risky for a super-late beta uplift.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Go to http://http-login.badssl.com/ and right-click into the password field. Make sure only the context menu pops up.
[List of other uplifts needed for the feature/fix]: Bug 1337772, which fixes an issue on slow machines (revealed by intermittent failures)
[Is the change risky?]: Medium
[Why is the change risky/not risky?]: It's native code that handles form input, but it had some bake time on Nightly already and for fixing bug 1337772 we spent a lot of time making sure this code is correct. It's covered by regression tests.
[String changes made/needed]: None

Comment 23

[Johann Hofmann [:johannh]]

Comment on attachment 8835972 [details]
Bug 1337259 - Don't show password autocomplete upon a right click into the password field.

Cancelling the approval request while we're figuring out bug 1337772

Comment 24

[Andrei Vaida [:avaida]]

Flagging this for manual testing, instructions in Comment 22.

Comment 25

[Ciprian Georgiu, Desktop QA]

I have reproduce this issue by following the STR from comment 0 and also comment 22, using an affected build.

This is no longer reproducible with 54 beta 2 under:
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS