1376973 - The favicon of tabs dropdown list does not honor originAttributes.

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Tim Huang[:timhuang]]

The drop down list of tabs will also load favicon and this should follow the originAttributes. However, it doesn't right now. 

[1] http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#7767

Comment 1

[Ethan Tseng [:ethan]]

This bug is important but not very urgent for First Party Isolation.  Set it as P2 for now.
If the Containers team has a different opinion, we could change it.

Comment 2

[Arthur Edelstein [:arthur]]

Tor Browser's current patch is at https://torpat.ch/22452
We have not yet implemented a regression test.

Comment 3

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020

Comment 4

[Tanvi Vyas[:tanvi]]

Tim, are you working on this?

Comment 5

[Tim Huang[:timhuang]]

I havn't spent time on this for a while, sorry. But, I want to resolve this issue. And I am planning to fix this in these two weeks. Thanks for bring this up.

Comment 6

[Tim Huang[:timhuang]]

Attachment: Bug 1376973 - Part 1: Making the favicon of the allTabs menu obeys originAttributes and rename attribute 'iconLoadingPrincipal' to 'iconloadingprincipal'.

Review commit: https://reviewboard.mozilla.org/r/189404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189404/

Comment 7

[Tim Huang[:timhuang]]

Attachment: Bug 1376973 - Part 2: Add test cases for making sure that favicons of allTabs popup menu is using correct originAttributes.

This patch adds test cases for testing favicons of allTabs menu in terms of originAttributes.
It adds tests in existing tests, browser_favicon_userContextId.js and browser_favicon_firstParty.js.
The test will open tabs with different originAttributes and trigger the popup of
allTabs menu. Then it will verify that whether the network requests of favicon
have correct originAttributes.

Review commit: https://reviewboard.mozilla.org/r/189406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189406/

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8918572 [details]
Bug 1376973 - Part 1: Making the favicon of the allTabs menu obeys originAttributes and rename attribute 'iconLoadingPrincipal' to 'iconloadingprincipal'.

https://reviewboard.mozilla.org/r/189404/#review195110

I cannot review these changes.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8918573 [details]
Bug 1376973 - Part 2: Add test cases for making sure that favicons of allTabs popup menu is using correct originAttributes.

https://reviewboard.mozilla.org/r/189406/#review195112

Comment 10

[Tim Huang[:timhuang]]

Comment on attachment 8918572 [details]
Bug 1376973 - Part 1: Making the favicon of the allTabs menu obeys originAttributes and rename attribute 'iconLoadingPrincipal' to 'iconloadingprincipal'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189404/diff/1-2/

Comment 11

[Tim Huang[:timhuang]]

Comment on attachment 8918573 [details]
Bug 1376973 - Part 2: Add test cases for making sure that favicons of allTabs popup menu is using correct originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189406/diff/1-2/

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8918572 [details]
Bug 1376973 - Part 1: Making the favicon of the allTabs menu obeys originAttributes and rename attribute 'iconLoadingPrincipal' to 'iconloadingprincipal'.

https://reviewboard.mozilla.org/r/189404/#review195376

I believe attribute names are supposed to be lowercase.

Comment 13

[Tim Huang[:timhuang]]

Comment on attachment 8918572 [details]
Bug 1376973 - Part 1: Making the favicon of the allTabs menu obeys originAttributes and rename attribute 'iconLoadingPrincipal' to 'iconloadingprincipal'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189404/diff/2-3/

Comment 14

[Tim Huang[:timhuang]]

Comment on attachment 8918573 [details]
Bug 1376973 - Part 2: Add test cases for making sure that favicons of allTabs popup menu is using correct originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189406/diff/2-3/

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8918572 [details]
Bug 1376973 - Part 1: Making the favicon of the allTabs menu obeys originAttributes and rename attribute 'iconLoadingPrincipal' to 'iconloadingprincipal'.

https://reviewboard.mozilla.org/r/189404/#review195812

Comment 16

[Arthur Edelstein [:arthur]]

Comment on attachment 8918572 [details]
Bug 1376973 - Part 1: Making the favicon of the allTabs menu obeys originAttributes and rename attribute 'iconLoadingPrincipal' to 'iconloadingprincipal'.

https://reviewboard.mozilla.org/r/189404/#review195968

Comment 17

[Arthur Edelstein [:arthur]]

Comment on attachment 8918573 [details]
Bug 1376973 - Part 2: Add test cases for making sure that favicons of allTabs popup menu is using correct originAttributes.

https://reviewboard.mozilla.org/r/189406/#review195970

Nice work!

Comment 18

[Tim Huang[:timhuang]]

Try looks good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a42a99dc2fc6

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f393e8b236d
Part 1: Making the favicon of the allTabs menu obeys originAttributes and rename attribute 'iconLoadingPrincipal' to 'iconloadingprincipal'. r=arthuredelstein,dao
https://hg.mozilla.org/integration/autoland/rev/c058687eb204
Part 2: Add test cases for making sure that favicons of allTabs popup menu is using correct originAttributes. r=arthuredelstein,baku

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failures, e.g. in browser/components/originattributes/test/browser/browser_favicon_firstParty.js:

https://hg.mozilla.org/integration/autoland/rev/df1853e9056e5722a274b5b373decf6d85088303
https://hg.mozilla.org/integration/autoland/rev/287fb2c54b04ea29744c1521ad5b5f8c279be907

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c058687eb20461d801d9e2a6ad2e0c28a9eb6d39&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=138210003&repo=autoland

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/originattributes/test/browser/browser_favicon_firstParty.js:374:24 | Use Services.cache2 rather than getService(). (mozilla/use-services)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/originattributes/test/browser/browser_favicon_userContextId.js:342:22 | Use Services.cache2 rather than getService(). (mozilla/use-services)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/originattributes/test/browser/browser_favicon_userContextId.js:357:22 | Use Services.cache2 rather than getService(). (mozilla/use-services)

As far as I know this is a change from bug 1230373 which landed in the last merge.

Comment 21

[Tim Huang[:timhuang]]

Comment on attachment 8918572 [details]
Bug 1376973 - Part 1: Making the favicon of the allTabs menu obeys originAttributes and rename attribute 'iconLoadingPrincipal' to 'iconloadingprincipal'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189404/diff/3-4/

Comment 22

[Tim Huang[:timhuang]]

Comment on attachment 8918573 [details]
Bug 1376973 - Part 2: Add test cases for making sure that favicons of allTabs popup menu is using correct originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189406/diff/3-4/

Comment 23

[Tim Huang[:timhuang]]

Thanks, I have updated the code and try shows that eslint is passed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d49ede4d0b23

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb3d7a0ae189
Part 1: Making the favicon of the allTabs menu obeys originAttributes and rename attribute 'iconLoadingPrincipal' to 'iconloadingprincipal'. r=arthuredelstein,dao
https://hg.mozilla.org/integration/autoland/rev/786495fe83ad
Part 2: Add test cases for making sure that favicons of allTabs popup menu is using correct originAttributes. r=arthuredelstein,baku

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cb3d7a0ae189
https://hg.mozilla.org/mozilla-central/rev/786495fe83ad