Closed Bug 1119386 Opened 9 years ago Closed 8 years ago

Use the loading document's principal to populate loadInfo for Favicons instead of using systemPrincipal

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: addon-compat)

Attachments

(5 files, 16 obsolete files)

5.88 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
3.27 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
13.30 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
26.10 KB, patch
mak
: review+
Details | Diff | Splinter Review
40.70 KB, patch
mak
: review+
Details | Diff | Splinter Review
Blocks: 1006868, 437014
Right now we use systemPrincipal in the loadInfo->loadingPrincipal.  Instead we should get the document the favicon is associated with and use it's principal as the loadingPrincipal.  In order to do this, we need to pass the document through various methods until the channel is created.
Blocks: 1006881
No longer blocks: 437014
Depends on: 437014
Component: DOM: Security → Places
Product: Core → Toolkit
Summary: Investigate security checks for favicons → Use the loading document's principal to populate loadInfo for Favicons instead of using systemPrincipal
Blocks: 1143922
Assignee: nobody → mozilla
Attached patch bug_1119386_toolkit.patch (obsolete) — Splinter Review
Attached patch bug_1119386_browser.patch (obsolete) — Splinter Review
Attached patch bug_1119386_docshell.patch (obsolete) — Splinter Review
Attached patch bug_1119386_dom.patch (obsolete) — Splinter Review
I went through all the callsites, extended the argument list where appropriate so that we pass the right loadingPrincipal (or loadInfo) all to way to where we actually create the channel for favicons. All favicons should now have the correct loadInfo arguments. Jonas, Tanvi, can you take a look before I flag modul peers for review?
Flags: needinfo?(tanvi)
Flags: needinfo?(jonas)
Attached patch combined_changes.patch (obsolete) — Splinter Review
One big patch containing all the changes (besides test updates).
Question for front-end team: So does this mean that we only use a xul:image for rendering? And *never* use the xul:image to actually trigger the network load of the favicon? What URI do we stick in the xul:image in order to do rendering?
Flags: needinfo?(jonas)
Comment on attachment 8603551 [details] [diff] [review]
combined_changes.patch

diff --git a/toolkit/components/places/BookmarkHTMLUtils.jsm b/toolkit/components/places/BookmarkHTMLUtils.jsm
--- a/toolkit/components/places/BookmarkHTMLUtils.jsm
+++ b/toolkit/components/places/BookmarkHTMLUtils.jsm
@@ -824,17 +824,19 @@ BookmarkImporter.prototype = {
    * the user visits the page. Our made up one should get expired when the
    * page no longer references it.
    */
   _setFaviconForURI: function setFaviconForURI(aPageURI, aIconURI, aData) {
     // if the input favicon URI is a chrome: URI, then we just save it and don't
     // worry about data
     if (aIconURI) {
       if (aIconURI.schemeIs("chrome")) {
+        // ckerschb: what's the right principal here? should we create one from the page URI?
For now, please do.  That is better than system and probably correct most of the time.  We'll see if the reviewers have a better idea.

         PlacesUtils.favicons.setAndFetchFaviconForPage(aPageURI, aIconURI,
+                                                       Services.scriptSecurityManager.getSystemPrincipal(),
                                                        false,
                                                        PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE);
         return;
       }
     }
Comment on attachment 8603551 [details] [diff] [review]
combined_changes.patch

Looks great Christoph!  Thank you for looking into this!

I have examples of mixed content favicons that might be helpful when we move security checks:
https://people.mozilla.org/~tvyas/mixedfavicon.html - uses link rel=icon
https://people.mozilla.org/~tvyas/mixedfavicon.html - uses link rel=short icon

There are apparently other ways to include favicons in a page, but I'm not familiar with them.  Perhaps some of the reviewers can provide details on how to write more test cases.
Attachment #8603551 - Flags: feedback+
Attachment #8603532 - Flags: review?(gijskruitbosch+bugs)
Attachment #8603534 - Flags: review?(gijskruitbosch+bugs)
Attachment #8603535 - Flags: review?(gijskruitbosch+bugs)
Attachment #8603536 - Flags: review?(bugs)
Attachment #8603537 - Flags: review?(bugs)
(In reply to Jonas Sicking (:sicking) from comment #9)
> Question for front-end team: So does this mean that we only use a xul:image
> for rendering? And *never* use the xul:image to actually trigger the network
> load of the favicon? What URI do we stick in the xul:image in order to do
> rendering?

Eh? I mean, casual inspection of the URI used for the tab favicon for this bug with DOM inspector shows:

src="https://bug1119386.bugzilla.mozilla.org/favicon.ico#-moz-resolution=16,16"

so, no, I would expect that those loads hit the network (or the cache, I suppose).


I've not looked at the patches in detail (will do that "today" post-sleep so when I actually feel like it's Wednesday instead of late Tuesday) but I expect that they fix (or attempt to fix) the favicon service used by bookmarks and perhaps also the location bar (less sure about that one). The bookmarks stuff uses URLs like:

image="moz-anno:favicon:http://www.mozilla.org/2005/made-up-favicon/1-1395118527460"

I haven't looked at how Christoph's patch would keep the requesting origin for those loads, and/or if it even needs to, and/or how suitable the same scheme would be for tab favicons.

Either way, ::mak may be a more suitable reviewer here. Marco, thoughts?
Flags: needinfo?(mak77)
Comment on attachment 8603532 [details] [diff] [review]
bug_1119386_toolkit.patch

Yeah, I don't know enough about this code to be of much use when reviewing this.

However... I believe the following things are true:

1) these URIs are stored in DBs and regurgitated for e.g. bookmarks. They are stored with the URI of the icon, not the URI of the page (this is easily verified via inspection with DOMI / browser toolbox of the XUL in which they are used).
2) multiple pages can have the same favicon specified
3) the "point" of these changes is that we want to ensure the favicon loads have the right principal
4) the URIs themselves have no principal embedded in them because the URI in there is just the target URI of the icon, not the loading principal
5) this patch doesn't change the URI representation
6) it follows logically that the patch cannot be effective in ensuring that favicon loads have the right principal, because we cannot disambiguate between a favicon URI for "http://www.mozilla.org/favicon.ico" initiated by mozilla.org and initiated by evil.org, because the URI scheme offers no such distinction.

