1488296 - Slack icon spreading on Bookmarks Toolbar

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[botsarehots]

Attachment: slackIsSpreading.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704192850

Steps to reproduce:

I do not know for sure how to reproduce this.

I guess use Slack while on Ubuntu 18.04 (though I believe this happened on 16.04 and some other versions of Firefox too). Have Slack on your Bookmarks Toolbar and use the Slack icons along with others to navigate to your most used sites.


Actual results:

As seen in the attached picture, my Bookmarks Toolbar has four Slack icons on it. The only one that leads to Slack is the far right one. Initially (months ago) only one of the icons swapped from their original (netflix.com, second slack icon from right). Then it spread to the other two (messages.android.com and web.groupme.com) fairly recently (past three weeks).


Expected results:

The bookmarks should have retained their original site's icons.

Comment 1

[botsarehots]

A quick note:

If I log into my Firefox account on another machine, (AKA my CentOS 7 laptop,) the Bookmarks Toolbar's icons will be icons from their original site; only one Slack icon will be present. (I do use this machine far less so maybe it would spread with time.)

Comment 2

[botsarehots]

Another quick comment: deleting the bookmark and re-bookmarking does not solve this.

Comment 3

[laszlo.bialis]

Hi!


I've tried to reproduce the issue mentioned above but without success.  I've tried both on Ubuntu 16.04 and 18.04 on the latest Release 62.0 Build ID: 20180830143136 and latest Nightly 64.0a1 Build ID: 20180906100252. 

