1277803 - Make the loading of favicon through the XUL:image uses the correct originAttributes

Status: RESOLVED FIXED

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

Description

[Tim Huang[:timhuang]]

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 1

[James Andreou]

Attachment: Bug1277803.patch

Comment 2

[Tanvi Vyas[:tanvi]]

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.

Comment 3

[James Andreou]

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

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 5

[Tanvi Vyas[:tanvi]]

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

Comment 6

[Tim Huang[:timhuang]]

(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

Comment 7

[Steven Englehardt [:englehardt]]

(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

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 9

[Steven Englehardt [:englehardt]]

I filed Bug 1294866 for the SessionStore fix.

Comment 10

[Steven Englehardt [:englehardt]]

Attachment: 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

Comment 11

[Steven Englehardt [:englehardt]]

Attachment: 1277803-serialization.patch

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

Comment 12

[Steven Englehardt [:englehardt]]

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

Comment 13

[Steven Englehardt [:englehardt]]

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

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 15

[Steven Englehardt [:englehardt]]

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

Comment 16

[Tanvi Vyas[:tanvi]]

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

Comment 17

[Tim Huang[:timhuang]]

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

Comment 18

[Tim Huang[:timhuang]]

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?

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

(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?

Comment 20

[Tim Huang[:timhuang]]

(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.

Comment 21

[Tim Huang[:timhuang]]

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 22

[Tim Huang[:timhuang]]

Attachment: Part 1 : Add a new ContentPolicy TYPE_INTERNAL_IMAGE_FAVICON for indicating a favicon loading.

Comment 23

[Tim Huang[:timhuang]]

Attachment: Part 2 : Make favicons loaded through XUL:image use the correct principal.

Comment 24

[Tim Huang[:timhuang]]

Attachment: Part 3 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

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 26

[Christoph Kerschbaumer [:ckerschb]]

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 }

Comment 28

[Tim Huang[:timhuang]]

Attachment: Part 1 : Add a new ContentPolicy TYPE_INTERNAL_IMAGE_FAVICON for indicating a favicon loading.

Addressing review comments.

Comment 29

[Tim Huang[:timhuang]]

Attachment: Part 2 : Make favicons loaded through XUL:image use the correct principal.

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

Comment 30

[Tim Huang[:timhuang]]

Attachment: Part 4 : A test to verify the loadingPrincipal of favicon loads.

Comment 31

[Tim Huang[:timhuang]]

Attachment: Part 5 : Add a test case for favicon loading in different userContextIds.

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

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

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 33

[Honza Bambas (:mayhemer)]

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...

Comment 34

[Tim Huang[:timhuang]]

Attachment: Part 4 : A test to verify the loadingPrincipal of favicon loads.

(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.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 36

[Tim Huang[:timhuang]]

Attachment: Part 3 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image.

Addressing review comments. Thanks, Honza.

Comment 37

[Honza Bambas (:mayhemer)]

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 38

[Andrea Marchesini [:baku]]

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.

Comment 39

[Tim Huang[:timhuang]]

(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 40

[:Gijs (out for now; he/him)]

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.

Comment 41

[Honza Bambas (:mayhemer)]

(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.

Comment 42

[Tim Huang[:timhuang]]

(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.

Comment 43

[Tim Huang[:timhuang]]

Attachment: Part 3 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image.

Comment 44

[Tim Huang[:timhuang]]

Attachment: Part 2 : Make favicons loaded through XUL:image use the correct principal.

Comment 45

[Tim Huang[:timhuang]]

(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.

Comment 46

[Tim Huang[:timhuang]]

Attachment: Part 3 : Make the image library uses the correct originAttributes and triggering principal when opening a image channel for favicon loading.

Comment 47

[Tim Huang[:timhuang]]

Attachment: Part 4 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image.

Updating commit message.

Comment 48

[Tim Huang[:timhuang]]

Attachment: Part 5 : A test to verify the loadingPrincipal of favicon loads.

Comment 49

[Tim Huang[:timhuang]]

Attachment: Part 6 : Add a test case for favicon loading in different userContextIds.

Comment 50

[Tim Huang[:timhuang]]

Attachment: Part 7 : Add a test case of favicon loading for first party isolation.

Comment 51

[:Gijs (out for now; he/him)]

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 52

[Arthur Edelstein [:arthur]]

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* ().

Comment 53

[Tim Huang[:timhuang]]

(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;
  }
}

Comment 54

[Arthur Edelstein [:arthur]]

(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;
> }

Comment 55

[Tim Huang[:timhuang]]

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

Comment 56

[:Gijs (out for now; he/him)]

(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.

Comment 57

[Tim Huang[:timhuang]]

(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.

Comment 58

[:Gijs (out for now; he/him)]

(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.

Comment 59

[Tim Huang[:timhuang]]

(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.

Comment 60

[Arthur Edelstein [:arthur]]

(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.

Comment 61

[:Gijs (out for now; he/him)]

(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.

Comment 62

[Arthur Edelstein [:arthur]]

(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 63

[Andrea Marchesini [:baku]]

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.

Comment 64

[Tim Huang[:timhuang]]

Attachment: Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Addressing review comments. Thanks, baku and arthur.

Comment 65

[Arthur Edelstein [: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 66

[Timothy Nikkel (:tnikkel)]

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?

Comment 67

[Tim Huang[:timhuang]]

(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.

Comment 68

[Tim Huang[:timhuang]]

Attachment: Part 2 : Make favicons loaded through XUL:image use the correct principal.

Addressing review comments. Thanks, tnikkel.

Comment 69

[:Gijs (out for now; he/him)]

(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...

Comment 70

[Tim Huang[:timhuang]]

Attachment: Part 7 : Add a test case of favicon loading for first party isolation.

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 71

[Arthur Edelstein [:arthur]]

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

Comment 72

[Tim Huang[:timhuang]]

Attachment: Part 8 : Add a test case of favicon loading of private browsing.

Comment 73

[Tim Huang[:timhuang]]

Attachment: Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Addressing the review comments. Thanks, Arthur.

Comment 74

[(no longer active)]

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

Comment 75

[Tim Huang[:timhuang]]

Attachment: Part 2 : Make favicons loaded through XUL:image use the correct principal. r=Gijs, tnikkel

Update commit message.

Comment 76

[Tim Huang[:timhuang]]

Attachment: Part 3 : Make the image library uses the correct originAttributes and triggering principal when opening a image channel for favicon loading.

Update commit message.

Comment 77

[Tim Huang[:timhuang]]

Attachment: Part 8 : Add a test case of favicon loading of private browsing.

Addressing the review comment. Thanks, ehsan.

Comment 78

[Tim Huang[:timhuang]]

Attachment: Part 6 : Add a test case for favicon loading in different userContextIds.

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

Comment 79

[Tim Huang[:timhuang]]

Attachment: Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Same as above.

Comment 80

[Tim Huang[:timhuang]]

Attachment: Part 8 : Add a test case of favicon loading of private browsing.

Same.

Comment 81

[Tim Huang[:timhuang]]

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

Comment 82

[Tim Huang[:timhuang]]

try looks good.

Comment 83

[Carsten Book [:Tomcat]]

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

Comment 84

[Tim Huang[:timhuang]]

Attachment: Part 4 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image.

Rebase to m-c.

Comment 85

[Pulsebot]

Pushed by [e-mail]:
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

Comment 86

[Pulsebot]

Backout by [e-mail]:
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

Comment 87

[Phil Ringnalda (:philor)]

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

Comment 88

[Tim Huang[:timhuang]]

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?

Comment 89

[Tim Huang[:timhuang]]

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.

Comment 90

[Tim Huang[:timhuang]]

Attachment: Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

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

Comment 91

[Christoph Kerschbaumer [:ckerschb]]

(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!

Comment 92

[Tim Huang[:timhuang]]

Attachment: Part 5 : A test to verify the loadingPrincipal of favicon loads.

Skip this test if it is Android.

Comment 93

[Tim Huang[:timhuang]]

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

Comment 94

[Pulsebot]

Pushed by [e-mail]:
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

Comment 95

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 96

[Phil Ringnalda (:philor)]

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.

Comment 97

[:Gijs (out for now; he/him)]

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

Comment 98

[Phil Ringnalda (:philor)]

Yep.

Comment 101

[Tim Huang[:timhuang]]

Attachment: Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Fixing the eslint error.

Comment 102

[Tim Huang[:timhuang]]

Attachment: Part 8 : Add a test case of favicon loading of private browsing.

Fixing the intermittent failure.

Comment 103

[Tim Huang[:timhuang]]

Attachment: Part 8 : Add a test case of favicon loading of private browsing.

Fixing the timeout problem.

Comment 104

[Tim Huang[:timhuang]]

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

Comment 105

[Tim Huang[:timhuang]]

Attachment: Part 6 : Add a test case for favicon loading in different userContextIds.

Fixing timeout issue.

Comment 106

[Tim Huang[:timhuang]]

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

Comment 107

[Tim Huang[:timhuang]]

Attachment: Part 6 : Add a test case for favicon loading in different userContextIds.

Still has timeout problem. Trying to fix.

Comment 108

[Tim Huang[:timhuang]]

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

Comment 109

[Tim Huang[:timhuang]]

Attachment: Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Fixing intermittent failures.

Comment 110

[Tim Huang[:timhuang]]

Attachment: Part 8 : Add a test case of favicon loading of private browsing.

Fixing intermittent failures.

Comment 111

[Tim Huang[:timhuang]]

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.

Comment 112

[Pulsebot]

Pushed by [e-mail]:
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

Comment 113

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 114

[Tim Huang[:timhuang]]

Thanks, I will take a look.

Comment 115

[Tim Huang[:timhuang]]

Attachment: Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Fixing the intermittents.

Comment 116

[Tim Huang[:timhuang]]

Attachment: Part 8 : Add a test case of favicon loading of private browsing.

Fixing the intermittents.

Comment 117

[Tim Huang[:timhuang]]

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

Run more times for testing intermittents.

Comment 118

[Pulsebot]

Pushed by [e-mail]:
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

Comment 119

[Carsten Book [:Tomcat]]

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

Comment 120

[Ethan Tseng [:ethan]]

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