Within Bug 1119386 we started to use the document's principal as the loadingPrincipal of the channel. There are 2 functions where we default to using the systemPrincipal in case the principal argument is omitted: * setAndFetchFaviconForPage() * replaceFaviconDataFromDataURL() We should give addon authors a chance to update their code for 2 or 3 versions and then default to the nullPrinipcal instead of the systemPrincipal. See also: +++ b/toolkit/components/places/mozIAsyncFavicons.idl + * @param aLoadingPrincipal + * Principal of the page whose favicon is being set. If this argument + * is omitted, the loadingPrincipal defaults to the systemPrincipal + * and we cannot guarantee security checks anymore.

What would add-on devs need to change in their code?

Within Bug 1119386 I set the addon-compat flag. Once Bug 1119386 has landed we would like addon authors to provide aLoadingPrincipal for void setAndFetchFaviconForPage(in nsIURI aPageURI, in nsIURI aFaviconURI, in boolean aForceReload, in unsigned long aFaviconLoadType, - [optional] in nsIFaviconDataCallback aCallback); + [optional] in nsIFaviconDataCallback aCallback, + [optional] in nsIPrincipal aLoadingPrincipal); and void replaceFaviconDataFromDataURL(in nsIURI aFaviconURI, in AString aDataURL, - [optional] in PRTime aExpiration); + [optional] in PRTime aExpiration, + [optional] in nsIPrincipal aLoadingPrincipal); At the moment we fall back to using the systemPrincipal, but after a few versions from now we should default to using the nullPrincipal and block such favicon loads.

Jorge, we should update that soon, but we should probably land a warning for addon devs at least for one cycle. Potentially land a warning in FF49 and than convert to the code to default to the nullPrincipal for FF50. Those are the places we need to udpate: http://mxr.mozilla.org/mozilla-central/search?string=1227289 What is the standard way to log a deprecation warning these days so addon devs will also see it?

I don't know what the standard way of logging these errors is, but as long as it shows up in the Browser Console, it should be easy for developers and reviewers to pick up on it. Other than that, the message should explain clearly what they need to do, and/or point to this bug.

Is this only an issue for addons? Or does gecko code sometimes not pass in a principal, forcing us to fallback to system?

(In reply to Tanvi Vyas [:tanvi] from comment #5) > Is this only an issue for addons? Or does gecko code sometimes not pass in > a principal, forcing us to fallback to system? Taniv, we added an assertion to make sure that Gecko always passes a principal [1]. Addons, that don't pass a principal explicitly will use a systemPrincipal as a fallback. Please note that it might as well be that Gecko passes a SystemPrincipal. If that is of relevance for what you are trying to accomplish, then someone would have to verify that. [1] https://dxr.mozilla.org/mozilla-central/search?q=1227289&redirect=false

Bill, Bug 1119386 landed in FF45, I suppose we can now default to using the nullPrincipal instead of using the systemPrincipal in case addons don't provide an explicit loadingPrincipal. Thanks!

Comment on attachment 8782409 [details] [diff] [review] bug_1227289_default_to_nullprincipal_for_favicon_loads.patch Review of attachment 8782409 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, OK. I did a search in the addons dxr. Out of about 30 add-ons that use these functions, none have been updated. I'm fine doing this, but we may want to push on outreach a little more.

(In reply to Bill McCloskey (:billm) from comment #8) > Hmm, OK. I did a search in the addons dxr. Out of about 30 add-ons that use > these functions, none have been updated. I'm fine doing this, but we may > want to push on outreach a little more. Thanks, Bill. I already set the addon-compat flag for this bug.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f0faa3abd17e Default to the NullPrincipal rather than SystemPrincipal for Favicons. r=billm

Could anyone give me particular example how to update setAndFetchFaviconForPage call to achieve the result as it was before this bug has landed?

Which add-on are you referring to? How is the function called in your code?

(In reply to Jorge Villalobos [:jorgev] from comment #13) > Which add-on are you referring to? How is the function called in your code? Setting ni? for JustOff.

I'm using setAndFetchFaviconForPage to prefetch and/or refresh favicons (https://addons.mozilla.org/en-US/firefox/files/browse/475401/file/content/thumbnail.js#L229). Can I simple add Services.scriptSecurityManager.getSystemPrincipal() to the args?

Since it looks like you're doing this from chrome code, I think the system principal is right.