[Affected Versions]:

• Nightly 70.0a1

[Affected Platforms]

• All Windows
• All Mac
• All Linux

[Prerequisites]

• Have a Firefox profile with multiple logins saved.

[Steps to reproduce]:

1. Open the latest Nightly browser with the profile from prerequisites.
2. Navigate to "about:logins" page.
3. Select a saved login that has a favicon.
4. Edit the login and click the "Save" button.
5. Observe the website's favicon.

[Expected results]:

• The website's favicon is correctly displayed.

[Actual results]:

• The website's favicon changes into the default one.

[Notes]:

• If the page is refreshed the website's favicon is correctly displayed.
• Attached a screen recording with the issue.

After the investigation of this issue, I realize that this problem not only occurs when editing the existing logins, but also happens when creating a new login. After creating a new login, it will also show the default favicon, the page needs to be refreshed in order to get the correct favicon.

The actual problem is that after either process of loginAdded or loginModified, the login does not have the faviconDataURI attribute, and the favicon will change to default because of that. The function which added the faviconDataURI attribute to login is addFavicons, which only get called when we refresh the page, so this is why the favicon will displays correctly after refreshing the page. (Actually, the items in the left list also get affected, but the code for it didn't change the favicon to default when there is no faviconDataURI attribute, so it will just stay as what it is. I think that should also be changed after fix the main issue since this approach seems a little bit tricky)

So I think the addFavicons function needs to be called each time we add or edit the logins, but the event.detail.value used for SendFavicons is different from added or modified, and I don't know how to get that value (favicons) while added or modified the login. I think I need to change something in AboutLoginsParent.jsm, but I'm not sure how can I make it works. Is it possible that I can get some hint about how to solve this issue?

The addFavicoms function call is pretty expensive. Instead of calling it on any update, can you use Object.assign to update the properties of the login without overwriting the favicon property?

Backed out for bc failures in aboutlogins/tests/browser/browser_confirmDeleteDialog.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Csuperseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=os%2Cx%2C10.14%2Cshippable%2Copt%2Cmochitests%2Ctest-macosx1014-64-shippable%2Fopt-mochitest-browser-chrome-e10s-1%2Cm%28bc1%29&fromchange=21823cc121eb93bd13aecf27f8b88fc260eca40e&tochange=d3f4115964cc3920bc29f1287ce22a5da298e8ae&selectedJob=267149123

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=267159435&repo=autoland&lineNumber=2054

Backout: https://hg.mozilla.org/integration/autoland/rev/cd796dafa4955124c901cbd16ef2db9655d06cd6

Hi Jared, I've tried to run the specific failed test with ./mach test browser/components/aboutlogins/tests/browser/browser_confirmDeleteDialog.js and also run the whole folder which contains this test, but I didn't get any test failure. Do you know what might cause that failure?

I saw that same test failure on my machine when I was building on top of your patch. It seems intermittent, maybe you need to run the full directory a few times before you see it.

I don't see anything in your patch that could have caused it. I will look later today to see if something landed before you that caused it but we are just noticing the intermittent test failure now.

If it is caused by your patch, then I would try reverting your patch chunk by chunk to see if you can narrow down what change caused it. That would help in fixing it.

I think the issue is unrelated to your patch, but the fix would be adding cancelButton to the array that is passed to translateElements,
https://searchfox.org/mozilla-central/rev/4218cb868d8deed13e902718ba2595d85e12b86b/browser/components/aboutlogins/tests/browser/browser_confirmDeleteDialog.js#33-37

Right now that test is waiting for the elements to get translated, though nothing is waiting for the cancelButton to get translated. Then the test fails if the translation isn't present.

I have confirmed locally that adding cancelButton to that array fixes the test failure.

I've run the full test directory with ./mach test browser/components/aboutlogins/tests/browser like more than twenty times in my local machine, but I passed all of them without any test failure. Did I use the wrong test command?

And for the cancelButton, do you mean I should change those lines in browser_confirmDeleteDialog.js to
await content.document.l10n.translateElements([ title, message, cancelButton, confirmDeleteButton, ]);? Since I cannot get the test failure but you've already confirmed it works, should I just change that in my patch or it should be fixed in another bug?

I'm not sure why it is not failing for you but it must be a timing difference, maybe machine specific.

Yes you should make that change in this bug. I'm not worried about filing a new bug for that. Thanks.

Thank you, I just changed that in my patch.

https://hg.mozilla.org/mozilla-central/rev/244af235f976

https://hg.mozilla.org/releases/mozilla-beta/rev/5969692be113

I have verified this issue but is still reproducible on the latest Nightly 71.0a1 (Build ID: 20190922213341) and the latest Firefox Beta 70.0b8 (Build ID: 20190919164641) (64-bit) on Windows 7, MacOS 10.14 and Arch 4.14.

• The website's favicon still changes into the default one when the login is edited.

From what I understand from the comments the patch fixes another issue? Can you please give us more details about what this patch fixes and how we cand verify it?
Also, should we log another issue for the website's favicon?

(In reply to Cosmin Muntean, Experiments QA from comment #16)

I have verified this issue but is still reproducible on the latest Nightly 71.0a1 (Build ID: 20190922213341) and the latest Firefox Beta 70.0b8 (Build ID: 20190919164641) (64-bit) on Windows 7, MacOS 10.14 and Arch 4.14.

• The website's favicon still changes into the default one when the login is edited.

From what I understand from the comments the patch fixes another issue? Can you please give us more details about what this patch fixes and how we can verify it?

The other issue that this patch fixes is an intermittent issue with one of our tests. This does not need verification.

Also, should we log another issue for the website's favicon?

Yes, please log another issue for the website's favicon. I can also confirm that this patch did not address the issue.

I filed bug 1583267 for the new issue.

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

The reported issue which is described in comment 0 was fixed and verified in bug 1583267. Considering this I will mark this bug as verified - fixed.