Closed Bug 1376973 Opened 2 years ago Closed 2 years ago

The favicon of tabs dropdown list does not honor originAttributes.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: timhuang, Assigned: timhuang)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [tor][tor 22452][OA][userContextId][domsecurity-active])

Attachments

(2 files)

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
Priority: -- → P1
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.
Priority: P1 → P2
Whiteboard: [tor][OA][userContextId][domsecurity-active] → [tor][tor 22452][OA][userContextId][domsecurity-active]
Tor Browser's current patch is at https://torpat.ch/22452
We have not yet implemented a regression test.
Tim, are you working on this?
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 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.
Attachment #8918572 - Flags: review?(amarchesini)
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
Attachment #8918573 - Flags: review?(amarchesini) → review+
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.
Attachment #8918572 - Flags: review?(dao+bmo) → review-
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
Attachment #8918572 - Flags: review?(dao+bmo) → review+
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
Attachment #8918572 - Flags: review?(arthuredelstein) → review+
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!
Attachment #8918573 - Flags: review?(arthuredelstein) → review+
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
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.
Flags: needinfo?(tihuang)
Thanks, I have updated the code and try shows that eslint is passed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d49ede4d0b23
Flags: needinfo?(tihuang)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cb3d7a0ae189
https://hg.mozilla.org/mozilla-central/rev/786495fe83ad
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.