Closed Bug 1576199 Opened 4 months ago Closed 3 months ago

Dismissed door hanger - show password malfunctions (specific cases)

Categories

(Toolkit :: Password Manager, defect, P2, minor)

70 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- disabled
firefox70 --- verified
firefox71 --- verified

People

(Reporter: adrian_sv, Assigned: sfoster)

References

(Blocks 1 open bug)

Details

(Whiteboard: [passwords:capture-UI] [passwords:generation] [skyline])

Attachments

(3 files)

Attached video show_password.mp4
[Environment:]

Ubuntu 16.04, Windows 10
70.0a1 2019-08-22

[Prerequisites:]
  1. Create a new profile.
  2. Load a login form.
  3. Fill in and save some credentials.
[Steps:]
  1. Load or reload the login form and use autofill (one login saved) or autocomplete (more logins) in order to populate the password field.
  2. With the password field populated with the password (password is hidden) - rightclick/generate password or autocomplete/generate password
  3. As soon as the generated password is autosaved, click on the password message panel (blue key) and check show password.
[Expected Result:]

The password is unmasked when the show password checkbox is checked.

[Actual Result:]

The password is not unmasked when the show password checkbox is checked.

[Note:]

-closing and reopening the door hanger will restore proper functionality to show password.
-it's a bit intermittent also, tried to minimize STR to reproduce.

Flags: needinfo?(MattN+bmo)

debug logs, in case it helps:
https://pastebin.com/8eBgDpRQ

We'll try to reproduce this and see if there is a low-risk solution.

Priority: -- → P2
Blocks: 1569989
No longer blocks: 1569989
Blocks: 1569989

Sam, I think you were going to take this next.

Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)
Here's some logging illustrating the issue:

When showing the doorhanger for the auto-saved/generated password loging, we get 2 successive calls to LoginManagerPrompter.init() and LoginManagerPrompter._showLoginCaptureDoorhanger. I added logging in LoginManagerPrompter.jsm#handleEvent, and you can see at the end of the logs

LoginManagerPrompter: Handling event: removed LoginManagerPrompter.jsm:1313:22

The eventListeners are removed when handling this event, so the password toggle - and actually all input handling for the username and password fields - is unhooked.

(In reply to Sam Foster [:sfoster] (he/him) from comment #5)

The eventListeners are removed when handling this event, so the password toggle - and actually all input handling for the username and password fields - is unhooked.

Each call to LoginManagerParent._getPrompter() gets us a different instance of the LoginManagerPrompter. Unless the references attached to the PopupNotification created by _showLoginCaptureDoorhanger are getting cross-wired, this should be benign. Investigating further...

(In reply to Sam Foster [:sfoster] (he/him) from comment #5)

LoginManagerPrompter: Handling event: removed LoginManagerPrompter.jsm:1313:22

The eventListeners are removed when handling this event, so the password toggle - and actually all input handling for the username and password fields - is unhooked.

I'm not sure that's actually the case since IIUC the references to the functions will be different for each _showLoginCaptureDoorhanger call since the functions are defined inside it. So the removal is removing the event listener from the previous prompt which is good. The problem is that showing isn't called for the 2nd _showLoginCaptureDoorhanger and therefore there ends up being no event listener left.

I think the underlying issue would be that PopupNotifications dispatched the "remove" event in this case even though it doesn't dispatch the "showing"/"shown" ones for the notification getting updated. Fixing that seems like it could be tricky though and break compatibility… this specific consumer is especially tricky because of the event listeners inside <popupnotificationcontent> which use functions defined in the closure.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=267327918&revision=e3379dc4370abb6a7468a6575a72f27d3ec1b68d

The patch amends those existing tests that baked in an expectation of 2 messages from _generatedPasswordFilledOrEdited. I wasn't able to think of useful new tests though. If you have ideas, let me know.

I was able to reproduce the browser_autocomplete_footer.js intermittent in the try results using --verify locally on linux. That test clicks the autocomplete footer item to open the management UI, and does it again using keyboard events. The helper to open the management UI records a telemetry event. It looks like its just racing there, maybe the test should use a wait for condition. I don't see a way for this patch to affect the test outcome though, so I just want to keep any eye on it.

Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/cd22422277a8
Avoid sending redundant messages from LMC to trigger notifications from the parent. r=MattN
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Comment on attachment 9092726 [details]
Bug 1576199 - Avoid sending redundant messages from LMC to trigger notifications from the parent. r?MattN

Beta/Release Uplift Approval Request

  • User impact if declined: password doorhanger checkbox and buttons won't work properly in this case
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment 0.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): We considered a long list of possible fixes and this seemed like the least risky as it aligns with our behaviour for login form submission.
  • String changes made/needed: None
Attachment #9092726 - Flags: approval-mozilla-beta?

Comment on attachment 9092726 [details]
Bug 1576199 - Avoid sending redundant messages from LMC to trigger notifications from the parent. r?MattN

Skyline patch, OK for uplift. Let's verify this in beta 9.

Attachment #9092726 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Confirmed as fixed on 70.0b9 20190923154733 beta version and Nightly 71.0a1 (2019-09-23) on Linux 18.04, MacOs 10.14.5 and Windows 10 x64.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
You need to log in before you can comment on or make changes to this bug.