Are my assumptions/conclusions correct?


Related: I noticed a lot of comments in the code that hardcode the system principal anyway, and that ask "where do we get the loading principal from here?". To me, this seems symptomatic of the above, but I could be misunderstanding the purpose of the patch, maybe? Not sure!

Finally... what are the concrete threat vectors we're trying to protect against here? As far as I know, pages cannot ascertain what happens to the load of a favicon, and a favicon in turn will know little of the page that's requesting to have it as a favicon (maybe the referrer, but I don't think so? Not sure - either way, this is within the control of the referring page so doesn't seem like something that could be abused by the favicon party, if it is different from the page party). It seems that the exposure in both directions is more limited than that of in-page images. If all a page wants is for images from evil.com to be loaded and displayed (for the sake of crashing/exploiting or whatever) by the browser it already has opportunities to do so that provide it with at least as much information, right?
Attachment #8603532 - Flags: review?(gijskruitbosch+bugs)
Attachment #8603534 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8603535 [details] [diff] [review]
bug_1119386_browser.patch

In case I'm totally missing the point here, please indicate in comments and request any further review from ::mak (or people he points to) instead. :-)
Attachment #8603535 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #13)
> 1) these URIs are stored in DBs and regurgitated for e.g. bookmarks.
> 2) multiple pages can have the same favicon specified
> 4) the URIs themselves have no principal embedded in them because the URI in
> there is just the target URI of the icon, not the loading principal
> 5) this patch doesn't change the URI representation
> 6) it follows logically that the patch cannot be effective

This is correct from the favicons storage and fetching point of view.
For bookmarks/history/locationbar we use special moz-anno: urls that just fetch blobs from a database, we don't load anything from the network.
We can associate the same blob to multiple uris, and in future we might want to fetch "the most common favicon" for a domain, based on the blobs.
We are though thinking to add network bypass when a requested favicon doesn't exist in the database (bug 573638)

The request here seems to change everything on paper, making some assumptions and future plans invalid. The plan has many obscurities and maybe needs more breakdown to take one consumer at a time.

There's risk to over-engineer stuff here, so I agree with Gijs it would be better to start from simple questions like: what we are trying to protect the user from, how that happens and how does that affect non-network fetches or shared icons.
Flags: needinfo?(mak77)
note the patch, as-is, is also breaking any add-on using the favicons API.
also CopyFavicon, is effectively only adding a reference to the database since the blob is already there, it's not fetching anything, so all of the changes to it are effectively only forwarding unused info atm.
> There's risk to over-engineer stuff here, so I agree with Gijs it would be
> better to start from simple questions like: what we are trying to protect
> the user from, how that happens and how does that affect non-network fetches
> or shared icons.

Currently every part of gecko and the Firefox frontend which opens a network connection does its own security checks before doing so. Which means that every place does slightly different security checks. Many times forgetting to do important checks like calling nsIContentPolicy or calling checkLoadURI. Forgetting to call nsIContentPolicy means that addons which implement security/privacy/content policies doesn't work. It also means that we won't do mixed-content-blocking, implement CSP, or do follow various other security policies that we have internally.

Forgetting to call checkLoadURI means that you can load things from the local filesystem or from chrome:// that you shouldn't be able to (this might not be a big deal for favicons specifically).

The second problem is redirect handling. If you do the load from chrome code you have to implement all your own security checks on redirects. That means that you are responsible for implementing CSP and mixed-content-blocking. This *might* be fixed if you make sure to use the page's loadgroup. But I'm not sure if that's possible to do in e10s since the loadgroup lives in the child process.

I'm probably forgetting some security policies as well. Redirect handling is particularly complicated to get right since we can't use the nsIContentPolicy mechanism there.

The other thing that you'll have to handle correctly is private browsing. We're also working on adding cookie jars to gecko which means that you need to make sure to use the correct cookie jar when creating the network request.

I know that the favicon code does some security checks, and I think that it grabs private-browsing information. But does it do all of the above?


The goal is to replace all of the above complexity with instead providing enough information when creating a channel that necko can take care of doing all security checks. By providing the information listed here at [1] (the same one can be passed to ioservice.newChannel*) necko will eventually be able to take on the responsibility of doing security checks.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#287
Can we serialize principals to strings, and if we can, how verbose are they, and would it make sense to embed the principal into these moz-anno URIs so that:

http://www.mozilla.org/somepage.html with favicon http://www.mozilla.org/favicon.ico

and

http://www.somewhere-totally-different.org/somepage.html with favicon http://www.mozilla.org/favicon.ico

get different URIs in the favicon storage ?


On the other hand, it seems like the point of the service is caching image data that was fetched and providing it again for use in browser UI without requiring connections to the sites the image data came from.

Jonas, forgive my ignorance, but do any of the security policies you named actually apply to favicons? And would they still apply if the party serving the favicon would not see a load (because we load the image from the DB and not from the original URI) ?
Flags: needinfo?(jonas)
Here are a couple examples explaining why favicon security checks are important.

Assume a user has set the pref to block mixed display content.  The user goes to https://democracy.com.  https://democracy.com has a favicon at http://democracy-images.com that is not in the user's cache.  If we use system principal as the loadingPrincipal for favicons, the favicon request for http://democracy-images.com will bypass security checks (like mixed content blocker) and the browser will make the HTTP network request.  Now any eavesdropper knows that the user with a specific IP was reading about democracy.

Assume a different user visits http://adult-blog.com.  http://adult-blog.com has an inappropriate favicon from http://images.com.  The browser caches the inappropriate image.  The user then visits https://mozilla.com.  https://mozilla.com has a CSP policy that states that images can only come from https://mozilla.com.  However, because of an XSS vulnerability on the site or a developer mistake/compromise, a favicon from http://images.com is included on the page.  If we use system principal as the loadingPrincipal for favicons, the favicon request for http://images.com will bypass security checks (in this case, CSP and mixed content blocker) and the inappropriate image will be pulled from the cache and displayed on mozilla.com, damaging its reputation and brand.
Flags: needinfo?(tanvi)
Comment on attachment 8603536 [details] [diff] [review]
bug_1119386_docshell.patch

