1363671 - No favicon on about:addons

Status: VERIFIED FIXED

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

Description

[Paul Silaghi, QA [:pauly]]

[Affected versions]:
- 54b6

[Affected platforms]:
- all

[Steps to reproduce]:
1. Start Fx with a clean profile
2. In a new tab, open about:addons (by typing in the location bar, and not by using the keys shortcut)

[Expected result]:
- Addons Manager opens and has a favicon

[Actual result]:
- Addons Manager opens and doesn't have a favicon

[Regression range]:
- recent regression, doesn't reproduce on Fx 53

[Additional notes]:
- on 55.0a1 (2017-05-09), the favicon is displayed after ~3-4sec

Comment 1

[Gerry Chang [:gchang]]

Hi :ckerschb, 
Can you help find someone to look at this issue?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

I can reproduce the problem, in fact I checked out mozilla-central, aurora and beta. I can't reproduce on mozilla-central, but I can reproduce on aurora and beta. Here are my STRs

1) Visit some http(s): webpage (e.g. https://www.mozilla.org/en-US/firefox/)
2) Enter 'about:addons' in the URL bar
3) Observe that the favicon is not loaded

The problem is that the contentSecurityManager blocks the favicon load because the triggeringPrincipal is the previously visited webpage instead of SystemPrincipal:

>  channelURI: chrome://mozapps/skin/extensions/extensionGeneric-16.png
>  loadingPrincipal: SystemPrincipal
>  triggeringPrincipal: https://www.mozilla.org/en-US/firefox/
>  principalToInherit: nullptr
>  contentPolicyType: 41
>  securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS, SEC_ALLOW_CHROME, 

Looking at the stacktrace (underneath), the load originates from nsImageBoxFrame.cpp. This needs a little more investigation, but potentially Bug 1319908 and friends [1] might have something to do with that.

Ehsan, any chance you could take a look?

[1] https://hg.mozilla.org/mozilla-central/rev/5021d272e953


%---snip----

#0  0x00007fffe6dc5adb in nsContentSecurityManager::doContentSecurityCheck (aChannel=0x7fffbce88f70, aInAndOutListener=...)
    at /home/ckerschb/source/beta/dom/security/nsContentSecurityManager.cpp:554
#1  0x00007fffe3c0867c in nsBaseChannel::AsyncOpen2 (this=0x7fffbce88f20, aListener=0x7fffd3c323a0) at /home/ckerschb/source/beta/netwerk/base/nsBaseChannel.cpp:703
#2  0x00007fffe51edf1b in imgLoader::LoadImage (this=0x7fffca353710, aURI=0x7fffb3de7f00, aInitialDocumentURI=0x7fffc9eb9600, aReferrerURI=0x7fffc9eb9600, 
    aReferrerPolicy=mozilla::net::RP_Unset, aLoadingPrincipal=0x7fffbc85fe80, aLoadGroup=0x7fffc9fd4ae0, aObserver=0x7fffb3587580, aContext=0x7fffbf3f0550, aLoadingDocument=
    0x7fffca414000, aLoadFlags=5120, aCacheKey=0x0, aContentPolicyType=41, initiatorType=..., _retval=0x7fffc28b9518) at /home/ckerschb/source/beta/image/imgLoader.cpp:2236
#3  0x00007fffe52f2528 in nsContentUtils::LoadImage (aURI=0x7fffb3de7f00, aContext=0x7fffbf3f0550, aLoadingDocument=0x7fffca414000, aLoadingPrincipal=0x7fffbc85fe80, 
    aReferrer=0x7fffc9eb9600, aReferrerPolicy=mozilla::net::RP_Unset, aObserver=0x7fffb3587580, aLoadFlags=5120, initiatorType=..., aRequest=0x7fffc28b9518, aContentPolicyType=41)
    at /home/ckerschb/source/beta/dom/base/nsContentUtils.cpp:3359
