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

VERIFIED FIXED in Firefox 54

Status

()

Toolkit
Password Manager
P1
normal
VERIFIED FIXED
5 months ago
2 months ago

People

(Reporter: MattN, Assigned: johannh)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 unaffected, firefox52 wontfix, firefox53 affected, firefox54 verified)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
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.
Flags: needinfo?(MattN+bmo)
Priority: -- → P1
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++.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 3

5 months ago
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.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 5

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c2f11ab41cb
Comment hidden (mozreview-request)
(Assignee)

Comment 7

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65e5e22d97fe
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.
Attachment #8835972 - Flags: review?(MattN+bmo)
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
Attachment #8835972 - Flags: review?(dale)
Comment hidden (mozreview-request)
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.
Attachment #8835972 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)

Comment 13

4 months ago
mozreview-review
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.
Attachment #8835972 - Flags: review?(mconley) → review+
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 hidden (mozreview-request)
(Assignee)

Comment 16

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbf2fe788f07
(Assignee)

Comment 17

4 months ago
> 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

4 months ago
Pushed by jhofmann@mozilla.com:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e5ba9006ad70
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

4 months ago
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Do you want to get this in beta53, or is the target for this change firefox 54?
status-firefox52: affected → wontfix
Flags: needinfo?(jhofmann)
(Assignee)

Comment 21

4 months ago
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!
Flags: needinfo?(jhofmann)
(Assignee)

Comment 22

4 months ago
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
Attachment #8835972 - Flags: approval-mozilla-beta?
(Assignee)

Comment 23

4 months ago
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
Attachment #8835972 - Flags: approval-mozilla-beta?
Flagging this for manual testing, instructions in Comment 22.
Flags: qe-verify+
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
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.