Closed Bug 1577513 Opened 2 months ago Closed 26 days ago

The website's favicon from the login item changes to the default one if the saved login is edited

Categories

(Firefox :: about:logins, defect, P3)

70 Branch
Desktop
All
defect

Tracking

()

RESOLVED FIXED
Firefox 71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- disabled
firefox70 --- fixed
firefox71 --- fixed

People

(Reporter: cosmin.muntean, Assigned: chengy12)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [passwords:management] [skyline])

Attachments

(2 files)

Attached image website favicon.gif

[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.
Priority: -- → P3
Assignee: nobody → chengy12
Status: NEW → ASSIGNED

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?

Flags: needinfo?(jaws)

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?

Flags: needinfo?(jaws)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/613a1aca13b5
The website's favicon from the login item changes to the default one if the saved login is edited, r=jaws

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?

Flags: needinfo?(chengy12) → needinfo?(jaws)

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.

Flags: needinfo?(jaws)

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?

Flags: needinfo?(jaws)

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.

Flags: needinfo?(jaws)

Thank you, I just changed that in my patch.

Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/244af235f976
The website's favicon from the login item changes to the default one if the saved login is edited, r=jaws
Status: ASSIGNED → RESOLVED
Closed: 26 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

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?

Flags: needinfo?(jaws)

(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.

Flags: needinfo?(jaws)
Flags: needinfo?(cosmin.muntean)

I filed bug 1583267 for the new issue.

Flags: needinfo?(cosmin.muntean)

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

Keywords: regression
Component: Password Manager → about:logins
Product: Toolkit → Firefox
Target Milestone: mozilla71 → Firefox 71
You need to log in before you can comment on or make changes to this bug.