1629174 - Login doorhangers show stale (possibly "") values for the username/password field if the doorhanger is dismissed immediately

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P3)

Description

[Matthew N. [:MattN]]

See bug 1608762. That site uses a form with target="_blank" so the doorhanger gets dismissed by the tab switch as soon as it opens. In this case we get a "showing" callback followed by a "dismissed" one but no "shown". The bug is that for some reason we were caching the values from the fields in "dismissed" and then using them for the next time the doorhanger is opened but the fields were only populated in "shown" so the values that got cached were from a previous instance of the doorhanger.

Removing the readDataFromUI(); from the "dismissed" callback fixes the issue… we already update the cache with an input event listener nowadays for both fields but that wasn't the case initially so I think this is just leftover… I can't think of a case where we would need to cache in dismissed where we wouldn't get input first… I tried to cause that but couldn't.

I also clear the fields now in the "dismissed" listener so that automated tests don't have stale values and therefore false positives. This has a nice side-effect of removing the credentials from memory sooner if the user doesn't save.

Comment 1

[Matthew N. [:MattN]]

Attachment: Bug 1629174 - Don't cache input values from the login doorhanger upon dismissal. r=sfoster

We already cache the values from input event listeners. The "dismissed" listener can happen before "shown" and cause old values to be used.

• Also clear the fields now in the "dismissed" listener so that automated tests don't have stale values and therefore false positives. This has a nice side-effect of removing the credentials from memory sooner if the user doesn't save.
• Added some logging for doorhanger opening/closing as it seems like it is low volume and could be useful in the future.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3789387d5563
Don't cache input values from the login doorhanger upon dismissal. r=sfoster

Comment 3

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/3789387d5563

Comment 4

[Timea Cernea [:tbabos][inactive]]

Attachment: Before and After Fix

Hi Matt,
Reproduced the stale username and password on an affected build and also checked out the latest Nightly for the fix.
The password is captured correctly, but the username is not captured.
Only the dismiss doorhanger is available for this site. Form submit opens the site in a new tab so we don't get the Save doorhanger.

Does this patch fix only the password capture part?

Comment 5

[Matthew N. [:MattN]]

Yes, this only fixes that the doorhanger fields were empty/stale even on other sites which opened form submissions in a new tab. Bug 1608762 will fix the username on that specific website.

Comment 6

[Timea Cernea [:tbabos][inactive]]

Sounds good, thank you. Marking it as verified-fixed on WIndows 10, MacOS 10.13 and Ubuntu 16.04 on the latest Beta 77.0b2 and Nightly 78.0a1 (2020-05-05).