When loading favicon through the XUL:image, it will not obey the originAttributes because XUL:image loads resources with the system principal. We should make this kind of loading follows the correct originAttributes.

Comment on attachment 8766908 [details] [diff] [review] Bug1277803.patch This doesn't look right. It seems like aURI is the uri where the favicon is coming from. Not the URI of the page that is loading the favicon. For the loadingPrincipal, you want the principal of the node that initiated the favicon load.

Maybe the URL of the tab window loading the favicon would suffice?

(In reply to James Andreou from comment #3) > Maybe the URL of the tab window loading the favicon would suffice? This is really hard to get right and I doubt there is a way to actually get that right without putting in quite some engineering effort. See numerous discussions starting here: https://bugzilla.mozilla.org/show_bug.cgi?id=1119386#c55 I think if you really want to get that right you would have to serialize the principal for the session-restore case.

When do favicon requests go through xul:image? Do all favicon requests go through this? Is this only used on session restore?

(In reply to Tanvi Vyas - out until 8/2 [:tanvi] from comment #5) > When do favicon requests go through xul:image? At first place, the loading through xul:image is triggered from [1]. This will set the url of the favicon into the attribute 'image' of the tab. And then, here [2] is the xul:image which actually loads the image. This xul:image will take the 'image' attributes as its src to load the favicon that is done by the xbl:inherits="src=image". So I think if we could make this xul:image to use the correct principal, then we can resolve this issue. > Do all favicon requests go through this? Is this only used on session restore? I think all favicon requests go through this part. [1] http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#881 [2] http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#6257

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4) > (In reply to James Andreou from comment #3) > > Maybe the URL of the tab window loading the favicon would suffice? > > This is really hard to get right and I doubt there is a way to actually get > that right without putting in quite some engineering effort. See numerous > discussions starting here: > https://bugzilla.mozilla.org/show_bug.cgi?id=1119386#c55 > > I think if you really want to get that right you would have to serialize the > principal for the session-restore case. Christoph, passing around the serialized loadingPrincipal seems like the best option. I’ve written the approach I’m planning to take below. Does this seem sufficient? 1. Serialize the principal passed to the PlacesUI loading code [1] 2. Set the loadingPrincipal attribute on the tab object in tabbrowser.xml in the same way image is set here [2]. 3. Move the loadingPrincipal to the XUL image object here [3]. 4. Grab the loadingPrincipal attribute off of the XUL image element as we do for the image src [4], deserialize it, and use it in the subsequent favicon load. 5. Ensure the serialized loadingPrincipal is added to the tabData object in SessionStore and deserialize it before passing to setIcon() [5]. [1] http://searchfox.org/mozilla-central/rev/5b35b60ada19621c84ecab78998416d8b86a8254/browser/base/content/tabbrowser.xml#872 [2] http://searchfox.org/mozilla-central/rev/5b35b60ada19621c84ecab78998416d8b86a8254/browser/base/content/tabbrowser.xml#881 [3] http://searchfox.org/mozilla-central/rev/5b35b60ada19621c84ecab78998416d8b86a8254/browser/base/content/tabbrowser.xml#6254 [4] http://searchfox.org/mozilla-central/rev/5b35b60ada19621c84ecab78998416d8b86a8254/layout/xul/nsImageBoxFrame.cpp#224 [5] http://searchfox.org/mozilla-central/rev/5b35b60ada19621c84ecab78998416d8b86a8254/browser/components/sessionstore/SessionStore.jsm#818

Hey Steven, first, thanks for taking on the work and trying to fix this bug. I don't think that you need to serialize anything once you are in tabbrowser.xml [1]. I suppose the only time we would have to serialize/deserialize the principal would be within sessionstore [2] where we pass null as the loadingPrincipal at the moment. But also in this case I would imagine that we load the favicon from the cache since we must have already loaded it once (so it ends up in the cache in the first place). So even in this case I would think that using the SystemPrincipal is fine. If you wanna fix the SessionStore problem, I think all you would have to do is take the code for serializing the principal from here [3] and move it into some Utils.jsm file which would then be called from various places within the codebase. Now it gets interesting: I honestly was thinking that we pass the right loadingPrincipal in all other cases, because I know I updated all the callsites to .setIcon(). And I think I am partially right :-) I took a closer look once again and visited my webpage and got the following values when loading the favicon: doContentSecurityCheck { channelURI: http://www.christophkerschbaumer.com/favicon.ico loadingPrincipal: SystemPrincipal triggeringPrincipal: SystemPrincipal contentPolicyType: 37 securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS, SEC_ALLOW_CHROME, initalSecurityChecksDone: no enforceSecurity: no } So we are using the SystemPrincipal as the loadingPrincipal which is obviously not correct. So I traced down that callsite (see stacktrace underneath) and I think (I am not entirely sure though) that we are just passing the wrong principal. If you update this line [4] to use doc->NodePrincipal() then I get the following results: doContentSecurityCheck { channelURI: http://www.christophkerschbaumer.com/favicon.ico loadingPrincipal: http://www.christophkerschbaumer.com/ triggeringPrincipal: http://www.christophkerschbaumer.com/ contentPolicyType: 37 securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS, SEC_ALLOW_CHROME, initalSecurityChecksDone: no enforceSecurity: no } Now, this seems correct to me (at least for my testcase). At the moment I don't know what other implications that change might have. I think the right path forward is to create a mochitest which loads a favicon (check for the right loadingPrincipal, triggeringPrincipal, OriginAttributes, etc). I think this easy patch should also fix the OriginAttributes problem. Let's give it a try :-) [1] https://hg.mozilla.org/mozilla-central/diff/02507513d65b/browser/base/content/tabbrowser.xml#l1.12 [2] https://hg.mozilla.org/mozilla-central/diff/02507513d65b/browser/components/sessionstore/SessionStore.jsm#l1.17 [3] https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionHistory.jsm#228 [4] https://hg.mozilla.org/mozilla-central/annotate/720b5d2c84d5b253d4dfde4897e13384dc97a46a/layout/xul/nsImageBoxFrame.cpp#l236 0x00007fffe87db083 in nsContentSecurityManager::doContentSecurityCheck (aChannel=0x7fffb8007058, aInAndOutListener=...) at /home/ckerschb/moz/mc/dom/security/nsContentSecurityManager.cpp:502 502 MOZ_ASSERT(false); (gdb) bt #0 0x00007fffe87db083 in nsContentSecurityManager::doContentSecurityCheck (aChannel=0x7fffb8007058, aInAndOutListener=...) at /home/ckerschb/moz/mc/dom/security/nsContentSecurityManager.cpp:502 #1 0x00007fffe59c1f59 in mozilla::net::nsHttpChannel::AsyncOpen2 (this=0x7fffb8007000, aListener=0x7fffc0ae7518) at /home/ckerschb/moz/mc/netwerk/protocol/http/nsHttpChannel.cpp:5559 #2 0x00007fffe9cefb23 in mozilla::places::AsyncFetchAndSetIconForPage::FetchFromNetwork (this=0x7fffc0ae7500) at /home/ckerschb/moz/mc/toolkit/components/places/FaviconHelpers.cpp:444 #3 0x00007fffe9d86af9 in mozilla::detail::RunnableMethodArguments<>::applyImpl<mozilla::places::AsyncFetchAndSetIconForPage, nsresult (mozilla::places::AsyncFetchAndSetIconForPage::*)()>(mozilla::places::AsyncFetchAndSetIconForPage*, nsresult (mozilla::places::AsyncFetchAndSetIconForPage::*)(), mozilla::Tuple<>&, mozilla::IndexSequence<>) (o=0x7fffc0ae7500, m=(nsresult (mozilla::places::AsyncFetchAndSetIconForPage::*)(mozilla::places::AsyncFetchAndSetIconForPage * const)) 0x7fffe9cef58e <mozilla::places::AsyncFetchAndSetIconForPage::FetchFromNetwork()>, args=...) at /home/ckerschb/moz/mc-obj-ff-dbg/dist/include/nsThreadUtils.h:729 #4 0x00007fffe9d869c8 in mozilla::detail::RunnableMethodArguments<>::apply<mozilla::places::AsyncFetchAndSetIconForPage, nsresult (mozilla::places::AsyncFetchAndSetIconForPage::*)()>(mozilla::places::AsyncFetchAndSetIconForPage*, nsresult (mozilla::places::AsyncFetchAndSetIconForPage::*)()) (this=0x7fffbc25da30, o=0x7fffc0ae7500, m=(nsresult (mozilla::places::AsyncFetchAndSetIconForPage::*)(mozilla::places::AsyncFetchAndSetIconForPage * const)) 0x7fffe9cef58e <mozilla::places::AsyncFetchAndSetIconForPage::FetchFromNetwork()>) at /home/ckerschb/moz/mc-obj-ff-dbg/dist/include/nsThreadUtils.h:736 #5 0x00007fffe9d867bf in mozilla::detail::RunnableMethodImpl<nsresult (mozilla::places::AsyncFetchAndSetIconForPage::*)(), true, false>::Run (this=0x7fffbc25da00) at /home/ckerschb/moz/mc-obj-ff-dbg/dist/include/nsThreadUtils.h:764 #6 0x00007fffe540d3bb in nsThread::ProcessNextEvent (this=0x7ffff6b7b500, aMayWait=false, aResult=0x7fffffffc6bf) at /home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:1058 #7 0x00007fffe5476093 in NS_ProcessNextEvent (aThread=0x7ffff6b7b500, aMayWait=false) at /home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:290 #8 0x00007fffe5bc9f29 in mozilla::ipc::MessagePump::Run (this=0x7fffe1d93040, aDelegate=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:96 #9 0x00007fffe5b3a42d in MessageLoop::RunInternal (this=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:232 #10 0x00007fffe5b3a3c2 in MessageLoop::RunHandler (this=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:225 #11 0x00007fffe5b3a39b in MessageLoop::Run (this=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:205 #12 0x00007fffe8e97afc in nsBaseAppShell::Run (this=0x7fffd34152b0) at /home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:156 #13 0x00007fffe9e45015 in nsAppStartup::Run (this=0x7fffd340b290) at /home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:284 #14 0x00007fffe9f05d0a in XREMain::XRE_mainRun (this=0x7fffffffcab0) at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4279 #15 0x00007fffe9f0629f in XREMain::XRE_main (this=0x7fffffffcab0, argc=1, argv=0x7fffffffdea8, aAppData=0x7fffffffccd0) at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4406 #16 0x00007fffe9f0656f in XRE_main (argc=1, argv=0x7fffffffdea8, aAppData=0x7fffffffccd0, aFlags=0) at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4497 #17 0x00000000004058c1 in do_main (argc=1, argv=0x7fffffffdea8, envp=0x7fffffffdeb8, xreDirectory=0x7ffff6b59780) at /home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:247 #18 0x0000000000405c28 in main (argc=1, argv=0x7fffffffdea8, envp=0x7fffffffdeb8) at /home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:380

I filed Bug 1294866 for the SessionStore fix.

WIP test patch. I would like to add additional test cases: * private windows * tabs which have different non-default originAttributes set * check cached favicon loads

WIP serialization approach. Need to verify cache loads works as expected.

There are two favicon requests which occur as a result of single tabbrowser::setIcon() call. The first occurs from AsyncFetchAndSetIconForPage [1] from PlacesUIUtils.loadFavicon [2]. This is the load captured in the stack trace in your comment. The second occurs as part of the standard XUL image Element loading and uses nsContentUtils::LoadImage [3]. This bug is attempting to fix the latter. Your suggestion didn’t work for me. My understanding is that mContent is a XULElement and doc is the XULDocument that element is defined in, not the tab content’s Document. Thus, doc->NodePrincipal() will still be the SystemPrincipal. This is what I observe when I test the change with my test patch, and makes sense as the request is occurring in the parent process. Serializing the principal can work, we just need to make sure not to use the XULElement loadingDocument to set up the new channel [4]. It seems reasonable to set up the channel only using the loadingPrincipal when the loadingDocument’s principal is SystemPrincipal and loadingPrincipal != SystemPrincipal (see my patch), but it’s not clear to me if this will break other image loads. I still need to verify that all of the checks that happen when inspecting the cache [5] will work correctly when the loadingDocument is a XULDocument with SystemPrincipal and the loadingPrincipal is the serialized content’s principal. I’d also like to add a few new test cases that open private windows and tabs with different origin attributes. [1] http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/toolkit/components/places/FaviconHelpers.cpp#352 [2] http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/browser/base/content/tabbrowser.xml#875 [3] http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/layout/xul/nsImageBoxFrame.cpp#236 [4] http://searchfox.org/mozilla-central/rev/f433f0dd7158d7bfc4c4607161fc6baa88b5a9f4/image/imgLoader.cpp#775 [5] http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/image/imgLoader.cpp#2118-2150

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

Wait, just to make sure I understand everything correctly here. You are running a chrome test, right? In that case SystemPrincipal is the expected loadingPrincipal. What happens if you create a plain mochitest? I would expect the principal to be the document's principal and not the systemPrincipal.

As we discussed on IRC yesterday, the test is written in chrome but the principal is the contentPrincipal for a browser opened in a new window [1] so that shouldn't matter. Using the WIP mochitest in Bug 1297156 [2] we see the same results, i.e. two requests, one using SystemPrincipal and one using the correct principal. Note that if we want to support Bug 1297156 my WIP patch is insufficient. I serialize the principal using principal.origin and deserialize with createCodebasePrincipal(origin), so CSP is dropped. [1] http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/browser/base/content/tabbrowser.xml#4207 [2] https://bug1297156.bmoattachments.org/attachment.cgi?id=8783865

Today is Steve's last day, so he won't be able to finish this bug. Try failures are probably because we are missing a null check at line 882 here: https://bugzilla.mozilla.org/attachment.cgi?id=8783101&action=diff#a/browser/base/content/tabbrowser.xml_sec2 We should probably land bug 1294866 first, and then update 1277803-serialization.patch based on the final patches in bug 1294866. This may also need a couple more tests with containers... trying to load favicons with different OriginAttributes. Examples from Steve: (In reply to Steven Englehardt [:englehardt] from comment #10) > Created attachment 8783100 [details] [diff] [review] > 1277803-test.patch > > WIP test patch. I would like to add additional test cases: > * private windows > * tabs which have different non-default originAttributes set > * check cached favicon loads

There is still a problem that we will hit the assertion [1] because of the originAttributes of the requesting node, the XUL image, is different from the one of the loadingPrincipal. The principal of the XUL image is the system principal which has default originAttributes, so if any non-default originAttributes are given in the loadingPrincipal then It will hit the assertion. [1] http://searchfox.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#2462

Per discussion with Yoshi, we should skip the comparison between loadInfo and loadContext for such favicon loading. To make this happen, I proposed that we use a channel that its triggering principal is the principal of the XUL image and its loading principal is the contentPrincipal. And we can skip the comparison in the NS_CompareLoadInfoAndLoadContext() if the triggering principal of the loadInfo is system principal and the loading principal is not system principal. But I don't know that is this a valid approach to use triggering principal and loading principal in this way. Christoph, what do you think?

(In reply to Tim Huang[:timhuang] from comment #18) > But I don't know that is this a valid approach to use triggering principal > and loading principal in this way. Christoph, what do you think? Well, just to make sure I understand the approach correctly, something like: if (nsContentUtils::IsSystemPrincipal(triggeringPrincipal) && !nsContenUtils::IsSystemPrincipal(loadingPrincipal) { // skip assertion } That sounds risky and potentially we skip assertions also for some other cases where we don't want to skip it. Could we do something else? E.g. create a new internal contentPolicy type, like e.g. TYPE_INTERNAL_IMAGE_FAVICON or something so we can skip assertions only precisely for that case?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19) > (In reply to Tim Huang[:timhuang] from comment #18) > > But I don't know that is this a valid approach to use triggering principal > > and loading principal in this way. Christoph, what do you think? > > Well, just to make sure I understand the approach correctly, something like: > > if (nsContentUtils::IsSystemPrincipal(triggeringPrincipal) && > !nsContenUtils::IsSystemPrincipal(loadingPrincipal) > { > // skip assertion > } Yes, you are right. But I think it will apply your idea and change to something like: if (loadInfo->InternalContentPolicyType() == nsIContentPolicy::TYPE_TYPE_INTERNAL_IMAGE_FAVICON) { // skip assertion } > > That sounds risky and potentially we skip assertions also for some other > cases where we don't want to skip it. Could we do something else? E.g. > create a new internal contentPolicy type, like e.g. > TYPE_INTERNAL_IMAGE_FAVICON or something so we can skip assertions only > precisely for that case? Yes, I think this is a good plan. I will do this way. Thanks.

Per discussion with Yoshi, the Channel for loading the favicon should have the contentPrincipal as its triggering principal and SystemPrincipal as its loading principal, but the loadInfo of this loading channel should use the correct originAttributes which have come from the content. And we still need to skip the comparison between loadInfo and loadContext since they will mismatch in this case, so it requires new content policy type TYPE_INTERNAL_IMAGE_FAVICON for making us skipping comparison accordingly.

Comment on attachment 8787586 [details] [diff] [review] Part 1 : Add a new ContentPolicy TYPE_INTERNAL_IMAGE_FAVICON for indicating a favicon loading. Review of attachment 8787586 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I suppose we should also update the following tests to use TYPE_IMAGE_FAVICON instead of TYPE_IMAGE: * browser/components/contextualidentity/test/browser/browser_favicon.js * toolkit/components/places/tests/browser/browser_favicon_setAndFetchFaviconForPage.js * toolkit/components/places/tests/favicons/test_moz-anno_favicon_mime_type.js * toolkit/components/places/tests/favicons/test_page-icon_protocol.js r=me with those nits addressed. ::: dom/base/nsContentPolicyUtils.h @@ +132,5 @@ > CASE_RETURN( TYPE_INTERNAL_IMAGE ); > CASE_RETURN( TYPE_INTERNAL_IMAGE_PRELOAD ); > CASE_RETURN( TYPE_INTERNAL_STYLESHEET ); > CASE_RETURN( TYPE_INTERNAL_STYLESHEET_PRELOAD ); > + CASE_RETURN( TYPE_INTERNAL_IMAGE_FAVICON ); please move it up right under TYPE_INTERNAL_IMAGE_PRELOAD so people looking at the file immediately see that there are basically 3 different image types.

Comment on attachment 8787587 [details] [diff] [review] Part 2 : Make favicons loaded through XUL:image use the correct principal. Review of attachment 8787587 [details] [diff] [review]: ----------------------------------------------------------------- Looks good so far, just a few nits! ::: browser/base/content/tabbrowser.xml @@ +860,5 @@ > <body> > <![CDATA[ > let browser = this.getBrowserForTab(aTab); > + let serhelper = Cc["@mozilla.org/network/serialization-helper;1"] > + .getService(Ci.nsISerializationHelper); nit: indendation and please rename to something like serializationHelper; @@ +880,2 @@ > aTab.setAttribute("image", sizedIconUrl); > + aTab.setAttribute("loadingprincipal", serhelper.serializeToString(loadingPrincipal)); nit: 80 chars lines please ::: image/imgLoader.cpp @@ +746,5 @@ > + aURI, > + loadInfo, > + nullptr, // loadGroup > + callbacks, > + aLoadFlags); I think I would prefer if we would keep the NewChannelWithTriggeringPrincipal(), because that remains the same, right? And then only within: if (aPolicyType == nsIContentPolicy::TYPE_INTERNAL_IMAGE_FAVICON) { we do nsCOMPtr<nsILoadInfo> loadInfo = (*aResult)->GetLoadInfo(); loadInfo->SetOriginAttributes(neckoAttrs); ::: layout/xul/nsImageBoxFrame.cpp @@ +234,5 @@ > + nsAutoString imageLoadingPrincipal; > + mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::loadingprincipal, > + imageLoadingPrincipal); > + if (!imageLoadingPrincipal.IsEmpty()) { > + nsCOMPtr<nsISupports> iSupports; nit: can you rename iSupports to something like serializedPrincipal; @@ +238,5 @@ > + nsCOMPtr<nsISupports> iSupports; > + nsresult rv = NS_DeserializeObject(NS_ConvertUTF16toUTF8(imageLoadingPrincipal), > + getter_AddRefs(iSupports)); > + if (NS_SUCCEEDED(rv)) { > + loadingPrincipal = do_QueryInterface(iSupports); What happens if the do_QueryInterface does not return a valid principal? In other words loadingPrincipal = nullptr after that line? Should we still use TYPE_INTERNAL_IAMGE_FAVICON in that case? I think you can drop the nsresult and just make sure the do_QueryInterface returns a valid principal and then check |if (loadingPrincipal) { assign TYPE_INTERNAL_IMAGE_FAVICON }

Addressing review comments.

Addressing review comments. Thanks, christoph. And flag billm for review.

This test case tests principals and cookies for favicon loading in different userContextIds.

Comment on attachment 8788822 [details] [diff] [review] Part 4 : A test to verify the loadingPrincipal of favicon loads. Review of attachment 8788822 [details] [diff] [review]: ----------------------------------------------------------------- r+, please address my nits and thanks for working on this! ::: dom/security/test/general/chrome.ini @@ +1,3 @@ > +[DEFAULT] > +support-files = > + favicon_bug1277803.ico did you forget to add that file? or is just splinter weired in not showing me the *.ico file? ::: dom/security/test/general/test_bug1277803.xul @@ +62,5 @@ > + // as its triggering principal. > + ok(triggeringPrincipal.equals(expectedPrincipal), > + "Correct triggeringPrincipal for favicon from XUL."); > + ok(loadingPrincipal.equals(systemPrincipal), > + "Correct loadingPrincipal URI for favicon from XUL."); I suppose you could remove that ok() since you are already within this branch. Same in the |else if|, but ultimately I leave that up to you. @@ +75,5 @@ > + "Correct loadingPrincipal URI for favicon from Places."); > + requestPlaces = true; > + } else { > + ok(false, "An unexpected favicon request.") > + } I suppose the sequence is always the same here right? First we load the icon using a sytemPrincipal and second we load the icon using a codeBasePrincipal, right? If so, could we slightly modify the test to: if (loadingPrincipalIsSystem() { ... requestXUL = true; return; } if (codeBasePrincipal() { ... ok (requestXUL, ...); requestPlaces = true; }

Comment on attachment 8787588 [details] [diff] [review] Part 3 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image. Review of attachment 8787588 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for late response. Took a while to go through the bug and the patches to understand what's going on here. It's always sensitive to mess with principals compare. This looks good to me. Please address the comments. Thanks. r=honzab ::: netwerk/base/nsNetUtil.cpp @@ +2411,5 @@ > if (isAboutPage) { > return NS_OK; > } > > + // We try to skip the favicon loading here. The favicon loading might be you try or you skip? @@ +2414,5 @@ > > + // We try to skip the favicon loading here. The favicon loading might be > + // triggered by the XUL image. For that case, the loadContext will have > + // default originAttributes since the XUL image uses SystemPrincipal, but > + // the loading principal will be the content prinicpal for favicon loading. and thus what? this comment doesn't fully state WHY you are pretending everything is OK for this case. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +2633,4 @@ > return shouldIntercept; > } > > void HttpBaseChannel::CheckPrivateBrowsing() The name of the method should be changed to AssertPrivateBrowsingId() or something. And should be debug-only? @@ +2646,4 @@ > DocShellOriginAttributes docShellAttrs; > loadContext->GetOriginAttributes(docShellAttrs); > MOZ_ASSERT(mLoadInfo->GetOriginAttributes().mPrivateBrowsingId == docShellAttrs.mPrivateBrowsingId, > "PrivateBrowsingId values are not the same between LoadInfo and LoadContext."); nit: please, when you are here, correct the indention of this block. or even better, since this is a single purpose method, maybe convert it to rather do early returns on negative conditions, like: if (!mLoadInfo) { return; } if (!loadContext) { return; } if (nsContentUtils::IsSystemPrincipal(mLoadInfo->LoadingPrincipal()) && mLoadInfo->InternalContentPolicyType() != nsIContentPolicy::TYPE_INTERNAL_IMAGE_FAVICON)) { return; } do the stuff...

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #32) > Comment on attachment 8788822 [details] [diff] [review] > Part 4 : A test to verify the loadingPrincipal of favicon loads. > > Review of attachment 8788822 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+, please address my nits and thanks for working on this! > Thanks, Christoph. > ::: dom/security/test/general/chrome.ini > @@ +1,3 @@ > > +[DEFAULT] > > +support-files = > > + favicon_bug1277803.ico > > did you forget to add that file? or is just splinter weired in not showing > me the *.ico file? > It's just the splinter doesn't show that file, because I can see this file in the details. > @@ +75,5 @@ > > + "Correct loadingPrincipal URI for favicon from Places."); > > + requestPlaces = true; > > + } else { > > + ok(false, "An unexpected favicon request.") > > + } > > I suppose the sequence is always the same here right? First we load the icon > using a sytemPrincipal and second we load the icon using a > codeBasePrincipal, right? If so, could we slightly modify the test to: > > if (loadingPrincipalIsSystem() { > ... > requestXUL = true; > return; > } > > if (codeBasePrincipal() { > ... > ok (requestXUL, ...); > requestPlaces = true; > } The sequence is not always the same here. I have tried modifying the test as you described and tested it with --run-until-failure. The result shows that the request from XUL comes first then the request from Places in most cases. However, occasionally, Places comes before XUL. Hence, we should just assume that they will not come in order here.

(In reply to Tim Huang[:timhuang] from comment #34) > The sequence is not always the same here. I have tried modifying the test as > you described and tested it with --run-until-failure. The result shows that > the request from XUL comes first then the request from Places in most cases. > However, occasionally, Places comes before XUL. Hence, we should just assume > that they will not come in order here. Thanks for testing. I was worried that that is the case. Either way, you can leave your testcase as is then.

Addressing review comments. Thanks, Honza.

Comment on attachment 8789254 [details] [diff] [review] Part 3 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image. Review of attachment 8789254 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpChannel.cpp @@ -5583,5 @@ > NS_CompareLoadInfoAndLoadContext(this); > > -#ifdef DEBUG > - CheckPrivateBrowsing(); > -#endif why did you remove the #ifdef DEBUG here?

Comment on attachment 8788825 [details] [diff] [review] Part 5 : Add a test case for favicon loading in different userContextIds. Review of attachment 8788825 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/originattributes/test/browser/browser_favicon_userContextId.js @@ +26,5 @@ > + var tools = SpecialPowers.Cc["@mozilla.org/image/tools;1"] > + .getService(SpecialPowers.Ci.imgITools); > + var imageCache = tools.getImgCacheForDocument(window.document); > + imageCache.clearCache(true); // true=chrome > + imageCache.clearCache(false); // false=content this is so so bad :/ true/false is super confusing. It's not your fault but we should improve this API.

(In reply to Honza Bambas (:mayhemer) from comment #37) > Comment on attachment 8789254 [details] [diff] [review] > Part 3 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the > request is the favicon loading from the XUL image. > > Review of attachment 8789254 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/protocol/http/nsHttpChannel.cpp > @@ -5583,5 @@ > > NS_CompareLoadInfoAndLoadContext(this); > > > > -#ifdef DEBUG > > - CheckPrivateBrowsing(); > > -#endif > > why did you remove the #ifdef DEBUG here? Because I move the '#ifdef DEBUG' into the HttpBaseChannel::AssertPrivateBrowsingId(). So I think the '#ifdef DEBUG' here is a duplicate work. Should I add them back?

Comment on attachment 8788812 [details] [diff] [review] Part 2 : Make favicons loaded through XUL:image use the correct principal. Review of attachment 8788812 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review for the tabbrowser changes. Someone else should review the imglib changes. ::: browser/base/content/tabbrowser.xml @@ +865,4 @@ > browser.mIconURL = aURI instanceof Ci.nsIURI ? aURI.spec : aURI; > + let loadingPrincipal = aLoadingPrincipal > + ? aLoadingPrincipal > + : Services.scriptSecurityManager.getSystemPrincipal(); I'm confused because this code seems duplicated with patch 2 in bug 1294866, which uses different attribute names and a different identifier for the serializer. So I don't quite follow how those patches are related. It seems like at least one of them is obsolete and/or superseded by the other. @@ +6270,5 @@ > <xul:image xbl:inherits="fadein,pinned,busy,progress,selected=visuallyselected" > class="tab-throbber" > role="presentation" > layer="true" /> > + <xul:image xbl:inherits="src=image,loadingprincipal,fadein,pinned,selected,visuallyselected,busy,crashed,sharing" The loading principal is specific to the icon, so it should be named something like "iconloadingprincipal" on the tab, and then inherited onto the image as loadingprincipal=iconloadingprincipal, to avoid confusion about this attribute saying anything about the loading principal of the page.

(In reply to Tim Huang[:timhuang] from comment #39) > Because I move the '#ifdef DEBUG' into the > HttpBaseChannel::AssertPrivateBrowsingId(). So I think the '#ifdef DEBUG' > here is a duplicate work. Should I add them back? I think the method should not exist (be even declared) when in non-debug build. It saves some small compilation time, and also make the code a bit smaller. And mainly makes clear on the method's purpose.

(In reply to :Gijs Kruitbosch from comment #40) > Comment on attachment 8788812 [details] [diff] [review] > Part 2 : Make favicons loaded through XUL:image use the correct principal. > > Review of attachment 8788812 [details] [diff] [review]: > ----------------------------------------------------------------- > > Clearing review for the tabbrowser changes. Someone else should review the > imglib changes. > > ::: browser/base/content/tabbrowser.xml > @@ +865,4 @@ > > browser.mIconURL = aURI instanceof Ci.nsIURI ? aURI.spec : aURI; > > + let loadingPrincipal = aLoadingPrincipal > > + ? aLoadingPrincipal > > + : Services.scriptSecurityManager.getSystemPrincipal(); > > I'm confused because this code seems duplicated with patch 2 in bug 1294866, > which uses different attribute names and a different identifier for the > serializer. So I don't quite follow how those patches are related. It seems > like at least one of them is obsolete and/or superseded by the other. > Actually, I was not sure about which bug will be landed first, this one or bug 1294866, so I made changes for both of them. The attribute here is different from the bug 1294866 with a different purpose that the attribute here is for favicon and the attribute of bug 1294866 is for session restore. But somehow, I think I could combine them into one. Right now, I believe the bug 1294866 will be landed first, so I will move the declaration of serializer to Bug 1294866, and the patch here will on top of that. Also, I will make changes of imgLib into another patch.

(In reply to Tim Huang[:timhuang] from comment #44) > Created attachment 8790578 [details] [diff] [review] > Part 2 : Make favicons loaded through XUL:image use the correct principal. This patch is based on the patch 2 of the Bug 1294866. And I split out the part of the imglib into another patch.

Updating commit message.

Comment on attachment 8790578 [details] [diff] [review] Part 2 : Make favicons loaded through XUL:image use the correct principal. Review of attachment 8790578 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, my use of "imglib" was very loose - I assumed nsImageBoxFrame and so on were all part of imglib as well. I can r+ this change to tabbrowser.xml, but someone else should review the nsImageBoxFrame + nsGkAtomList changes. I'll add :tn as a reviewer as I see you've asked him for review on the "real" imglib change.

Comment on attachment 8790587 [details] [diff] [review] Part 7 : Add a test case of favicon loading for first party isolation. Review of attachment 8790587 [details] [diff] [review]: ----------------------------------------------------------------- These cookie tests look good overall. See my suggested revisions below. It's good to be testing cookies. But I think, to be confident that favicons are correctly isolated, we should test two other things as well: (1) that favicon images are correctly isolated by first party in the image cache, and (2) that the expected first party domain can be retrieved from the favicon request's httpChannel. Tor Browser uses (2) to send the httpChannel over a Tor circuit associated with the first party. Can we add those tests here? ::: browser/components/originattributes/test/browser/browser_favicon_firstParty.js @@ +16,5 @@ > +const THIRD_PARTY_SITE = "http://" + THIRD_PARTY; > + > +const TEST_PAGE = "/browser/browser/components/originattributes/test/browser/" + > + "file_favicon.html"; > +const TEST_THIRD_PARTY_PATE = "/browser/browser/components/originattributes/test/browser/" + Should be TEST_THIRD_PARTY_PAGE, though party pâté sounds delicious. :) Also needs to be corrected below. @@ +37,5 @@ > +function FaviconObserver(aFirstPartyDomain, aExpectedCookie, aPageURI) { > + this.reset(aFirstPartyDomain, aExpectedCookie, aPageURI); > +} > + > +FaviconObserver.prototype = { One possible simplification is to create a function that accepts aFirstPartyDomain, aExpectedCookie, aPageURI, and returns a Promise that resolves when it is done. Just pass arguments along with faviconReqXUL, faviconReqPlaces, all as closures. Then faviconLoaded can be the return value. Something like function observeFavicon(aFirstPartyDomain, aExpectedCookie, aPageURI) { return new Promise(resolve => { let observer = { observer : function (aSubject, aTopic, aData) { Services.obs.removeObserver(observer, ...); ... resolve(); } }; Services.obs.addObserver(observer, ...); }); Then you don't need a reset() function, getter etc. @@ +46,5 @@ > + // channel. All requests for the favicon should contain the correct > + // firstPartyDomain. There are two requests for a favicon loading, one > + // from the Places library and one from the XUL image. The difference > + // of them is the loading principal. The Places will use the content > + // principal and the XUL image will use the system principal. I'm curious, why are Place and XUL different in this respect? Does it need to be this way, or would it be helpful if one of them were changed to be like the other? Also, I never understood why there are two requests for favicon loading -- I wonder if one can be removed. In any case, it's good to be testing both. @@ +61,5 @@ > + > + if (reqLoadInfo) { > + loadingPrincipal = reqLoadInfo.loadingPrincipal; > + triggeringPrincipal = reqLoadInfo.triggeringPrincipal; > + } Is it possible for reqLoadInfo to be null or undefined here? Then loadingPrincipal and triggeringPrincipal will also be undefined and I think errors will be thrown below. If it is not supposed to happen, then maybe fail here instead of later? @@ +116,5 @@ > + yield BrowserTestUtils.browserLoaded(browser); > + return {tab, browser}; > +} > + > +function* generateCookies() { Add a single argument here: generateCookies(/*bool*/ thirdParty) Then you can refactor to combine generateCookiesThirdParty with generateCookies. @@ +142,5 @@ > + > + return cookies; > +} > + > +function* assignCookiesUnderFirstParty(aURL, aFirstParty, aCookieValue){ Move this function definition above generateCookies so it's easier to find. @@ +195,5 @@ > + ]}); > +}); > + > +add_task(function* test_favicon_firstParty() { > + // Clear all image caches before running the test. If you refactor generateCookies, then you can use for (let thirdParty of [false, true]) { ... } and combine all three tasks into one function* ().

(In reply to Arthur Edelstein [:arthuredelstein] from comment #52) > Comment on attachment 8790587 [details] [diff] [review] > Part 7 : Add a test case of favicon loading for first party isolation. > > Review of attachment 8790587 [details] [diff] [review]: > ----------------------------------------------------------------- > Arthur, thanks for your review. > These cookie tests look good overall. See my suggested revisions below. > > It's good to be testing cookies. But I think, to be confident that favicons > are correctly isolated, we should test two other things as well: > (1) that favicon images are correctly isolated by first party in the image > cache, and I think I can add a test of image cache here for favicon loading. > (2) that the expected first party domain can be retrieved from the favicon > request's httpChannel. > Tor Browser uses (2) to send the httpChannel over a Tor circuit associated > with the first party. > I have tested the firstPartyDomain of originAttributes of the channel's loadInfo. Isn't that what you want? > Can we add those tests here? > > ::: > browser/components/originattributes/test/browser/browser_favicon_firstParty. > js > @@ +46,5 @@ > > + // channel. All requests for the favicon should contain the correct > > + // firstPartyDomain. There are two requests for a favicon loading, one > > + // from the Places library and one from the XUL image. The difference > > + // of them is the loading principal. The Places will use the content > > + // principal and the XUL image will use the system principal. > > I'm curious, why are Place and XUL different in this respect? Does it need > to be this way, or would it be helpful if one of them were changed to be > like the other? It's because the XUL uses the system principal for the current design. So the favicon request from XUL will use the system principal as its loading principal, but the request from Places uses the content principal. > Also, I never understood why there are two requests for > favicon loading -- I wonder if one can be removed. In any case, it's good to > be testing both. I know it's messy here. There is a bug for tackling this problem, Bug 1288054. Here, I think the best way is to make sure they are using correct originAttributes. > @@ +61,5 @@ > > + > > + if (reqLoadInfo) { > > + loadingPrincipal = reqLoadInfo.loadingPrincipal; > > + triggeringPrincipal = reqLoadInfo.triggeringPrincipal; > > + } > > Is it possible for reqLoadInfo to be null or undefined here? Then > loadingPrincipal and triggeringPrincipal will also be undefined and I think > errors will be thrown below. If it is not supposed to happen, then maybe > fail here instead of later? > I think the loadInfo won't be null here. So are you requesting something like: if (reqLoadInfo) { loadingPrincipal = reqLoadInfo.loadingPrincipal; triggeringPrincipal = reqLoadInfo.triggeringPrincipal; if (!loadingPrincipal || !triggeringPrincipal) { ok(false, "The loadingPrincipal and triggeringPrincipal should not be null."); return; } }

(In reply to Tim Huang[:timhuang] from comment #53) > I think I can add a test of image cache here for favicon loading. Thank you! > I have tested the firstPartyDomain of originAttributes of the channel's > loadInfo. Isn't that what you want? You have and it is. I'm sorry -- somehow I didn't read it correctly before! > > Also, I never understood why there are two requests for > > favicon loading -- I wonder if one can be removed. In any case, it's good to > > be testing both. > > I know it's messy here. There is a bug for tackling this problem, Bug > 1288054. Here, I think the best way is to make sure they are using correct > originAttributes. Agreed. > I think the loadInfo won't be null here. So are you requesting something > like: > > if (reqLoadInfo) { > loadingPrincipal = reqLoadInfo.loadingPrincipal; > triggeringPrincipal = reqLoadInfo.triggeringPrincipal; > > if (!loadingPrincipal || !triggeringPrincipal) { > ok(false, "The loadingPrincipal and triggeringPrincipal should not be > null."); > return; > } > } I was confused by the line if (reqLoadInfo) { because it seems to indicate that in the case where reqLoadInfo is null, we just skip the { } block and continue, when in fact we should immediately fail. Instead you could use something like: > if (!reqLoadInfo) { > ok(false, "reqLoadInfo should not be null.");\ > return; > } > loadingPrincipal = reqLoadInfo.loadingPrincipal; > triggeringPrincipal = reqLoadInfo.triggeringPrincipal; > if (!loadingPrincipal || !triggeringPrincipal) { > ok(false, "The loadingPrincipal and triggeringPrincipal should not be null."); > return; > }

It seems like that the favicon service of the places doesn't obey originAttributes in terms of the cache [1]. I think we should fix this here as well. Gijs, what do you think about this? [1] http://searchfox.org/mozilla-central/source/toolkit/components/places/FaviconHelpers.cpp#146

(In reply to Tim Huang[:timhuang] from comment #55) > It seems like that the favicon service of the places doesn't obey > originAttributes in terms of the cache [1]. I think we should fix this here > as well. > > Gijs, what do you think about this? > > [1] > http://searchfox.org/mozilla-central/source/toolkit/components/places/ > FaviconHelpers.cpp#146 I think it's probably better to use a followup for that at this stage. I also don't really know what you mean off-hand. IIRC we pass a principal to this code already and so it should use it if we actually do a network fetch. It may not use it if it retrieves data from cache, but that will only ever be data that will have been legitimately fetched at some point, and it won't hit any servers and so from that perspective no inforomation is leaked or anything. Given also that there aren't currently restrictions on cross-domain requests (e.g. if I'm foo.com my favicon is allowed to come from bar.com) I don't think fixing this needs to be a priority.

(In reply to :Gijs Kruitbosch from comment #56) > I think it's probably better to use a followup for that at this stage. I > also don't really know what you mean off-hand. IIRC we pass a principal to > this code already and so it should use it if we actually do a network fetch. > It may not use it if it retrieves data from cache, but that will only ever > be data that will have been legitimately fetched at some point, and it won't > hit any servers and so from that perspective no inforomation is leaked or > anything. Given also that there aren't currently restrictions on > cross-domain requests (e.g. if I'm foo.com my favicon is allowed to come > from bar.com) I don't think fixing this needs to be a priority. Yes, I will file a follow up bug for this. And it is true that we will not leak information if we fetch favicons from the cache. However, it also leaves a clue for allowing tracking an individual. For example, I visit a website in default container, the favicon will be loaded and cached here. And then, I visit the same site with a different container, the favicon request will not send out since the favicon has been cached. Which means the site will know that you are a repeat visitor.

(In reply to Tim Huang[:timhuang] from comment #57) > However, it also leaves a clue for allowing tracking an > individual. For example, I visit a website in default container, the favicon > will be loaded and cached here. And then, I visit the same site with a > different container, the favicon request will not send out since the favicon > has been cached. Which means the site will know that you are a repeat > visitor. What's your threat model here? This is a binary signal (request vs. no request). How would that allow correlating with a *specific* earlier visitor if there is *no* request? The favicon request will still happen for the xul:image, and that will likely never (permanently, x-ref bug 1301988) stop being the case, irrespective of repeat visits. De-duplicating the requests is the subject of bug 1288054. I don't think there's much value in focusing on avoiding the reuse of cached imagery beyond just fixing that bug.

(In reply to :Gijs Kruitbosch from comment #58) > What's your threat model here? This is a binary signal (request vs. no > request). How would that allow correlating with a *specific* earlier visitor > if there is *no* request? > The favicon along may not allow people to be tracked since it's only a binary signal just as you said. But the tracking could be possible if it combines with other factors, one possibility is the source IP address. Say, the website tracks the IP address for each user, then the website could identify an earlier visitor by combining favicon request and IP address. But, this kind of tracking could be diminished by using NAT or dynamic IP address. > The favicon request will still happen for the xul:image, and that will > likely never (permanently, x-ref bug 1301988) stop being the case, > irrespective of repeat visits. De-duplicating the requests is the subject of > bug 1288054. I don't think there's much value in focusing on avoiding the > reuse of cached imagery beyond just fixing that bug. You are right here, the xul:image will send the request even if the favicon be cached by Places. So the tracking here may not be a problem unless the website counts the number of favicon requests. Hence, I agree with you that this should be a low priority.

(In reply to Tim Huang[:timhuang] from comment #59) > (In reply to :Gijs Kruitbosch from comment #58) > > What's your threat model here? This is a binary signal (request vs. no > > request). How would that allow correlating with a *specific* earlier visitor > > if there is *no* request? > > The favicon along may not allow people to be tracked since it's only a > binary signal just as you said. If the favicon includes an Etag, or a unique Last-Modified header, then it's trivial to track users with a single favicon. Additionally, it's straightforward to load 100 favicons sequentially using JavaScript. So it's easy to imagine tracking individual users via favicons alone. Also, in Tor Browser, we want to be able request favicons over the same Tor circuit as the originating top-level page. Otherwise we are leaking what site the user is visiting over the wrong circuit. > > The favicon request will still happen for the xul:image, and that will > > likely never (permanently, x-ref bug 1301988) stop being the case, > > irrespective of repeat visits. De-duplicating the requests is the subject of > > bug 1288054. I don't think there's much value in focusing on avoiding the > > reuse of cached imagery beyond just fixing that bug. > > You are right here, the xul:image will send the request even if the favicon > be cached by Places. So the tracking here may not be a problem unless the > website counts the number of favicon requests. Hence, I agree with you that > this should be a low priority. In my view, isolating favicons by first party should have a similar priority to isolating any other cacheable content by first party (such images, scripts, HTML pages, etc.), because the tracking threat is very similar.

(In reply to Arthur Edelstein [:arthuredelstein] from comment #60) > (In reply to Tim Huang[:timhuang] from comment #59) > > (In reply to :Gijs Kruitbosch from comment #58) > > > What's your threat model here? This is a binary signal (request vs. no > > > request). How would that allow correlating with a *specific* earlier visitor > > > if there is *no* request? > > > > The favicon along may not allow people to be tracked since it's only a > > binary signal just as you said. > > If the favicon includes an Etag, or a unique Last-Modified header, then it's > trivial to track users with a single favicon. Additionally, it's > straightforward to load 100 favicons sequentially using JavaScript. So it's > easy to imagine tracking individual users via favicons alone. But the point that Tim was making is about favicons that are *not* loaded. There is no such data for those favicons. > In my view, isolating favicons by first party should have a similar priority > to isolating any other cacheable content by first party (such images, > scripts, HTML pages, etc.), because the tracking threat is very similar. An interesting point, but orthogonal to this bug.

(In reply to :Gijs Kruitbosch from comment #61) > (In reply to Arthur Edelstein [:arthuredelstein] from comment #60) > > (In reply to Tim Huang[:timhuang] from comment #59) > > > (In reply to :Gijs Kruitbosch from comment #58) > > > > What's your threat model here? This is a binary signal (request vs. no > > > > request). How would that allow correlating with a *specific* earlier visitor > > > > if there is *no* request? > > > > > > The favicon along may not allow people to be tracked since it's only a > > > binary signal just as you said. > > > > If the favicon includes an Etag, or a unique Last-Modified header, then it's > > trivial to track users with a single favicon. Additionally, it's > > straightforward to load 100 favicons sequentially using JavaScript. So it's > > easy to imagine tracking individual users via favicons alone. > > But the point that Tim was making is about favicons that are *not* loaded. > There is no such data for those favicons. If a favicon is not loaded, that's evidence that the browser has cached the favicon. Especially if other images are requested as expected, such as, as Tim points out, the XUL copy of the favicon. In any case, I do take your point the bug 1288054 will be a good solution, assuming the final favicon module enforces OA isolation.

Comment on attachment 8790587 [details] [diff] [review] Part 7 : Add a test case of favicon loading for first party isolation. Review of attachment 8790587 [details] [diff] [review]: ----------------------------------------------------------------- Arthur gave you a perfect review here. r+ with his comments.

Addressing review comments. Thanks, baku and arthur.

Comment on attachment 8794432 [details] [diff] [review] Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein Review of attachment 8794432 [details] [diff] [review]: ----------------------------------------------------------------- Hi, Tim. Thanks for the added caching tests. I'm suggesting some simplifications to avoid having to create a separate http server and eliminate global variables. Otherwise looks good. ::: browser/components/originattributes/test/browser/browser_favicon_firstParty.js @@ +1,2 @@ > +/** > + * Bug 1277803 - A test caes for testing favicon loading across different first party domains. caes -> case @@ +3,5 @@ > + */ > + > +const { classes: Cc, Constructor: CC, interfaces: Ci, utils: Cu } = Components; > + > +let {HttpServer} = Cu.import("resource://testing-common/httpd.js", {}); I think you don't need to create an http server if you monitor http-on-examine-cached-response vs http-on-examine-response. See my suggestion for waitForFaviconResponse below. In that case you can remove gHit, gHttpServer, and various associated functions. @@ +6,5 @@ > + > +let {HttpServer} = Cu.import("resource://testing-common/httpd.js", {}); > + > +XPCOMUtils.defineLazyModuleGetter(this, "Promise", > + "resource://gre/modules/Promise.jsm"); Is this needed? I thought Promise was built-in already (I could be wrong). @@ +31,5 @@ > +let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal(); > +let makeURI = Cu.import("resource://gre/modules/BrowserUtils.jsm", {}).BrowserUtils.makeURI; > + > +// A value to hold the favicon. > +let gFaviconData; Drop gFaviconData and use local variables. @@ +34,5 @@ > +// A value to hold the favicon. > +let gFaviconData; > + > +function getIconFile() { > + new Promise(resolve => { return new Promise... @@ +42,5 @@ > + contentPolicyType: Ci.nsIContentPolicy.TYPE_INTERNAL_IMAGE_FAVICON > + }, function(inputStream, status) { > + let size = inputStream.available(); > + gFaviconData = NetUtil.readInputStreamToString(inputStream, size); > + resolve(); Use resolve(faviconData); @@ +59,5 @@ > + }); > +}); > + > +// A boolean to show that whether the favicon is loaded. > +let gHit = false; Remove this `gHit` global and use waitForFaviconResponse returned promise as suggested below. @@ +88,5 @@ > + let observer = { > + observe(aSubject, aTopic, aData) { > + // Make sure that the topic is 'http-on-modify-request'. > + if (aTopic === "http-on-modify-request") { > + // We check the firstPartyDomain for the originAttributes of the loading I wonder if we should enclose this case in a try...catch. If something throws an error (such as httpChannel is somehow null) then would the test fail? @@ +137,5 @@ > + Services.obs.addObserver(observer, "http-on-modify-request", false); > + }); > +} > + > +function waitOnFavicon(aFaviconURL) { // Rename and move this function next to second task. function waitOnFaviconResponse(aFaviconURL) { @@ +154,5 @@ > + } > + > + Services.obs.removeObserver(observer, "http-on-examine-response", false); > + Services.obs.removeObserver(observer, "http-on-examine-cached-response", false); > + resolve(); resolve(aTopic); @@ +265,5 @@ > + } > + } > +}); > + > +add_task(function* test_favicon_cache_firstParty() { // Call this here to be safe? clearAlImageCaches(); @@ +272,5 @@ > + > + // Open the tab for the first site. > + let tabInfo = yield openTab(TEST_SITE_ONE + TEST_CACHE_PAGE); > + > + yield waitOnFavicon(TEST_FAVICON_CACHE_URI); let response = yield waitOnFaviconResponse(TEST_FAVICON_CACHE_URI); @@ +274,5 @@ > + let tabInfo = yield openTab(TEST_SITE_ONE + TEST_CACHE_PAGE); > + > + yield waitOnFavicon(TEST_FAVICON_CACHE_URI); > + > + ok(gHit, "The favicon image should be loaded through network."); is(response === "http-on-examine-response", "The favicon image should be loaded through network."); // (And similar below). @@ +286,5 @@ > + tabInfo = yield openTab(TEST_SITE_ONE + TEST_CACHE_PAGE); > + > + yield waitOnFavicon(TEST_FAVICON_CACHE_URI); > + > + ok(!gHit, "The favicon image should not be loaded through network."); is(response === "http-on-examine-cached-response", "The favicon image should not be loaded through network.");

Comment on attachment 8790578 [details] [diff] [review] Part 2 : Make favicons loaded through XUL:image use the correct principal. >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >- <xul:image xbl:inherits="src=image,fadein,pinned,selected=visuallyselected,busy,crashed,sharing" >+ <xul:image xbl:inherits="src=image,loadingprincipal=iconLoadingPrincipal,fadein,pinned,selected,visuallyselected,busy,crashed,sharing" You didn't ask me to review this part, but was it intentional to change "selected=visuallyselected" to "selected,visuallyselected"? That seems unrelated to this change. >@@ -221,27 +222,48 @@ nsImageBoxFrame::UpdateImage() >+ nsCOMPtr<nsISupports> serializedPrincipal; >+ NS_DeserializeObject(NS_ConvertUTF16toUTF8(imageLoadingPrincipal), >+ getter_AddRefs(serializedPrincipal)); >+ loadingPrincipal = do_QueryInterface(serializedPrincipal); >+ >+ if (loadingPrincipal) { >+ // Set the content policy type to TYPE_INTERNAL_IMAGE_FAVICON for >+ // indicating it's a favicon loading. >+ contentPolicyType = nsIContentPolicy::TYPE_INTERNAL_IMAGE_FAVICON; >+ } If loadingPrincipal can actually be null here, don't we want to fallback to mContent->NodePrincipal() instead of null?

(In reply to Timothy Nikkel (:tnikkel) from comment #66) > Comment on attachment 8790578 [details] [diff] [review] > Part 2 : Make favicons loaded through XUL:image use the correct principal. > > >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml > >- <xul:image xbl:inherits="src=image,fadein,pinned,selected=visuallyselected,busy,crashed,sharing" > >+ <xul:image xbl:inherits="src=image,loadingprincipal=iconLoadingPrincipal,fadein,pinned,selected,visuallyselected,busy,crashed,sharing" > > You didn't ask me to review this part, but was it intentional to change > "selected=visuallyselected" to "selected,visuallyselected"? That seems > unrelated to this change. > Oh, I must made this change unintentionally. I will change it back to "selected=visuallyselected". > >@@ -221,27 +222,48 @@ nsImageBoxFrame::UpdateImage() > >+ nsCOMPtr<nsISupports> serializedPrincipal; > >+ NS_DeserializeObject(NS_ConvertUTF16toUTF8(imageLoadingPrincipal), > >+ getter_AddRefs(serializedPrincipal)); > >+ loadingPrincipal = do_QueryInterface(serializedPrincipal); > >+ > >+ if (loadingPrincipal) { > >+ // Set the content policy type to TYPE_INTERNAL_IMAGE_FAVICON for > >+ // indicating it's a favicon loading. > >+ contentPolicyType = nsIContentPolicy::TYPE_INTERNAL_IMAGE_FAVICON; > >+ } > > If loadingPrincipal can actually be null here, don't we want to fallback to > mContent->NodePrincipal() instead of null? You are right here, I will fallback the loadingPrincipal to mContent->NodePrincipal() if it is null.

Addressing review comments. Thanks, tnikkel.

(In reply to Timothy Nikkel (:tnikkel) from comment #66) > Comment on attachment 8790578 [details] [diff] [review] > Part 2 : Make favicons loaded through XUL:image use the correct principal. > > >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml > >- <xul:image xbl:inherits="src=image,fadein,pinned,selected=visuallyselected,busy,crashed,sharing" > >+ <xul:image xbl:inherits="src=image,loadingprincipal=iconLoadingPrincipal,fadein,pinned,selected,visuallyselected,busy,crashed,sharing" > > You didn't ask me to review this part, but was it intentional to change > "selected=visuallyselected" to "selected,visuallyselected"? That seems > unrelated to this change. Oof, thanks for catching that...

Rewrite the cache test because the previous one does not handle the cache properly that tabs were closed too fast to be cached. Arther, please look this again, thanks.

Comment on attachment 8796323 [details] [diff] [review] Part 7 : Add a test case of favicon loading for first party isolation. Review of attachment 8796323 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: browser/components/originattributes/test/browser/browser_favicon_firstParty.js @@ +10,5 @@ > + > +const TEST_SITE_ONE = "http://" + FIRST_PARTY_ONE; > +const TEST_SITE_TWO = "http://" + FIRST_PARTY_TWO; > +const THIRD_PARTY_SITE = "http://" + THIRD_PARTY; > + const TEST_DIRECTORY = "/browser/browser/components/originattributes/test/browser/"; @@ +12,5 @@ > +const TEST_SITE_TWO = "http://" + FIRST_PARTY_TWO; > +const THIRD_PARTY_SITE = "http://" + THIRD_PARTY; > + > +const TEST_PAGE = "/browser/browser/components/originattributes/test/browser/" + > + "file_favicon.html"; const TEST_PAGE = TEST_DIRECTORY + "file_favicon.html"; (and similar below). @@ +185,5 @@ > + > + // Open the tab for the first site. > + let tabInfo = yield openTab(TEST_SITE_ONE + aTestPage); > + > + // Waiting for favicon requests are all made. Waiting until @@ +198,5 @@ > + > + // Open the tab for the second site. > + tabInfo = yield openTab(TEST_SITE_TWO + aTestPage); > + > + // Waiting for favicon requests are all made. Waiting until

Addressing the review comments. Thanks, Arthur.

Comment on attachment 8797344 [details] [diff] [review] Part 8 : Add a test case of favicon loading of private browsing. Review of attachment 8797344 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks great! ::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_favicon.js @@ +152,5 @@ > + // Create a private browsing window. > + let privateWindow = yield BrowserTestUtils.openNewBrowserWindow({ private: true }); > + let pageURI = makeURI(TEST_PAGE); > + > + // Generate two randon cookies for non-private window and private window Nit: *random

Update commit message.

Update commit message.

Addressing the review comment. Thanks, ehsan.

Add a clean up function to clear cookies and caches for preventing affecting other tests.

Same as above.

Same.

try looks good.

has problems to apply: adding 1277803 to series file renamed 1277803 -> Bug-1277803---Part-4--Make-the-NSCompareLoadInfoAn.patch applying Bug-1277803---Part-4--Make-the-NSCompareLoadInfoAn.patch patching file netwerk/protocol/http/HttpChannelChild.cpp Hunk #1 FAILED at 1748 1 out of 1 hunks FAILED -- saving rejects to file netwerk/protocol/http/HttpChannelChild.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh Bug-1277803---Part-4--Make-the-NSCompareLoadInfoAn.patch

Rebase to m-c.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d283c59402ce Part 1: Add a new ContentPolicy TYPE_INTERNAL_IMAGE_FAVICON for indicating a favicon loading. r=ckerschb https://hg.mozilla.org/integration/mozilla-inbound/rev/1d88072cf36a Part 2: Make favicons loaded through XUL:image use the correct principal. r=Gijs, r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a02bf398be Part 3: Make the image library uses the correct originAttributes and triggering principal when opening a image channel for favicon loading. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/d17370e68325 Part 4: Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image. r=honzab https://hg.mozilla.org/integration/mozilla-inbound/rev/76788d4f83ce Part 5: Add a test to verify the loadingPrincipal of favicon loads. r=ckerschb https://hg.mozilla.org/integration/mozilla-inbound/rev/f173e3210edf Part 6: Add a test case for favicon loading in different userContextIds. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/c9a23f0f9093 Part 7: Add a test case of favicon loading for first party isolation. r=baku, r=arthuredelstein https://hg.mozilla.org/integration/mozilla-inbound/rev/d416481d4813 Part 8: Add a test case of favicon loading of private browsing. r=ehsan

Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ee01bfb3065a Backed out changeset d416481d4813 for test_bug1277803.xul bustage https://hg.mozilla.org/integration/mozilla-inbound/rev/a0176b536d5a Backed out changeset c9a23f0f9093 https://hg.mozilla.org/integration/mozilla-inbound/rev/773ced366e56 Backed out changeset f173e3210edf https://hg.mozilla.org/integration/mozilla-inbound/rev/ae8e64903b30 Backed out changeset 76788d4f83ce https://hg.mozilla.org/integration/mozilla-inbound/rev/1a2caa8f6ba6 Backed out changeset d17370e68325 https://hg.mozilla.org/integration/mozilla-inbound/rev/45798b451d74 Backed out changeset d3a02bf398be https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa317481105 Backed out changeset 1d88072cf36a https://hg.mozilla.org/integration/mozilla-inbound/rev/d7790b3df592 Backed out changeset d283c59402ce

Beyond that permaorange on Android, also intermittents: https://treeherder.mozilla.org/logviewer.html#?job_id=37225917&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=37228640&repo=mozilla-inbound (below that other known failure) https://treeherder.mozilla.org/logviewer.html#?job_id=37229284&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=37227648&repo=mozilla-inbound

The failure on Android is because of that the test 'test_bug1277803.xul' is based on the behavior of the desktop browser. So this test won't work for the android. Hence, I think we should disable this test on Android. Christoph, what do you think about this?

The intermittents were triggered by the async behavior of the favicon loading of the places library. It looks like that the second favicon request was sent before the first favicon has been properly cached in the cache test. I think I have to find a way to make sure that the favicon has been fully loaded to prevent this intermittent.

Addressing the problem of the intermittent by making sure the favicon has been cached properly when testing cache.

(In reply to Tim Huang[:timhuang] from comment #88) > The failure on Android is because of that the test 'test_bug1277803.xul' is > based on the behavior of the desktop browser. So this test won't work for > the android. Hence, I think we should disable this test on Android. > > Christoph, what do you think about this? I suppose it's fine to dusable that test on Android for now because I would like to see that bug fixed. Please file a follow up so we can investigate and potentially add a test that also work on Android. Thanks!

Skip this test if it is Android.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f29f88c5c1b9 Part 1: Add a new ContentPolicy TYPE_INTERNAL_IMAGE_FAVICON for indicating a favicon loading. r=ckerschb https://hg.mozilla.org/integration/autoland/rev/58c23d794ba5 Part 2: Make favicons loaded through XUL:image use the correct principal. r=Gijs, r=tnikkel https://hg.mozilla.org/integration/autoland/rev/f4adcb16d1ec Part 3: Make the image library uses the correct originAttributes and triggering principal when opening a image channel for favicon loading. r=tnikkel https://hg.mozilla.org/integration/autoland/rev/835470de461d Part 4: Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image. r=honzab https://hg.mozilla.org/integration/autoland/rev/b6ea3bd24359 Part 5: Add a test to verify that the loadingPrincipal of favicon loads. r=ckerschb https://hg.mozilla.org/integration/autoland/rev/ef181cdd290f Part 6: Add a test case for favicon loading in different userContextIds. r=baku https://hg.mozilla.org/integration/autoland/rev/61947b3073ba Part 7: Add a test case of favicon loading for first party isolation. r=baku, r=arthuredelstein https://hg.mozilla.org/integration/autoland/rev/a046eef2014c Part 8: Add a test case of favicon loading of private browsing. r=ehsan

Eslint complained about the violation of a rule (whitespace between ) and { missing), pushed a follow-up: https://hg.mozilla.org/integration/autoland/rev/3745b8ba92b1faf5da9762831b72de987b962b68

Backed out in https://hg.mozilla.org/integration/autoland/rev/ef962d8857009eff3a2df14c06e4bc72acb13691 for frequent failures in e10s browser_privatebrowsing_favicon.js, both the https://treeherder.mozilla.org/logviewer.html#?job_id=4716555&repo=autoland failure flavor and the https://treeherder.mozilla.org/logviewer.html#?job_id=4716563&repo=autoland timeout plus leak until shutdown flavor.

philor, are both bug 1308684 and bug 1308680 maybe also fallout from this change that have gone away since it was backed out?

Yep.

Fixing the eslint error.

Fixing the intermittent failure.

Fixing the timeout problem.

Fixing timeout issue.

Still has timeout problem. Trying to fix.

Fixing intermittent failures.

Fixing intermittent failures.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=195aa13c2756d630e4de254bb1eef32a5743d866 I have run tests for ten times to make sure intermittent failures never happen again at certain platforms.

Pushed by jhao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/be767a6f7ecd Part 1 : Add a new ContentPolicy TYPE_INTERNAL_IMAGE_FAVICON for indicating a favicon loading. r=ckerschb https://hg.mozilla.org/integration/mozilla-inbound/rev/8d119ca96999 Part 2 : Make favicons loaded through XUL:image use the correct principal. r=Gijs, tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/3be9a06248af Part 3 : Make the image library uses the correct originAttributes and triggering principal when opening a image channel for favicon loading. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d82459d152 Part 4 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image. r=honzab https://hg.mozilla.org/integration/mozilla-inbound/rev/b1cb0a195ca1 Part 5 : A test to verify the loadingPrincipal of favicon loads. r=ckerschb https://hg.mozilla.org/integration/mozilla-inbound/rev/2d17a40a9077 Part 6 : Add a test case for favicon loading in different userContextIds. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/49da326bfe68 Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein https://hg.mozilla.org/integration/mozilla-inbound/rev/477890efdb88 Part 8 : Add a test case of favicon loading of private browsing. r=ehsan

Needed apparently more than ten runs, because I've seen several failures like https://treeherder.mozilla.org/logviewer.html#?job_id=37484775&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=37487195&repo=mozilla-inbound since this landed. All backed out in https://hg.mozilla.org/mozilla-central/rev/22be4ae74653b25186665f22e52a50e7027fd36b

Thanks, I will take a look.

Fixing the intermittents.

Fixing the intermittents.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72bd27c7989d Run more times for testing intermittents.

Pushed by jhao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4639ff15be81 Part 1 : Add a new ContentPolicy TYPE_INTERNAL_IMAGE_FAVICON for indicating a favicon loading. r=ckerschb https://hg.mozilla.org/integration/mozilla-inbound/rev/5480dde6c54e Part 2 : Make favicons loaded through XUL:image use the correct principal. r=Gijs, tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b4886a1415 Part 3 : Make the image library uses the correct originAttributes and triggering principal when opening a image channel for favicon loading. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9d0ae4bba4 Part 4 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image. r=honzab https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb1c64b4059 Part 5 : A test to verify the loadingPrincipal of favicon loads. r=ckerschb https://hg.mozilla.org/integration/mozilla-inbound/rev/df5881a8b808 Part 6 : Add a test case for favicon loading in different userContextIds. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b99627c2da Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein https://hg.mozilla.org/integration/mozilla-inbound/rev/60137e6c3530 Part 8 : Add a test case of favicon loading of private browsing. r=ehsan

https://hg.mozilla.org/mozilla-central/rev/4639ff15be81 https://hg.mozilla.org/mozilla-central/rev/5480dde6c54e https://hg.mozilla.org/mozilla-central/rev/d7b4886a1415 https://hg.mozilla.org/mozilla-central/rev/ae9d0ae4bba4 https://hg.mozilla.org/mozilla-central/rev/4bb1c64b4059 https://hg.mozilla.org/mozilla-central/rev/df5881a8b808 https://hg.mozilla.org/mozilla-central/rev/b6b99627c2da https://hg.mozilla.org/mozilla-central/rev/60137e6c3530

Tim, thank you for the hard work to complete this arduous work. Good job! Thanks for all the reviewers as well. Cheers!