#4  0x00007fffe7bda171 in nsImageBoxFrame::UpdateImage (this=0x7fffc28b94a8) at /home/ckerschb/source/beta/layout/xul/nsImageBoxFrame.cpp:246
#5  0x00007fffe7bd99aa in nsImageBoxFrame::AttributeChanged (this=0x7fffc28b94a8, aNameSpaceID=0, aAttribute=0x7fffdac7dc70, aModType=1)
    at /home/ckerschb/source/beta/layout/xul/nsImageBoxFrame.cpp:138
#6  0x00007fffe787984b in mozilla::GeckoRestyleManager::AttributeChanged (this=0x7fffd47eb6f0, aElement=0x7fffbf3f0550, aNameSpaceID=0, aAttribute=0x7fffdac7dc70, aModType=1, 
    aOldValue=0x7fffffff8e70) at /home/ckerschb/source/beta/layout/base/GeckoRestyleManager.cpp:316
#7  0x00007fffe78c3af5 in mozilla::RestyleManager::AttributeChanged (this=0x7fffd47eb6f0, aElement=0x7fffbf3f0550, aNameSpaceID=0, aAttribute=0x7fffdac7dc70, aModType=1, 
    aOldValue=0x7fffffff8e70) at /home/ckerschb/source/beta-obj-ff-dbg/dist/include/mozilla/RestyleManagerInlines.h:72
#8  0x00007fffe789920d in mozilla::PresShell::AttributeChanged (this=0x7fffc974c400, aDocument=0x7fffca414000, aElement=0x7fffbf3f0550, aNameSpaceID=0, aAttribute=0x7fffdac7dc70, 
    aModType=1, aOldValue=0x7fffffff8e70) at /home/ckerschb/source/beta/layout/base/PresShell.cpp:4399
#9  0x00007fffe55e1f59 in nsNodeUtils::AttributeChanged (aElement=0x7fffbf3f0550, aNameSpaceID=0, aAttribute=0x7fffdac7dc70, aModType=1, aOldValue=0x7fffffff8e70)
    at /home/ckerschb/source/beta/dom/base/nsNodeUtils.cpp:145
#10 0x00007fffe5404d7f in mozilla::dom::Element::SetAttrAndNotify (this=0x7fffbf3f0550, aNamespaceID=0, aName=0x7fffdac7dc70, aPrefix=0x0, aOldValue=..., aParsedValue=..., 
    aModType=1 '\001', aFireMutation=false, aNotify=true, aCallAfterSetAttr=true) at /home/ckerschb/source/beta/dom/base/Element.cpp:2493
#11 0x00007fffe5404274 in mozilla::dom::Element::SetAttr (this=0x7fffbf3f0550, aNamespaceID=0, aName=0x7fffdac7dc70, aPrefix=0x0, aValue=..., aNotify=true)
    at /home/ckerschb/source/beta/dom/base/Element.cpp:2356
#12 0x00007fffe4ceeb08 in nsIContent::SetAttr (this=0x7fffbf3f0550, aNameSpaceID=0, aName=0x7fffdac7dc70, aValue=..., aNotify=true)
    at /home/ckerschb/source/beta/dom/base/nsIContent.h:365
#13 0x00007fffe72336de in nsXBLPrototypeBinding::AttributeChanged (this=0x7fffbf5d3500, aAttribute=0x7fffd87d86a0, aNameSpaceID=0, aRemoveFlag=false, aChangedElement=0x7fffbfcce230, 
    aAnonymousContent=0x7fffbf3f00d0, aNotify=true) at /home/ckerschb/source/beta/dom/xbl/nsXBLPrototypeBinding.cpp:379
#14 0x00007fffe7220981 in nsXBLBinding::AttributeChanged (this=0x7fffbf46d740, aAttribute=0x7fffd87d86a0, aNameSpaceID=0, aRemoveFlag=false, aNotify=true)
    at /home/ckerschb/source/beta/dom/xbl/nsXBLBinding.cpp:615
