Make sure that favicons are properly segregated based on usercontextID. If not, fix the code. If so, write a test. +++ This bug was initially created as a clone of Bug #1264564 +++

Kamil or Paul, please test to see that requests for favicons in different user context only have the cookies associated with the user context they are in. A reference to the code would also be great if you can find one.

I just tested this, and this seems like an issue. From testing it looks like the cookie that is sent for favicon request is any cookie set outside containers (ie not in work,personal etc, just a regular tab) I guess that means the request is made from a context with no userContext set. Digging for the code now...

This probably depends on or is at least related to bug 1227289.

Actually Christoph informs that we aren't using asyncopen2 yet for loading favicons (bug 1196013). This may or may not fix this bug, it depends on exactly how the cookie is attached. I haven't been able to track this down yet.

Whoever is going to fix that bug, *please* make sure to add a test to our codebase because historically favicons have always been an attack vector within browsers.

Thanks for going through this Paul! Someone in IRC mentioned that the favicon service might be running from XUL? This could be similar to the awesome bar autocomplete issue that's also running using XUL via bug # 1244340 comment #11?

Ok Im closer, but still looking. In Tabbrowser.xml [1] the correct principal seems to be passed to the setIcon function. Breaking at in this function while in the "banking" container I see: XPCWrappedNative_NoHelper { URI: XPCWrappedNative_NoHelper, isNullPrincipal: false, cspJSON: "{}", jarPrefix: "", originAttributes: Object, origin: "https://misuse.co^userContextId=2", originNoSuffix: "https://misuse.co", originSuffix: "^userContextId=2", baseDomain: "misuse.co", appStatus: 0 } as the loading principal, right before: this.mFaviconService.setAndFetchFaviconForPage( browser.currentURI, aURI, false, loadType, null, loadingPrincipal); } Still digging... [1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#856

Im still stuck here - I'll try Bill tomorrow. From talking with Christoph, there definitely is a problem relating to session restore see [1] and also this comment in the code [2]. But in the regular load case, from "setIcon" in the tabbrowser.xml, I traced the code down into nsFaviconService::SetAndFetchFaviconForPage then into the RunnableMethod(?) AsyncFetchAndSetIconForPage, which has a task called AsyncFetchAndSetIconForPage::FetchFromNetwork() [4]. In this method,NS_NewChannel is called, with the mLoadingPrincipal from above (afaict). I don't know what's next? I assume if you pass the correct principal to the channel then it should have the correct cookie, but maybe there is code here that isn't origin aware? I'll escalate to folks in US when they wake up. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1119386#c55 [2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#869 [3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsFaviconService.cpp#209 [4] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/FaviconHelpers.cpp#402 https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/FaviconHelpers.cpp#419

Though I very much wonder if the patch in bug 1196013 will fix this initial problem: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1196013&attachment=8746679 There is then the first issue (session restore etc).

The reason why the favicon does not obey originAttributes is that the loading channel of the favicon does not have a loadcontext. This causes NS_GetOriginAttributes()[1] fails and the nsCookieService will utilize the default user context instead. But I don't know why the channel of loading favicon doesn't have a loadcontext. I will keep digging on it. [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#1265

After better understanding the code here, it appears that it is not the channel doesn't have the load context. It is the notification callbacks doesn't have the load context, and the notification callback is AsyncFetchAndSetIconForPage[1] for the loading channel of the favicon. BTW, the code which calls NS_GetOriginAttributes() is located at nsCookieService.cpp[2]. [1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/FaviconHelpers.h#101 [2] https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#1906

When loading a page, there will be two loads of the favicon that I could see from the browser console. One load is triggered by the PlacesUIUtils.loadFavicon()[1], and another one is originated from the xul:image[2]. For the first case, I will submit a patch to fix. But for the second one, it is hard to be resolved because the xul:image will use the system princial to load the image, and the system principal follows the default originAttributes. The bug 1166891 is targeting this issue, so I will focus on the first case here. [1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#868 [2] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#6192

Jonas, could you help on review this patch? And Should I find a necko peer to look this as well?

Comment on attachment 8758173 [details] [diff] [review] Make the NS_GetOriginAttributes() acquires originAttributes from the loadInfo when loadContext is not available. Review of attachment 8758173 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsNetUtil.cpp @@ +1270,2 @@ > NS_QueryNotificationCallbacks(aChannel, loadContext); > + loadInfo = aChannel->GetLoadInfo(); You can squeeze these into one line nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo(); @@ +1276,5 @@ > + if (loadContext) { > + DocShellOriginAttributes doa; > + loadContext->GetOriginAttributes(doa); > + aAttributes.InheritFromDocShellToNecko(doa); > + } else { You have to add a comment when this is the case? What side effects might this change cause? Jonas, can't we always rely on the loadInfo instead of the loadContext? Originattributes should match, right?

> > Jonas, can't we always rely on the loadInfo instead of the loadContext? > Originattributes should match, right? I've tried to use loadInfo in NS_GetOriginAttributes just 1~2 weeks ago and the try looks horrible.... https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a086bcd6c23 But I didn't spend time to figure out how much work to fix that yet.

(In reply to Yoshi Huang[:allstars.chh] from comment #15) > I've tried to use loadInfo in NS_GetOriginAttributes just 1~2 weeks ago and > the try looks horrible.... > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a086bcd6c23 > > But I didn't spend time to figure out how much work to fix that yet. Ah, that's good to know - thanks. Then I suppose we should special case the favicon case here (which is what the patch already does), but it also might return loadinfo originattributes in other cases than favicons in case there is no loadcontext.

Christoph, thanks for your feedback. This patch has made changes according to what you said.

Valentin, could you help on reviewing this patch? Thanks.

Fix a problem that the test will timeout in non-e10s mode.

Rebase from central.

Comment on attachment 8760065 [details] [diff] [review] Part 2: Add a test case to verify that the loading of favicon obey originAttributes. Review of attachment 8760065 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, r=me

Is this ready to land?

Update commit message.

Try looks good.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2238c0c342a9 Part 1: Make the NS_GetOriginAttributes() acquires originAttributes from the loadInfo when loadContext is not available. r=valentin https://hg.mozilla.org/integration/mozilla-inbound/rev/3a5b19ff2f41 Part 2: Add a test case to verify that the loading of favicon obey originAttributes. r=ckerschb

https://hg.mozilla.org/mozilla-central/rev/2238c0c342a9 https://hg.mozilla.org/mozilla-central/rev/3a5b19ff2f41

Do we care about favicon requests for the test pilot? It would be better if it worked, but doesnt seem like the end of the world if it doesn't and not sure if its worth the risk of uplift?

(In reply to Paul Theriault [:pauljt] from comment #29) > Do we care about favicon requests for the test pilot? It would be better if > it worked, but doesnt seem like the end of the world if it doesn't and not > sure if its worth the risk of uplift? Talked with Tim offline. It's better not to uplift this bug at this moment because it does not completely resolve the favicon issue. There are two follow-up bugs, which I added them to the See Also field.