contentChild->SendCopyFavicon is kind of hackish. We should do the principal checks on child side. But, fine, for now.
Attachment #8603536 - Flags: review?(bugs) → review+
Comment on attachment 8603537 [details] [diff] [review]
bug_1119386_dom.patch

Oh, forgot that IPC::Principal -> nsIPrincipal works that way.
Attachment #8603537 - Flags: review?(bugs) → review+
(In reply to :Gijs Kruitbosch from comment #19)
> Can we serialize principals to strings, and if we can, how verbose are they,
> and would it make sense to embed the principal into these moz-anno URIs so
> that:
> 
> http://www.mozilla.org/somepage.html with favicon
> http://www.mozilla.org/favicon.ico
> 
> and
> 
> http://www.somewhere-totally-different.org/somepage.html with favicon
> http://www.mozilla.org/favicon.ico
> 
> get different URIs in the favicon storage ?

I don't think we need to make it quite this complicated. But I need to better understand how the favicon code does its caching. Lets chat on vidyo.
Flags: needinfo?(jonas)
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1166887
Depends on: 1166891
Gijs, Marco, Jonas, Christoph and I met today to discuss how to proceed on this bug.  Notes and followups here https://etherpad.mozilla.org/auumgvVpgA.  Thanks!
Comment on attachment 8603532 [details] [diff] [review]
bug_1119386_toolkit.patch

Review of attachment 8603532 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/AsyncFaviconHelpers.h
@@ +12,5 @@
>  #include "nsIInterfaceRequestor.h"
>  #include "nsIStreamListener.h"
>  #include "nsThreadUtils.h"
>  
> + class nsIPrincipal;

nit: leading whitespace

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +833,2 @@
>          PlacesUtils.favicons.setAndFetchFaviconForPage(aPageURI, aIconURI,
> +                                                       Services.scriptSecurityManager.getSystemPrincipal(),

I think this can remain system principal assuming we do the checks about not putting data in the DB that doesn't pass proper checks.

I don't know how hard it is to construct a principal here based on just the URI (and/or how useful it'll be because there'll be no referrer policy or CSP or other stuff to actually go on, besides the URI).

I also don't expect it'll really matter for this usecase, but we do need to pass something, obviously. :-)

@@ +864,5 @@
>      // bookmark is visited again later, the URI and data will be fetched.
>      PlacesUtils.favicons.replaceFaviconDataFromDataURL(faviconURI, aData);
> +    // ckerschb: what's the right principal here? should we create one from the aPageURI?
> +    PlacesUtils.favicons.setAndFetchFaviconForPage(aPageURI, faviconURI,
> +                                                   Services.scriptSecurityManager.getSystemPrincipal(),

Same as above.

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +478,5 @@
>              // Create a fake faviconURI to use (FIXME: bug 523932)
>              let faviconURI = NetUtil.newURI("fake-favicon-uri:" + aData.uri);
>              PlacesUtils.favicons.replaceFaviconDataFromDataURL(
>                faviconURI, aData.icon, 0);
> +            // ckerschb: what's the right principal here?

We can do the same here as in the BookmarkHTMLUtils case.

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +55,5 @@
>     * @see nsIFaviconDataCallback in nsIFaviconService.idl.
>     */
>    void setAndFetchFaviconForPage(in nsIURI aPageURI,
>                                   in nsIURI aFaviconURI,
> +                                 in nsIPrincipal aLoadingPrincipal,

As noted in the meeting, this should be the last param. It's a little messy because there's an optional param already there, so the principal should probably be optional too, and default to system principal (which is sadfaces, but there we are).

If you feel like being amazing you could make the params be nsISupports and have the implementation accept either the callback and the principal in either order.

@@ +127,5 @@
>     *         Thrown if the favicon is overbloated and won't be saved to the db.
>     */
>    void replaceFaviconDataFromDataURL(in nsIURI aFaviconURI,
>                                       in AString aDataURL,
> +                                     in nsIPrincipal aLoadingPrincipal,

Same here, I expect.

::: toolkit/components/places/nsAnnoProtocolHandler.cpp
@@ +47,5 @@
>    nsresult rv = NS_NewURI(getter_AddRefs(defaultIconURI),
>                            NS_LITERAL_CSTRING(FAVICON_DEFAULT_URL));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  return NS_NewChannelInternal(aChannel, defaultIconURI, aLoadInfo);

As discussed in the meeting, the original channel is normally going to be the XUL file that requested the link, so it might be worth a comment about that so that we don't get the security checks wrong in the future. :-)

@@ +363,3 @@
>  
>    rv = faviconService->GetFaviconDataAsync(aAnnotationURI, callback);
> +  NS_ENSURE_SUCCESS(rv, GetDefaultIcon(aLoadInfo, _channel));

I will have to admit that I do not understand this code, which is why this isn't a review... but yeah.
Comment on attachment 8603535 [details] [diff] [review]
bug_1119386_browser.patch

Review of attachment 8603535 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +880,2 @@
>                this.mFaviconService.setAndFetchFaviconForPage(browser.currentURI,
> +                                                             aURI, loadingPrincipal, false,

So this is fine, but I agree that ideally we should not do this until the load of the icon in the tab itself succeeded, and use the dep bug to enforce principal-based security there. We can in principle do this separately from these changes, though, keeping the extant security checks that prevent the URI even ending up in XUL-land when we get told there's a new/different icon for a page.

::: browser/components/feeds/FeedWriter.js
@@ +1389,5 @@
>                                           .QueryInterface(Ci.nsIDocShell)
>                                           .QueryInterface(Ci.nsILoadContext)
>                                           .usePrivateBrowsing;
> +    this._faviconService.setAndFetchFaviconForPage(readerURI, faviconURI,
> +      this._feedPrincipal, false,

Marco pointed out that this is not going to work correctly; we're the people asking for this icon, not the feed, as it's the icon of the feed reader. There should now be a separate bug on file to just rip out this code.
(I'm going to assume I don't need to look at the tests for now)
Flags: needinfo?(gijskruitbosch+bugs)
See Also: → 1167259
Attached patch bug_1119386_toolkit.patch (obsolete) — Splinter Review
So, here we go: I incorporated the suggestions from Gijs and also what we have discussed in the meeting the other day. I rebased all the patches, except the test-cases, which need some more attention actually (I will do that soonish).

One thing however doesn't make me feel that comfortable, which is defaulting to the systemPrincipal in case no principal is provided for the optional last argument to setAndFetchFaviconForPage. Since loadingPrincipal *must* be an optional argument at the end of setAndFetchFaviconForPage because there is already one optional argument so that we don't break any addons. For now I am using the nullPrincipal and we have to re-evaluate what we do in such a case.
Attachment #8603532 - Attachment is obsolete: true
Attachment #8603535 - Attachment is obsolete: true
Attachment #8603536 - Attachment is obsolete: true
Attachment #8603537 - Attachment is obsolete: true
Attachment #8603551 - Attachment is obsolete: true
Attached patch bug_1119386_browser.patch (obsolete) — Splinter Review
Bill, from an e10s point of view, are we going to run into any trouble or can we safely use 'browser.contentPrincipal' within tabbrowser.xml or do we need to hand a 'principal' argument to setIcon()?
Flags: needinfo?(wmccloskey)
Attached patch bug_1119386_docshell.patch (obsolete) — Splinter Review
Just rebased; carrying over r+ from smaug.
Attachment #8609188 - Flags: review+
Attached patch bug_1119386_dom.patch (obsolete) — Splinter Review
Just rebased; carrying over r+ from smaug.
Attachment #8609189 - Flags: review+
Mak, I forgot to mention that since we are going to eliminate code within
> browser/components/feeds/FeedWriter.js
we could use the nullPrincipal as an interim solution.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #32)
> Mak, I forgot to mention that since we are going to eliminate code within
> > browser/components/feeds/FeedWriter.js
> we could use the nullPrincipal as an interim solution.

sure, or just fix the removal bug, it should be trivial.
(In reply to Marco Bonardo [::mak] from comment #33)
> > we could use the nullPrincipal as an interim solution.
> 
> sure, or just fix the removal bug, it should be trivial.

famous last words :-)
Comment on attachment 8609182 [details] [diff] [review]
bug_1119386_toolkit.patch

Review of attachment 8609182 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsFaviconService.cpp
@@ +237,5 @@
> +  // if no loadingPrincipal is provided, we default to the nullPrincipal to make
> +  // sure security checks fail and we don't allow to perform a network request.
> +  nsCOMPtr<nsIPrincipal> loadingPrincipal = aLoadingPrincipal ?
> +                                            aLoadingPrincipal :
> +                                            new nsNullPrincipal();

Just discussed that with Tanvi in person: we are going to add an NS_ASSERTION that the optional loadingPrincipal argument is set (so we catch all of the callsites within Gecko) but are going to default to the systemPrincipal. That way, addons would still work in release builds.

@@ +345,5 @@
> +  // sure security checks fail and we don't allow to perform a network request.
> +  nsCOMPtr<nsIPrincipal> loadingPrincipal = aLoadingPrincipal ?
> +                                            aLoadingPrincipal :
> +                                            new nsNullPrincipal();
> +  NS_ENSURE_TRUE(loadingPrincipal, NS_ERROR_FAILURE);

same here
MOZ_ASSERT is even better. nobody cares about NS_ASSERTIONs :)
It occurs to me just now that one alternative to enabling setting a .loadingPrincipal on a <xul:image>, would be to write some JS code which manually creates a channel and does the load and stores the result in a Blob. Then the XUL code could display that blob using a blob:-uri, and we could store the blob in the places database without hitting the network again.

This might also make it easier to fix the favicon handling in fennec.

Totally up to you guys though!
Attached patch bug_1119386_toolkit.patch (obsolete) — Splinter Review
MOZ_ASSERT() it is :-)
Attachment #8609182 - Attachment is obsolete: true
Attached patch bug_1119386_toolkit_tests.patch (obsolete) — Splinter Review
I also rebased all of the tests - here we go.
Attachment #8603534 - Attachment is obsolete: true
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #29)
> Created attachment 8609187 [details] [diff] [review]
> bug_1119386_browser.patch
> 
> Bill, from an e10s point of view, are we going to run into any trouble or
> can we safely use 'browser.contentPrincipal' within tabbrowser.xml or do we
> need to hand a 'principal' argument to setIcon()?

Using contentPrincipal should work in most cases, but I see some problems. The contentPrincipal is sent up from an onLocationChange listener in the child process. Then it gets cached on the <browser> element until the next onLocationChange.

In swapBrowsersAndCloseOther, we call setIcon on aOurTab before we've set up its contentPrincipal (which happens in _swapBrowserDocShells). So that one is incorrect.

In oop-browser-crashed, we call setIcon and I think contentPrincipal will the principal for about:tabcrashed. But we're using a favicon from the crashed page, so that one is also incorrect.

The useDefaultIcon caller seems okay since it happens in onStateChange with STATE_STOP, which I think happens after onLocationChange.

The most frequent caller is probably the Link:SetIcon message. That one happens when DOMLinkAdded fires in the child process. I'm guessing that's after onLocationChange, so it should be correct.

I'll leave it up to you to decide how important these problems are. Perhaps you could add an optional parameter to setIcon and only pass in principals for swapBrowsersAndCloseOther and oop-browser-crashed. Otherwise we would default to contentPrincipal. Not sure it matters too much though. The two problematic cases are ones where we want to apply a pre-existing icon to a page that already had it.
Flags: needinfo?(wmccloskey)
Just rebased the patch; carrying over r+ from smaug.
Attachment #8609188 - Attachment is obsolete: true
Attachment #8684995 - Flags: review+
Just rebased the patch; carrying over r+ from smaug.
Attachment #8609189 - Attachment is obsolete: true
Attachment #8684996 - Flags: review+
Gijs, unfortunately this bug hasn't got much attention in a long time. Since we are getting closer to having all callsites converted from using AsyncOpen() to AsyncOpen2() this bug becomes more important again. Anyway, I rebased the first two patches (dom, docshell) that already had an r+ from smaug.

a) I remember we had a meeting about how to handle this bug a long time ago, but I can't recall exactly what we agreed on. If I missed something, please just clear the review flag and let me know. What I remember though is that for the toolkit patch we 'just' have to make the principal argument optional for setAndFetchFaviconForPage() and replaceFaviconDataFromDataURL() so we don't break addons. We also agreed to fall back to using the systemPrincipal, but assert that a principal is passed from within gecko code.

b) The patch that needs more attention is the 'browser' patch within this bug. I think we have to address the concerns within Comment 40, or was there something else we have to take care of?
Attachment #8609559 - Attachment is obsolete: true
Attachment #8685002 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8685002 [details] [diff] [review]
bug_1119386_part4_favico_toolkit.patch

Review of attachment 8685002 [details] [diff] [review]:
-----------------------------------------------------------------

Marco is a better reviewer here.

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +840,5 @@
>      if (aIconURI) {
>        if (aIconURI.schemeIs("chrome")) {
> +        PlacesUtils.favicons.setAndFetchFaviconForPage(aPageURI, aIconURI, false,
> +                                                       PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE, null,
> +                                                       Services.scriptSecurityManager.getSystemPrincipal());

Can't we create a principal from the page URI? Same question for the other cases here & in BookmarkJSONUtils. We should probably sanitize and not allow JS or data or file: URIs though.

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +49,5 @@
>     * @param aCallback
>     *        Once we're done setting and/or fetching the favicon, we invoke this
>     *        callback.
> +   * @param aLoadingPrincipal
> +   *        Principal of the page whose favicon is being set.

Nit: both of these should document what we'll do if the argument is omitted.

::: toolkit/components/places/nsFaviconService.cpp
@@ +230,5 @@
> +  // let's default to the systemPrincipal if no loadingPrincipal is provided
> +  // so addons not providing a loadingPrincipal do not break in release builds.
> +  nsCOMPtr<nsIPrincipal> loadingPrincipal = aLoadingPrincipal;
> +  MOZ_ASSERT(loadingPrincipal, "please provide aLoadingPrincipal for this favicon");
> +  if (!loadingPrincipal) {

I'd really like to warn in the console so add-on users are also incentivized to do this. Needinfo :jorge to see if we can add validation to encourage people to have enough arguments here.

@@ +339,5 @@
> +  // let's default to the systemPrincipal if no loadingPrincipal is provided
> +  // so addons not providing a loadingPrincipal do not break in release builds.
> +  nsCOMPtr<nsIPrincipal> loadingPrincipal = aLoadingPrincipal;
> +  MOZ_ASSERT(loadingPrincipal, "please provide aLoadingPrincipal for this favicon");
> +  if (!loadingPrincipal) {

ditto
Attachment #8685002 - Flags: review?(mak77)
Attachment #8685002 - Flags: review?(gijskruitbosch+bugs)
Attachment #8685002 - Flags: feedback+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #43)
> Created attachment 8685002 [details] [diff] [review]
> bug_1119386_part4_favico_toolkit.patch
> 
> Gijs, unfortunately this bug hasn't got much attention in a long time. Since
> we are getting closer to having all callsites converted from using
> AsyncOpen() to AsyncOpen2() this bug becomes more important again. Anyway, I
> rebased the first two patches (dom, docshell) that already had an r+ from
> smaug.
> 
> a) I remember we had a meeting about how to handle this bug a long time ago,
> but I can't recall exactly what we agreed on. If I missed something, please
> just clear the review flag and let me know.

I don't remember.

>  What I remember though is that
> for the toolkit patch we 'just' have to make the principal argument optional
> for setAndFetchFaviconForPage() and replaceFaviconDataFromDataURL() so we
> don't break addons. We also agreed to fall back to using the
> systemPrincipal, but assert that a principal is passed from within gecko
> code.
> 
> b) The patch that needs more attention is the 'browser' patch within this
> bug. I think we have to address the concerns within Comment 40, or was there
> something else we have to take care of?

I don't remember this either. :-(
Tanvi just found these old notes on an etherpad which might be helpful:

*Action item[chris]: in tabbrowser.xml use browser.contentPrincipal in current patch.  but does that work in e10s?  contentPrincipal is not a cpow.  Alternative is we could look at the callsite of section and send the principal from there.  Christoph to ask e10s folks.

* Action item[chris]: change idl so that it is at the end of the parameter list. Christoph to do this.

* Action Item[gijs]: Gijs to review the code and check for places we are setting the wrong loadingPrincipal.  

* Action item [gijs]: get the principal to the xul element.  Gijs can try and help with this.
http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsImageBoxFrame.cpp#208
nsContentUtils::LoadImage.  gets the node principal from the xul element. Gijs to file a bug and make it a dependency of https://bugzilla.mozilla.org/show_bug.cgi?id=1119386

Feedreader.  rss icon? 
go to planet.mozilla.  choose to subscribe to an rss.  and choose an application (instead of bookmarks) like yahoo.  has an rss generic icon.  this code path is probably not being called - 
http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/FeedWriter.js#1378
we shoudl probably get rid of this code.

* Action Item[mak]: remove http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/FeedWriter.js#1378.  Marco to file a bug for it and make it a dependency for https://bugzilla.mozilla.org/show_bug.cgi?id=1119386
Here's a follow-up on the favicon issue. I think there are two changes needed:

* In swapBrowsersAndCloseOther, you want to pass otherBrowser.contentPrincipal into setIcon here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2519
...and then use that as the principal.

* For the main favicon case, you can send the principal as part of the message here:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentLinkHandler.jsm#111
I think it should be content.document.nodePrincipal or something like that. Then pass it to setIcon here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3431
Blocks: 1196013
Hey Bill, thanks for the info in Comment 47. I think that is all we need for this bug to work. I don't know what to do about components/feeds/FeedWriter.js though, I think that code is deprecated but I am not sure. Don't know if there is a better principal available which we could pass as the loadingPrincipal.
Attachment #8609187 - Attachment is obsolete: true
Attachment #8685711 - Flags: review?(wmccloskey)
@Marco: I incorporated the suggestions from Gijs from Comment 44. I had to update the patch though because we have to ProxyRelase the Principal within AsyncFaviconHelpers.cpp. Since that happens in the base class (AsyncFaviconHelperBase) for callbacks I think it makes sense to do the same thing for the principal.

The things that are important to look at:
* Are we passing the right loadingPrincipal on all the callsites? Gijs suggested to create a principal from the URI within BookmarkHTMLUtils and also BookmarkJSONUtils. I am not sure, I also don't know if that is a good idea in terms of originAttributes which we are about to add. I think we have to be careful.



@Jorge: Please look at Gijs suggestion underneath:
(In reply to :Gijs Kruitbosch from comment #44)
> I'd really like to warn in the console so add-on users are also incentivized
> to do this. Needinfo :jorge to see if we can add validation to encourage
> people to have enough arguments here.
I left two 'TODO' notes in the code, which I would update if you let me know that that makes sense. Otherwise let me know how we can encourage addon authors to pass the right amount of arguments.
Attachment #8685002 - Attachment is obsolete: true
Attachment #8685002 - Flags: review?(mak77)
Flags: needinfo?(jorge)
Attachment #8685715 - Flags: review?(mak77)
Can you give me more context? What is it we want developers to do differently?
Flags: needinfo?(jorge) → needinfo?(gijskruitbosch+bugs)
(In reply to Jorge Villalobos [:jorgev] from comment #50)
> Can you give me more context? What is it we want developers to do
> differently?

We would like to encourage addon authors to pass the optional argument 'aLoadingPrincipal' [1] for
* setAndFetchFaviconForPage, and also for
* replaceFaviconDataFromDataURL

What is the best way to accomplish that? If addon authors don't pass that optional argument, then we will default to using the systemPrincipal which will then bypass security checks.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8685715&action=diff
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jorge)
Status: NEW → ASSIGNED
If it's something they'll *need* to do after this bug is resolved, then I can just add the addon-compat flag to his bug so we can include it in the compatibility blog post and possibly the automatic compatibility validation we do for every release.

If it's something we want to encourage right away, then a JS console message is a good way to give developers a heads up.

We can do both if it makes sense, of course.
Flags: needinfo?(jorge)
Comment on attachment 8685711 [details] [diff] [review]
bug_1119386_part3_favico_browser.patch

Review of attachment 8685711 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good, but I just noticed this caller:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#778
I'm not sure what to do with that one. I guess we could try to serialize the principal, but that seems a bit crazy.
Attachment #8685711 - Flags: review?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #53)
> Comment on attachment 8685711 [details] [diff] [review]
> bug_1119386_part3_favico_browser.patch
> 
> Review of attachment 8685711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good, but I just noticed this caller:
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/SessionStore.jsm#778
> I'm not sure what to do with that one. I guess we could try to serialize the
> principal, but that seems a bit crazy.

Mhm, could we fall back to using browser.contentPrincipal if 'aLoadingPrinicpal' is empty? Would that make sense at all? How hard would it be to serialize the principal? Are we talking about the JS equivalent of IPC::Principal? 

Also there are a bunch of tests we have to update that use setIcon:
http://mxr.mozilla.org/mozilla-central/search?string=er.setIcon%28
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #54)
> Mhm, could we fall back to using browser.contentPrincipal if
> 'aLoadingPrinicpal' is empty? Would that make sense at all? How hard would
> it be to serialize the principal? Are we talking about the JS equivalent of
> IPC::Principal? 

We can't use contentPrincipal here since we haven't started loading the page yet. This is the situation where the user clicks "Restore Previous Session". We show a bunch of tabs with their titles and favicons, but they don't get loaded until the user clicks on them.

It looks like we do have some code that serializes and deserializes principals for session store:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionHistory.jsm?force=1#217
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionHistory.jsm?force=1#364
It's only used for the owner field of an nsISHEntry (which is always an nsIPrincipal, as far as I can tell). Maybe we could split this code into a Utils.jsm in sessionstore and use it more generally for serializing any principal.

Still, this is going to be a bit tricky to get right. I suspect in almost all cases the favicon will be cached here, since you've already visited the page. Is it necessary to have the right principal in that case?

> Also there are a bunch of tests we have to update that use setIcon:
> http://mxr.mozilla.org/mozilla-central/search?string=er.setIcon%28

Yeah, I dunno. If they pass, do we care?
Bill, not sure if this is a review or rather a needinfo request. Anyway, I think serializing the principal for the session-restore case is overkill. We have two options in my opinion:
a) We default to the nullPrincipal in case 'aLoadingPrincipal' is null within
   tabbrowser.xml. Defaulting to the nullPrincipal allows accessing the cache
   but doesn't actually allow to perform a network request.
b) We could default to the systemPrincipal in case 'aLoadingPrincipal' is null, which
   would also allow to fetch the icon over the network but is also security critical. Not
   sure how likely an attacker can trigger that case though :-)

I am not sure about the tradeoffs and what cases we do care more. Jonas leans towards the systemPrincipal approach. What do you think?
Attachment #8685711 - Attachment is obsolete: true
Attachment #8686779 - Flags: review?(wmccloskey)
Comment on attachment 8686779 [details] [diff] [review]
bug_1119386_part3_favico_browser.patch

Review of attachment 8686779 [details] [diff] [review]:
-----------------------------------------------------------------

You should probably ask :mak about this since I don't know how likely it is that a page being restored from session restore won't have its favicon cached. If it's very unlikely to happen, then I think the null principal is fine. If it's likely, then we should use the system principal.

Otherwise everything looks fine.

::: browser/base/content/browser.js
@@ +3437,5 @@
>      }
>    },
>  
> +  setIcon: function(aBrowser, aURL, aLoadingPrincipal) {
> +

No extra line here.

::: browser/base/content/tabbrowser.xml
@@ +864,3 @@
>                  aURI = makeURI(aURI);
> +              }
> +              // if aLoadingPrinicpal is null we default to the nullPrincipal