#15 0x00007fffe5404999 in mozilla::dom::Element::SetAttrAndNotify (this=0x7fffbfcce230, aNamespaceID=0, aName=0x7fffd87d86a0, aPrefix=0x0, aOldValue=..., aParsedValue=..., 
    aModType=1 '\001', aFireMutation=false, aNotify=true, aCallAfterSetAttr=true) at /home/ckerschb/source/beta/dom/base/Element.cpp:2457
#16 0x00007fffe5404274 in mozilla::dom::Element::SetAttr (this=0x7fffbfcce230, aNamespaceID=0, aName=0x7fffd87d86a0, aPrefix=0x0, aValue=..., aNotify=true)
    at /home/ckerschb/source/beta/dom/base/Element.cpp:2356
#17 0x00007fffe5400935 in mozilla::dom::Element::SetAttribute (this=0x7fffbfcce230, aName=..., aValue=..., aError=...) at /home/ckerschb/source/beta/dom/base/Element.cpp:1238
#18 0x00007fffe60ec1e6 in mozilla::dom::ElementBinding::setAttribute (cx=0x7fffdc632000, obj=..., self=0x7fffbfcce230, args=...)
    at /home/ckerschb/source/beta-obj-ff-dbg/dom/bindings/ElementBinding.cpp:726
#19 0x00007fffe63aedf3 in mozilla::dom::GenericBindingMethod (cx=0x7fffdc632000, argc=2, vp=0x7fffd868e1e0) at /home/ckerschb/source/beta/dom/bindings/BindingUtils.cpp:2953
#20 0x00007fffe989f824 in js::CallJSNative (cx=0x7fffdc632000, native=0x7fffe63aebc0 <mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/ckerschb/source/beta/js/src/jscntxtinlines.h:282
#21 0x00007fffe987afd1 in js::InternalCallOrConstruct (cx=0x7fffdc632000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:448
#22 0x00007fffe987b34a in InternalCall (cx=0x7fffdc632000, args=...) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:493
#23 0x00007fffe987b388 in js::CallFromStack (cx=0x7fffdc632000, args=...) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:499
#24 0x00007fffe9889192 in Interpret (cx=0x7fffdc632000, state=...) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:2990
#25 0x00007fffe987abd3 in js::RunScript (cx=0x7fffdc632000, state=...) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:394
#26 0x00007fffe987b0af in js::InternalCallOrConstruct (cx=0x7fffdc632000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:466
#27 0x00007fffe987b34a in InternalCall (cx=0x7fffdc632000, args=...) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:493
#28 0x00007fffe987b388 in js::CallFromStack (cx=0x7fffdc632000, args=...) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:499
#29 0x00007fffe9889192 in Interpret (cx=0x7fffdc632000, state=...) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:2990
#30 0x00007fffe987abd3 in js::RunScript (cx=0x7fffdc632000, state=...) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:394
#31 0x00007fffe987b0af in js::InternalCallOrConstruct (cx=0x7fffdc632000, args=..., construct=js::NO_CONSTRUCT) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:466
#32 0x00007fffe987b34a in InternalCall (cx=0x7fffdc632000, args=...) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:493
#33 0x00007fffe987b404 in js::Call (cx=0x7fffdc632000, fval=..., thisv=..., args=..., rval=...) at /home/ckerschb/source/beta/js/src/vm/Interpreter.cpp:512
#34 0x00007fffe9df0048 in JS_CallFunctionValue (cx=0x7fffdc632000, obj=..., fval=..., args=..., rval=...) at /home/ckerschb/source/beta/js/src/jsapi.cpp:2828
#35 0x00007fffe533bbde in nsFrameMessageManager::ReceiveMessage (this=0x7fffc8ba2600, aTarget=0x7fffbf009c10, aTargetFrameLoader=0x7fffbf52f5e0, aTargetClosed=false, aMessage=..., 
    aIsSync=false, aCloneData=0x7fffbce8b1d8, aCpows=0x7fffffffc090, aPrincipal=0x0, aRetVal=0x0) at /home/ckerschb/source/beta/dom/base/nsFrameMessageManager.cpp:1071