Could you please retest using safe mode (https://goo.gl/AR5o9d), maybe even a new clean Firefox profile (https://goo.gl/AWo6h8), to eliminate add-ons or custom settings as a possible cause?

Comment 4

[botsarehots]

(In reply to laszlo.bialis from comment #3)
> Hi!
> 
> 
> I've tried to reproduce the issue mentioned above but without success.  I've
> tried both on Ubuntu 16.04 and 18.04 on the latest Release 62.0 Build ID:
> 20180830143136 and latest Nightly 64.0a1 Build ID: 20180906100252. 
> 
> Could you please retest using safe mode (https://goo.gl/AR5o9d), maybe even
> a new clean Firefox profile (https://goo.gl/AWo6h8), to eliminate add-ons or
> custom settings as a possible cause?


Thank you for responding.

Here are the results of your two requests.

Request 1: Using safe mode.
* There was no change to Bookmarks Toolbar's icon appearances. Meaning that there were still four Slack icons as shown in the PNG attachment.

Request 2: New clean Firefox profile.
* This does not read from my original Bookmarks Toolbar. Therefore the appearance of the Bookmarks Toolbar was non existent.
Using this process I found a quicker way to reproduce the error in Firefox 61.0.1
1. Enable the Bookmarks Toolbar from the Customize tab.
2. Bookmark `slack.com` by visiting `slack.com` and dragging the https lock onto the Bookmarks Toolbar.
3. Repeat step 3 with `messages.android.com`.
4. Rapidly (and repeatedly) click between the newly created bookmarks for `slack.com` and `messages.android.com`.
This should cause the bookmark icon for `messages.android.com` to change to the bookmark icon for `slack.com` on the Bookmarks Toolbar.

Comment 5

[botsarehots]

(In reply to botsarehots from comment #4)

I made a simple mistake in my reply.

Under "Request 2: New clean Firefox profile." in step 3, it should say:
"3. Repeat step 2 with `messages.android.com`."
Instead of the original:
"3. Repeat step 3 with `messages.android.com`.".

Apologies to any future AI overlords that are stuck in an infinite loop.

Comment 6

[laszlo.bialis]

Hi,

I've tried again to reproduce this behavior, but again without success. This time I've used only Ubuntu 18.04 x64, but tried with the latest Nightly 64.0a1 Build ID: 20180907100116 and also Release 61.0.1 Build ID: 20180704003137.

Can you give us a screen shot of the about:support page? You can easily get to this page if you type about:support and press Enter in a Firefox address bar.

Please also download Firefox Nightly from here: https://nightly.mozilla.org/  or the latest Release 62 from: mozilla.org and retest the problem. 
If you manage to reproduce the issue on the latest version of Nightly or Release, please also give us a screen recording, just to make sure that we understand the correct issue.

Comment 7

[botsarehots]

Attachment: Video requested for comment 6.

The bug is reproduced in Firefox Nightly 64.0a1. Recording aimed at 144 frames per second in 1920x1080. Operating system: Ubuntu MATE 18.04

Comment 8

[botsarehots]

Attachment: xhtml file in replacement for screenshot requested in comment 6.

If you are for whatever reason not able to see view the contents of this xhtml file appropriately, please let me know and I will do manual screenshots.

Comment 9

[botsarehots]

(In reply to laszlo.bialis from comment #6)
> Hi,
> 
> I've tried again to reproduce this behavior, but again without success. This
> time I've used only Ubuntu 18.04 x64, but tried with the latest Nightly
> 64.0a1 Build ID: 20180907100116 and also Release 61.0.1 Build ID:
> 20180704003137.
> 
> Can you give us a screen shot of the about:support page? You can easily get
> to this page if you type about:support and press Enter in a Firefox address
> bar.
> 
> Please also download Firefox Nightly from here: https://nightly.mozilla.org/
> or the latest Release 62 from: mozilla.org and retest the problem. 
> If you manage to reproduce the issue on the latest version of Nightly or
> Release, please also give us a screen recording, just to make sure that we
> understand the correct issue.

The two new attachments should fulfill these requests. I was able to successfully reproduce the bug in the nightly build. I replaced your screenshot request with the xhtml file made from "Save Page As..." --> "Web Page, complete". If this is not acceptable please let me know.

A minor detail I will put in now: The flavor of Ubuntu I am using is MATE. That is Ubuntu MATE 18.04.1 LTS 64 bit. (But I do not think this would differ significantly from Ubuntu 18.04 regarding Firefox.)

Comment 10

[laszlo.bialis]

Hi,

Thank you for your feedback!
I've also managed to reproduce the issue on Ubuntu 18.04 and on Win10 x64, on Nightly 64.0a1 Build ID: 20180910220142 with the steps from the provided video, with the difference that for me it took a larger period of time (approx 4 min) of rapidly and repeatedly clicking between the two bookmark icons, until the slack icon had doubled. Indeed after the icon doubled, deleting and saving again the bookmark, nor restarting the browser(by upgrade) didn't solve the issue, the same slack icon remained. 
Assigning a component, please feel free to change it if it's not the correct one.

Comment 11

[Marco Bonardo [:mak]]

So, it looks like a race condition when we store favicons, I actually thought we had fixed all of these with recent favicons 
changes that actually associate a favicon load with a page uri, but I must be wrong.

We previously had reports of this bug happening with RES (Reddit Enhancement Suite) add-on, or other add-ons, when they manually manipulate <link>s in the page. That's clearly not the case here.
What we should be looking at if there's any async behavior between storing the favicon load and storing the associated page uri, because if that exists than the page uri could change in the meanwhile, or maybe the current uri is just not up-to-date.
It's is clear that this requires continuous loads to happen, and those confuse our code.

Comment 12

[Dave Townsend [:mossop]]

The problem is probably here: https://searchfox.org/mozilla-central/source/browser/components/places/PlacesUIUtils.jsm#182

We associate the passed icon with the browser's current URI which may have changed in the meantime. If we had the favicon loader pass along the page URI for the icon it would probably solve this.

Comment 13

[Marco Bonardo [:mak]]

bumping up since it looks easily actionable (I actually thought we already stopped using currentURI)

Comment 14

[Marco Bonardo [:mak]]

I'm not sure how to test this, but I can probably make the code change.

Comment 15

[Marco Bonardo [:mak]]

Attachment: Bug 1488296 - Race condition when setting favicons for a browser with a changed currentURI. r=Mossop

There is a race condition between the time we decide to fetch an icon and the time we actually store that icon, where the original browser currentURI may have changed.

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 9008470 [details]
Bug 1488296 - Race condition when setting favicons for a browser with a changed currentURI. r=Mossop

Dave Townsend [:mossop] (he/him) has approved the revision.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fddb4d2af447
Race condition when setting favicons for a browser with a changed currentURI. r=mossop

Comment 18

[Dorel Luca [:dluca]]

Backed out changeset fddb4d2af447 (bug 1488296) for browser-chrome failures on browser/components/originattributes/test/browser/browser_favicon_userContextId.js

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=198949154&repo=autoland&lineNumber=3050

INFO - TEST-PASS | browser/components/originattributes/test/browser/browser_favicon_userContextId.js | The cookie of the favicon loading is correct. - 
[task 2018-09-12T22:25:32.907Z] 22:25:32     INFO - Buffered messages finished
[task 2018-09-12T22:25:32.914Z] 22:25:32     INFO - TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_favicon_userContextId.js | The loadInfo has correct userContextId - Got 0, expected 1
[task 2018-09-12T22:25:32.916Z] 22:25:32     INFO - Stack trace:
[task 2018-09-12T22:25:32.917Z] 22:25:32     INFO - chrome://mochikit/content/browser-test.js:test_is:1304
[task 2018-09-12T22:25:32.918Z] 22:25:32     INFO - chrome://mochitests/content/browser/browser/components/originattributes/test/browser/browser_favicon_userContextId.js:observe:89
[task 2018-09-12T22:25:32.920Z] 22:25:32     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-09-12T22:25:32.923Z] 22:25:32     INFO - TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_favicon_userContextId.js | The loadingPrincipal of favicon loading from content should be the content prinicpal - 
[task 2018-09-12T22:25:32.924Z] 22:25:32     INFO - Stack trace:
[task 2018-09-12T22:25:32.925Z] 22:25:32     INFO - chrome://mochitests/content/browser/browser/components/originattributes/test/browser/browser_favicon_userContextId.js:observe:92
[task 2018-09-12T22:25:32.926Z] 22:25:32     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-09-12T22:25:32.928Z] 22:25:32     INFO - TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_favicon_userContextId.js | The triggeringPrincipal of favicon loading from content should be the content prinicpal - 
[task 2018-09-12T22:25:32.930Z] 22:25:32     INFO - Stack trace:
[task 2018-09-12T22:25:32.932Z] 22:25:32     INFO - chrome://mochitests/content/browser/browser/components/originattributes/test/browser/browser_favicon_userContextId.js:observe:94
[task 2018-09-12T22:25:32.933Z] 22:25:32     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-09-12T22:25:32.934Z] 22:25:32     INFO - TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_favicon_userContextId.js | uncaught exception - NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.getRequestHeader] at observe@chrome://mochitests/content/browser/browser/components/originattributes/test/browser/browser_favicon_userContextId.js:97:27
[task 2018-09-12T22:25:32.936Z] 22:25:32     INFO - 
[task 2018-09-12T22:25:32.938Z] 22:25:32     INFO - Stack trace:
[task 2018-09-12T22:25:32.938Z] 22:25:32     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1655
[task 2018-09-12T22:25:32.939Z] 22:25:32     INFO - OnErrorEventHandlerNonNull*chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:null:1635
[task 2018-09-12T22:25:32.940Z] 22:25:32     INFO - chrome://mochikit/content/browser-test.js:Tester:404
[task 2018-09-12T22:25:32.941Z] 22:25:32     INFO - chrome://mochikit/content/browser-harness.xul:createTester/</<:251
[task 2018-09-12T22:25:32.942Z] 22:25:32     INFO - GECKO(6435) | JavaScript error: chrome://mochitests/content/browser/browser/components/originattributes/test/browser/browser_favicon_userContextId.js, line 97: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.getRequestHeader]
[task 2018-09-12T22:25:32.942Z] 22:25:32     INFO - Console message: [JavaScript Error: "NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.getRequestHeader]" {file: "chrome://mochitests/content/browser/browser/components/originattributes/test/browser/browser_favicon_userContextId.js" line: 97}]
[task 2018-09-12T22:25:32.943Z] 22:25:32     INFO - observe@chrome://mochitests/content/browser/browser/components/originattributes/test/browser/browser_favicon_userContextId.js:97:27
[task 2018-09-12T22:25:32.945Z] 22:25:32     INFO - 
[task 2018-09-12T22:25:32.945Z] 22:25:32     INFO - TEST-PASS | browser/components/originattributes/test/browser/browser_favicon_userContextId.js | The loadInfo has correct userContextId -

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=fddb4d2af447a6dc9cf8edb35191aa1b67ec3c36

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

Comment 19

[Marco Bonardo [:mak]]

oops, I updated only one loadFavicon, but there are an external and an internal one.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4335c690a98c
Race condition when setting favicons for a browser with a changed currentURI. r=mossop

Comment 21

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4335c690a98c

Comment 22

[Marco Bonardo [:mak]]

Could you please verify if the problem is solved in the next Nightly?

Comment 23

[botsarehots]

The problem appears to be solved in 63.0a1 (2018-08-14) (64-bit). Thanks everyone!

Comment 24

[botsarehots]

(In reply to botsarehots from comment #23)
> The problem appears to be solved in 63.0a1 (2018-08-14) (64-bit). Thanks
> everyone!

Apologies, I copied the wrong version:
The problem appears to be solved in 64.0a1 (2018-09-20) (64-bit).

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 9008470 [details]
Bug 1488296 - Race condition when setting favicons for a browser with a changed currentURI. r=Mossop

Approval Request Comment
[Feature/Bug causing the regression]: not a regression
[User impact if declined]: This is a bug we have from some time, where the wrong icon may spread to other pages/bookmarks, we finally found the culprit and it would be nice if we could stop spreading misplaced icons. Removing some of these icons requires a clear history.
[Is this code covered by automated tests?]: no, it's complex
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: it would be nice, steps  are in previous comments
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: the logic doesn't change, we're just passing an additional argument rather than extracting it later
[String changes made/needed]: none

Comment 26

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9008470 [details]
Bug 1488296 - Race condition when setting favicons for a browser with a changed currentURI. r=Mossop

Bug fix on nightly for a week with no regression reported, uplift approved for 63 beta 9, thanks.

Comment 27

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/df9eabc4e3e1

Comment 28

[Cristian Fogel [:cfogel]]

Managed to reproduce using the method provided in the video on 64.0a1 (2018-09-07).

Verified with 64.0a1 (2018-09-24) on macOS 10.11, win10x64, Ubuntu 16.04LTS and can confirm the issue is no longer reproducible.
Tested for about 5minutes on each platform and the icons did not change.

Comment 29

[Cristian Fogel [:cfogel]]

Verified on 63.0b9 as well. Issue no longer encountered.

Comment 32

[gw.graebner]

Rem.: I had initiated 1490608 Wrong favicons... (-duplicate), FF63.01 (64), on Win 10 PRO 64


Sorry to say, but also in the present version FF63.01 the favicons are still not showing correctly with their correspondant Web-address ! The behaviour just changed in some way (meaning that there is a 'new mix' of icon/address pairs)...

Furthermore, I just started to use the FF start page, and recognized that the icons shown there are NOT IDENTICAL to the ones in the favourites dropdown; however, they are mixed up 'randomly' as well !


(Rem.: I tried to evaluate a bit more with FF's builtin tools, and it seemed that this might be caused by a server side issue; I identified a message 'unable to load --.ico' (from the requested site address)... - Please regard this a hint only; I didn't work on this matter systematically)


Please check again !

Comment 33

[Marco Bonardo [:mak]]

(In reply to gw.graebner from comment #32)
> Sorry to say, but also in the present version FF63.01 the favicons are still
> not showing correctly with their correspondant Web-address ! The behaviour
> just changed in some way (meaning that there is a 'new mix' of icon/address
> pairs)...

Thanks, could you please file a new bug and provide us some examples/screenshot of websites showing the wrong icon?

> Furthermore, I just started to use the FF start page, and recognized that
> the icons shown there are NOT IDENTICAL to the ones in the favourites
> dropdown; however, they are mixed up 'randomly' as well !

By randomly do you mean they are from different pages, or just different from the tab/bookmarks but part of the correct site?

Comment 34

[Marco Bonardo [:mak]]

Oh nevermind filing a new bug, I'll reopen your original one for investigation.