Please capitalize the first letter of sentences in comments.

::: browser/components/sessionstore/SessionStore.jsm
@@ +774,5 @@
>          }
>  
>          // Restore the tab icon.
>          if ("image" in tabData) {
> +          // using null as the loadingPrincipal will default to the nullPrincipal

Capitalize this too please.
Attachment #8686779 - Flags: review?(wmccloskey) → review+
Comment on attachment 8686779 [details] [diff] [review]
bug_1119386_part3_favico_browser.patch

Review of attachment 8686779 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bill McCloskey (:billm) from comment #57)
> You should probably ask :mak about this since I don't know how likely it is
> that a page being restored from session restore won't have its favicon
> cached. If it's very unlikely to happen, then I think the null principal is
> fine. If it's likely, then we should use the system principal.

Marco, any preferences on how we handle that case?
Attachment #8686779 - Flags: review?(mak77)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #58)
> (In reply to Bill McCloskey (:billm) from comment #57)
> > You should probably ask :mak about this since I don't know how likely it is
> > that a page being restored from session restore won't have its favicon
> > cached.

When history is enabled we always try try cache icons, so I'd expect in most cases we should have cached data from the previous session.
So far I can think of 2 cases where history may be missing:
1. Clear history on shutdown. I don't recall if it's possible to enable clear history on shutdown but still restore the last session. If possible this case will be common for any user with that setting.
2. places.sqlite was found corrupt on startup and thrown away. I think this is rare enough that we can ignore it.

The other thing is that some urls are not accepted into history, thus their favicons alway come from the network. In CanAddURI you can find the list:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#1128
Out of that list the more likely having a favicon is "about:", but I think all important about: pages have local favicons coming from chrome:// urls, and I think those are unaffected by this null principal restriction on network, right?
Comment on attachment 8686779 [details] [diff] [review]
bug_1119386_part3_favico_browser.patch

Review of attachment 8686779 [details] [diff] [review]:
-----------------------------------------------------------------

I assume this request was mostly about the session question.
the changes look fine and Bill already reviewed them. Just a couple nits:

::: browser/base/content/tabbrowser.xml
@@ +865,5 @@
> +              }
> +              // if aLoadingPrinicpal is null we default to the nullPrincipal
> +              // which will block to fetch the icon from the network but allows
> +              // to get it from the cache.
> +              var loadingPrincipal = aLoadingPrincipal

nit: apart from "var browser..." most of the code here uses let, would be nice to uniform that.

@@ +872,5 @@
> +              this.mFaviconService.setAndFetchFaviconForPage(
> +                browser.currentURI, aURI, false,
> +                PrivateBrowsingUtils.isWindowPrivate(window) ?
> +                  this.mFaviconService.FAVICON_LOAD_PRIVATE :
> +                  this.mFaviconService.FAVICON_LOAD_NON_PRIVATE,

while touching this, would you mind moving this pb stuff into a local var (something like loadType) to improve readability of the API call?
Attachment #8686779 - Flags: review?(mak77)
Comment on attachment 8685715 [details] [diff] [review]
bug_1119386_part4_favico_toolkit.patch

Review of attachment 8685715 [details] [diff] [review]:
-----------------------------------------------------------------

I think it's mostly good and what we discussed before. Still it's doing too many changes to favicons code that doesn't need a principal at all.
I'm suggesting a different approach that I hope will do.

::: toolkit/components/places/AsyncFaviconHelpers.h
@@ +100,5 @@
>  class AsyncFaviconHelperBase : public nsRunnable
>  {
>  protected:
> +  explicit AsyncFaviconHelperBase(nsCOMPtr<nsIFaviconDataCallback>& aCallback,
> +                                  nsCOMPtr<nsIPrincipal>& aLoadingPrincipal);

I think I don't like this much cause it forces us to add the loading principal as an argument to every class, even if they don't need it at all.

The only class that needs the principal is AsyncFetchAndSetIconFromNetwork that is invoked off the main-thread by AsyncFetchAndSetIconForPage::Run.
That means the only 2 classes needing a reference to the principal are AsyncFetchAndSetIconForPage and AsyncFetchAndSetIconFromNetwork.
The AsyncFetchAndSetIconForPage constructor is always invoked on main-thread (you can add a MOZ_ASSERT there), thus it's a good place to touch the principal ptr.

I'd suggest to use a nsMainThreadPtrHandle<nsIPrincipal> mLoadingPrincipal in AsyncFetchAndSetIconForPage.
mLoadingPrincipal = new nsMainThreadPtrHolder<nsIXPConnectJSObjectHolder>(aLoadingPrincipal) in the constructor.
At that point you have something that you can pass around and will be freed on the main-thread.

::: toolkit/components/places/mozIAsyncFavicons.idl
@@ +51,5 @@
>     *        callback.
> +   * @param aLoadingPrincipal
> +   *        Principal of the page whose favicon is being set. If this argument
> +   *        is omitted, the loadingPrincipal defaults to the systemPrincipal
> +   *        and will bypass security checks!

I think we should have a deadline after which this will change from systemPrincipal to nullPrincipal, it may be worth filing a bug to change that like 2 or 3 versions from now, after the AMO hooks have notified add-on authors about the change.
Also, imo it reads as a positive thing "it will bypass security checks!", while it should sound like something very wrong to do,like "the loadingPrincipal defaults to the systemPrincipal and we cannot guarantee security checks anymore."

@@ +125,5 @@
>     *        Until this time, we won't try to load it again.
> +   * @param aLoadingPrincipal
> +   *        Principal of the page whose favicon is being set. If this argument
> +   *        is omitted, the loadingPrincipal defaults to the systemPrincipal
> +   *        and will bypass security checks!

ditto

::: toolkit/components/places/nsFaviconService.cpp
@@ +227,5 @@
>        return NS_OK;
>    }
>  
> +  // let's default to the systemPrincipal if no loadingPrincipal is provided
> +  // so addons not providing a loadingPrincipal do not break in release builds.

if you file the bug to convert this to a nullPrincipal, please add here a TODO (bug XYZ): prefix

@@ +337,5 @@
>    rv = ioService->GetProtocolHandler("data", getter_AddRefs(protocolHandler));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // let's default to the systemPrincipal if no loadingPrincipal is provided
> +  // so addons not providing a loadingPrincipal do not break in release builds.

ditto
Attachment #8685715 - Flags: review?(mak77)
ehr s/nsIXPConnectJSObjectHolder/nsIPrincipal/
Incorporated the requested changes/nits from billm and mak. Carrying over r+.

(In reply to Marco Bonardo [::mak] from comment #59)
> Out of that list the more likely having a favicon is "about:", but I think
> all important about: pages have local favicons coming from chrome:// urls,
> and I think those are unaffected by this null principal restriction on
> network, right?

As long as those chrome URLs are accessible to content, then we are good. Let's check that once we get to it though. Within bug 1196013 we will convert favicons to use AsyncOpen2. I'll cc you on the bug and we can have another look there. For now, we should stick with the nullPrinipal.
Attachment #8686779 - Attachment is obsolete: true
Attachment #8690955 - Flags: review+
Blocks: 1227289
Marco, thanks for the feedback. I totally agree with your comments and we should rather use nsMainThreadPtrHolder to proxy the release. As explained in Comment 49, the main reason I added the loadingPrincipal to the base class was to make it fit the current setup. Either way, using nsMainThreadPtrHolder is the way nicer solution.

(In reply to Marco Bonardo [::mak] from comment #61)
> The AsyncFetchAndSetIconForPage constructor is always invoked on main-thread
> (you can add a MOZ_ASSERT there), thus it's a good place to touch the
> principal ptr.

No need actually to add that assertion since AsyncFetchAndSetIconForPage::start already bails out early if not called on the main thread.

> ::: toolkit/components/places/mozIAsyncFavicons.idl
> @@ +51,5 @@
> >     *        callback.
> > +   * @param aLoadingPrincipal
> > +   *        Principal of the page whose favicon is being set. If this argument
> > +   *        is omitted, the loadingPrincipal defaults to the systemPrincipal
> > +   *        and will bypass security checks!
> 
> I think we should have a deadline after which this will change from
> systemPrincipal to nullPrincipal, it may be worth filing a bug to change
> that like 2 or 3 versions from now, after the AMO hooks have notified add-on
> authors about the change.
> Also, imo it reads as a positive thing "it will bypass security checks!",
> while it should sound like something very wrong to do,like "the
> loadingPrincipal defaults to the systemPrincipal and we cannot guarantee
> security checks anymore."

That is a good point. I made it more scary for consumers using your phrase. I also filed Bug 1227289, CC'ed you and will consult Jorge when we should flip that switch and default to the nullPrincipal rather than the systemPrincipal.


For this bug I think it's fine to just set the addon-compat flag as suggested by Jorge within Comment 52. No need to pollute the console with an additonal message.
Attachment #8685715 - Attachment is obsolete: true
Attachment #8691052 - Flags: review?(mak77)
I also updated all the tests to make it fit the new setup. All of them are passing locally, but haven't pushed everything to TRY yet.
Attachment #8609560 - Attachment is obsolete: true
Attachment #8691053 - Flags: review?(mak77)
Keywords: addon-compat
Comment on attachment 8691052 [details] [diff] [review]
bug_1119386_part4_favico_toolkit.patch

Review of attachment 8691052 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following fixed

::: toolkit/components/places/AsyncFaviconHelpers.h
@@ +200,5 @@
>    AsyncFetchAndSetIconFromNetwork(IconData& aIcon,
>                                    PageData& aPage,
>                                    bool aFaviconLoadPrivate,
> +                                  nsCOMPtr<nsIFaviconDataCallback>& aCallback,
> +                                  nsMainThreadPtrHandle<nsIPrincipal> aLoadingPrincipal);

I think this should be a "const nsMainThreadPtrHandle<nsIPrincipal>&" since we want to pass a const reference, not a copy.
Attachment #8691052 - Flags: review?(mak77) → review+
Comment on attachment 8691053 [details] [diff] [review]
bug_1119386_part4_favico_toolkit_tests.patch

Review of attachment 8691053 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me, if Try is happy, I am happy.

::: toolkit/components/places/tests/unit/test_history_observer.js
@@ +160,5 @@
>    // we use an URI representing the smallest possible PNG file.
>    PlacesUtils.favicons.setAndFetchFaviconForPage(testuri, SMALLPNG_DATA_URI,
>                                                   false,
>                                                   PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE,
> +                                                 null, 

trailing space
Attachment #8691053 - Flags: review?(mak77) → review+
Comment on attachment 8690955 [details] [diff] [review]
bug_1119386_part3_favico_browser.patch

Review of attachment 8690955 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +867,5 @@
> +              // which will block to fetch the icon from the network but allows
> +              // to get it from the cache.
> +              let loadingPrincipal = aLoadingPrincipal
> +                ? aLoadingPrincipal
> +                : Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal);

Marco, I have been chatting with Jonas yesterday and we came to the conclusion that falling back to the systemPrincipal is the better option here. There are two main reasons:

1) At the moment (like in current release) we are using the systemPrincipal for all favicon loads. Within this patch we are passing the right loadingPrincipal wherever we can. That seems like a major improvement overall. The session restore case is definitely somehow special and (as discussed) serializing the principal for that case seems overkill for the purpose of this bug.

2) If we are passing the nullPrincipal we can not access chrome:// URLs unless we would pass the ALLOW_CHROME flag to security checks, which is not something we want to do for IMAGE loads. I initially thought we could allow the nullPrincipal to access chrome://, but Jonas told me that is not a good idea. Because that would also allow content to access chrome:// which seems scary.

Anyway, I would update that part of the code to fall back to the systemPrincipal rather than the nullPrincipal if you are fine with it.
Attachment #8690955 - Flags: feedback?(mak77)
Comment on attachment 8690955 [details] [diff] [review]
bug_1119386_part3_favico_browser.patch

Review of attachment 8690955 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, WFM.
Attachment #8690955 - Flags: feedback?(mak77) → feedback+
Depends on: 1228445
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #68)
> Anyway, I would update that part of the code to fall back to the
> systemPrincipal rather than the nullPrincipal if you are fine with it.

Can you file a new bug with the places we are falling back to system when we ideally shouldn't be (like system restore)?  Maybe we can fix it one day if, for example, someone decides to serialize the principal for other reasons.
(In reply to Tanvi Vyas [:tanvi] from comment #72)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #68)
> > Anyway, I would update that part of the code to fall back to the
> > systemPrincipal rather than the nullPrincipal if you are fine with it.
> 
> Can you file a new bug with the places we are falling back to system when we
> ideally shouldn't be (like system restore)?  Maybe we can fix it one day if,
> for example, someone decides to serialize the principal for other reasons.

In fact, I already did, it's Bug 1227289. I left a comment with the bugnumber in the code. In roughly ~18 weeks we should be able to land that followup and default to the nullPrincipal instead of the systemprincipal in cases addons still don't pass the optional argument. I will consult Jorge before I make the change.
Blocks: 333811
You need to log in before you can comment on or make changes to this bug.