Comment 3

[(no longer active)]

I don't know why you think this is caused by bug 1319908, since a) that bug was OSX only, b) it didn't use the triggering principal in any way, and c) it affected the favicon in the bookmarks menu.  Based on that, this is almost certainly not a regression from that bug.

I unfortunately don't have time to look into it to figure what actually regressed it.  Someone needs to bisect it to get a regression range and ask the person who really regressed it.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #3)
> I don't know why you think this is caused by bug 1319908, since a) that bug
> was OSX only, b) it didn't use the triggering principal in any way, and c)
> it affected the favicon in the bookmarks menu.  Based on that, this is
> almost certainly not a regression from that bug.

I thought because that bug changes the loadingPrincipal for some image loads which internally is then used as the triggeringPrincipal for the favicon load in question  :-) Either way, someone needs to find a regression range. Gerry, can you provide that?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: 9edc76120a27a6c407e9f45d0b7a9b0877b80f57
INFO: First bad revision: 584df356a66b1f5d48c73625dd08589a11029845
INFO: Pushlog:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=9edc76120a27a6c407e9f45d0b7a9b0877b80f57&tochange=584df356a66b1f5d48c73625dd08589a11029845

I can't reproduce this on trunk or aurora builds at all. Nothing obvious in that regression range, but maybe bug 1346288 somehow?

Comment 6

[(no longer active)]

Removing myself from the CC list.

Comment 7

[Gerry Chang [:gchang]]

Hi Gabor, 
Can I have your help to shed some light here?

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Gerry Chang [:gchang] from comment #7)
> Hi Gabor, 
> Can I have your help to shed some light here?

This looks like a timing issue somewhere in xul, which I'm not very familiar with.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> INFO: Last good revision: 9edc76120a27a6c407e9f45d0b7a9b0877b80f57
> INFO: First bad revision: 584df356a66b1f5d48c73625dd08589a11029845
> INFO: Pushlog:
> https://hg.mozilla.org/releases/mozilla-beta/
> pushloghtml?fromchange=9edc76120a27a6c407e9f45d0b7a9b0877b80f57&tochange=584d
> f356a66b1f5d48c73625dd08589a11029845
> 
> I can't reproduce this on trunk or aurora builds at all. Nothing obvious in
> that regression range, but maybe bug 1346288 somehow?

No, I'm pretty sure that is unrelated.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> The problem is that the contentSecurityManager blocks the favicon load
> because the triggeringPrincipal is the previously visited webpage instead of
> SystemPrincipal:

The solution seems to be to not update the favicon before the document would have the correct new URL, but I have no idea where any of this happen in the code, so I'm not the right person to figure out where did this timing issue happen and what fixed it..

Based on bug 1277803 I think who might know this area a bit more is Tim. I'm passing the ball to him.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

FWIW, Try bisection says this is a regression from rev 6073f26ea1db (bug 1356695). I have no idea how it can be true, but I've also confirmed that mozilla-beta tip w/ the revisions from that bug (and one other they depend on) backed out no longer reproduces.

Comment 10

[Tim Huang[:timhuang]]

Yes, this is a timing issue. The root cause of this problem is that favicon loading been triggered before the 'iconLoadingPrincipal' has been updated properly. This 'iconLoadingPrincipal' will become the trigger principal when loading the favicon. So, the triggering principal is incorrect and causes this problem.

Maybe Bug 1356695 changes the timing and trigger this issue. I will submit a patch to resolve this.

Comment 11

[Tim Huang[:timhuang]]

Attachment: Bug 1363671 - Making the window attribute 'iconLoadingPrincipal' been set before than window attributes 'image' during favicon loading.

This patch makes window attributes 'iconLoadingPrincipal' been set before than
window attributes 'image' during loading favicon to prevent the timing issue.
Since setting the 'image' will trigger the favicon loading, so we have to set
'iconLoadingPrincipal' early than that to make sure nsImageBoxFrame will use the
correct 'iconLoadingPrincipal'.

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

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8869000 [details]
Bug 1363671 - Making the window attribute 'iconLoadingPrincipal' been set before than window attributes 'image' during favicon loading.

https://reviewboard.mozilla.org/r/140676/#review145014

thanks

::: commit-message-baf05:7
(Diff revision 1)
> +
> +This patch makes window attributes 'iconLoadingPrincipal' been set before than
> +window attributes 'image' during loading favicon to prevent the timing issue.
> +Since setting the 'image' will trigger the favicon loading, so we have to set
> +'iconLoadingPrincipal' early than that to make sure nsImageBoxFrame will use the
> +correct 'iconLoadingPrincipal'.

Setting window attribute 'iconLoadingPrincipal' before window attribute 'image' which allows using the right principal for favicon loads and prevents intermittent timing issues.

Comment 13

[Tim Huang[:timhuang]]

Comment on attachment 8869000 [details]
Bug 1363671 - Making the window attribute 'iconLoadingPrincipal' been set before than window attributes 'image' during favicon loading.

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

Comment 14

[Mozilla Autoland]

hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 1 changes to 1 files (+1 heads)
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Error accessing https://api.pub.build.mozilla.org/treestatus/trees/try :
remote: HTTP Error 503: Service Temporarily Unavailable
remote: Unable to check if the tree is open - treating as if CLOSED.
remote: To push regardless, include "CLOSED TREE" in your push comment.
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.a_treeclosure hook failed
abort: push failed on remote

Comment 15

[Tim Huang[:timhuang]]

Try looks good.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9edc6836e9b5
Make the window attribute 'iconLoadingPrincipal' be set before than window attributes 'image' during favicon loading. r=ckerschb

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9edc6836e9b5

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Given that we were only able to ever reproduce the issue on Beta, I've pushed this patch on Try on top of that repo so we can confirm that it works there too.

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

I've confirmed that I can still reproduce the problem on recent Beta CI builds but the Try push from comment 18 no longer does. Please nominate this for Beta approval when you get a chance :)

Comment 20

[Tim Huang[:timhuang]]

I have talked this with Gerry, he has some idea about this. So I'll pass the ball to him.

Comment 21

[Gerry Chang [:gchang]]

After discussing with Tim & Ryan, we think the risk should be low. It's worth uplifting to Beta54.

Hi Tim,
Can you help create the uplift request for Beta54?

Comment 22

[Tim Huang[:timhuang]]

Comment on attachment 8869000 [details]
Bug 1363671 - Making the window attribute 'iconLoadingPrincipal' been set before than window attributes 'image' during favicon loading.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1294886
[User impact if declined]: The favicon will not show in certain situations. Described in comment 2
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes
1) Visit some http(s): webpage (e.g. https://www.mozilla.org/en-US/firefox/)
2) Enter 'about:addons' in the URL bar
3) Observe that whether the favicon is loaded

[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: not risky
[Why is the change risky/not risky?]:
It's only reorder the sequence of setting window attributes during loading favicon. So, It is not risky.
[String changes made/needed]: none

Comment 23

[Gerry Chang [:gchang]]

Comment on attachment 8869000 [details]
Bug 1363671 - Making the window attribute 'iconLoadingPrincipal' been set before than window attributes 'image' during favicon loading.

Polish an UI issue that favicon is not shown. Beta54+. Should be in 54 beta 12.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/984ed49b767e

Comment 25

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Verified as fixed using Firefox 54 beta 12 under Win 10 64-bit, Ubuntu 16.04 LTS 64-bit and Mac OS X 10.10.