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

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: timhuang, Assigned: timhuang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [OA][userContextId][domsecurity-active][tor][tor 13670.1])

Attachments

(8 attachments, 34 obsolete attachments)

14.45 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
5.38 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
2.47 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
6.29 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
6.86 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
12.49 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
15.74 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
14.49 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
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

a year ago
Created attachment 8766908 [details] [diff] [review]
Bug1277803.patch
Attachment #8766908 - Flags: review?(tanvi)
Component: Security → DOM: Security
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.
Attachment #8766908 - Flags: review?(tanvi) → review-

Comment 3

a year ago
Maybe the URL of the tab window loading the favicon would suffice?
Flags: needinfo?(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.
Flags: needinfo?(ckerschb)

Updated

a year ago
See Also: → bug 1270678
Blocks: 1264564
Whiteboard: [OA][userContextId][domsecurity-backlog] → [OA][userContextId][domsecurity-backlog3]
When do favicon requests go through xul:image?  Do all favicon requests go through this?  Is this only used on session restore?
(Assignee)

Comment 6

a year ago
(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
Assignee: nobody → senglehardt
(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
Flags: needinfo?(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
Flags: needinfo?(ckerschb)
Blocks: 1277808
I filed Bug 1294866 for the SessionStore fix.
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
Attachment #8783100 - Flags: feedback?(ckerschb)
Created attachment 8783101 [details] [diff] [review]
1277803-serialization.patch

WIP serialization approach. Need to verify cache loads works as expected.
Attachment #8766908 - Attachment is obsolete: true
Attachment #8783100 - Flags: feedback?(ckerschb)
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
Flags: needinfo?(ckerschb)
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.
Flags: needinfo?(ckerschb) → needinfo?(senglehardt)
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
Flags: needinfo?(senglehardt)
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
Flags: needinfo?(tihuang)
(Assignee)

Updated

a year ago
Assignee: bugzilla → tihuang
Flags: needinfo?(tihuang)
(Assignee)

Comment 17

a year ago
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
(Assignee)

Comment 18

a year ago
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?
Flags: needinfo?(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?
Flags: needinfo?(ckerschb)
(Assignee)

Comment 20

a year ago
(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.

Updated

a year ago
Blocks: 1299996
(Assignee)

Comment 21

a year ago
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.
(Assignee)

Comment 22

a year ago
Created attachment 8787586 [details] [diff] [review]
Part 1 : Add a new ContentPolicy TYPE_INTERNAL_IMAGE_FAVICON for indicating a favicon loading.
Attachment #8787586 - Flags: review?(ckerschb)
(Assignee)

Comment 23

a year ago
Created attachment 8787587 [details] [diff] [review]
Part 2 : Make favicons loaded through XUL:image use the correct principal.
Attachment #8787587 - Flags: feedback?(ckerschb)
(Assignee)

Updated

a year ago
Attachment #8783101 - Attachment is obsolete: true
(Assignee)

Comment 24

a year ago
Created attachment 8787588 [details] [diff] [review]
Part 3 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image.
Attachment #8787588 - Flags: review?(honzab.moz)
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.
Attachment #8787586 - Flags: review?(ckerschb) → review+
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 }
Attachment #8787587 - Flags: feedback?(ckerschb) → feedback+
(Assignee)

Updated

a year ago
Duplicate of this bug: 1264564
(Assignee)

Comment 28

a year ago
Created attachment 8788773 [details] [diff] [review]
Part 1 : Add a new ContentPolicy TYPE_INTERNAL_IMAGE_FAVICON for indicating a favicon loading.

Addressing review comments.
Attachment #8788773 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8787586 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [OA][userContextId][domsecurity-backlog3] → [OA][userContextId][domsecurity-active]
(Assignee)

Comment 29

a year ago
Created attachment 8788812 [details] [diff] [review]
Part 2 : Make favicons loaded through XUL:image use the correct principal.

Addressing review comments. Thanks, christoph. And flag billm for review.
Attachment #8788812 - Flags: review?(wmccloskey)
(Assignee)

Updated

a year ago
Attachment #8787587 - Attachment is obsolete: true
(Assignee)

Comment 30

a year ago
Created attachment 8788822 [details] [diff] [review]
Part 4 : A test to verify the loadingPrincipal of favicon loads.
Attachment #8788822 - Flags: review?(ckerschb)
(Assignee)

Updated

a year ago
Attachment #8783100 - Attachment is obsolete: true
(Assignee)

Comment 31

a year ago
Created attachment 8788825 [details] [diff] [review]
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.
Attachment #8788825 - Flags: review?(amarchesini)
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;
}
Attachment #8788822 - Flags: review?(ckerschb) → review+
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...
Attachment #8787588 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 34

a year ago
Created attachment 8789190 [details] [diff] [review]
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.
Attachment #8789190 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8788822 - Attachment is obsolete: true
(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.
(Assignee)

Comment 36

a year ago
Created attachment 8789254 [details] [diff] [review]
Part 3 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image.

Addressing review comments. Thanks, Honza.
Attachment #8789254 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8787588 - Attachment is obsolete: true
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.
Attachment #8788825 - Flags: review?(amarchesini) → review+
Whiteboard: [OA][userContextId][domsecurity-active] → [OA][userContextId][domsecurity-active][tor]
(Assignee)

Comment 39

a year ago
(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?
Flags: needinfo?(honzab.moz)
(Assignee)

Updated

a year ago
Attachment #8788812 - Flags: review?(wmccloskey) → review?(gijskruitbosch+bugs)

Comment 40

a year ago
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.
Attachment #8788812 - Flags: review?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 42

a year ago
(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.
(Assignee)

Comment 43

a year ago
Created attachment 8789834 [details] [diff] [review]
Part 3 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image.
Attachment #8789834 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8789254 - Attachment is obsolete: true
(Assignee)

Comment 44

11 months ago
Created attachment 8790578 [details] [diff] [review]
Part 2 : Make favicons loaded through XUL:image use the correct principal.
Attachment #8790578 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

11 months ago
Attachment #8788812 - Attachment is obsolete: true
(Assignee)

Comment 45

11 months ago
(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.
(Assignee)

Comment 46

11 months ago
Created attachment 8790581 [details] [diff] [review]
Part 3 : Make the image library uses the correct originAttributes and triggering principal when opening a image channel for favicon loading.
Attachment #8790581 - Flags: review?(tnikkel)
(Assignee)

Comment 47

11 months ago
Created attachment 8790583 [details] [diff] [review]
Part 4 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image.

Updating commit message.
Attachment #8790583 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8789834 - Attachment is obsolete: true
(Assignee)

Comment 48

11 months ago
Created attachment 8790584 [details] [diff] [review]
Part 5 : A test to verify the loadingPrincipal of favicon loads.
Attachment #8790584 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8789190 - Attachment is obsolete: true
(Assignee)

Comment 49

11 months ago
Created attachment 8790586 [details] [diff] [review]
Part 6 : Add a test case for favicon loading in different userContextIds.
Attachment #8790586 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8788825 - Attachment is obsolete: true
(Assignee)

Comment 50

11 months ago
Created attachment 8790587 [details] [diff] [review]
Part 7 : Add a test case of favicon loading for first party isolation.
Attachment #8790587 - Flags: review?(arthuredelstein)
Attachment #8790587 - Flags: review?(amarchesini)

Comment 51

11 months ago
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.
Attachment #8790578 - Flags: review?(tnikkel)
Attachment #8790578 - Flags: review?(gijskruitbosch+bugs)
Attachment #8790578 - Flags: review+
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* ().
Attachment #8790587 - Flags: review?(arthuredelstein)
(Assignee)

Comment 53

11 months ago
(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;
  }
}
Flags: needinfo?(arthuredelstein)
(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;
> }
Flags: needinfo?(arthuredelstein)
(Assignee)

Comment 55

11 months ago
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
Flags: needinfo?(gijskruitbosch+bugs)

Comment 56

11 months ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 57

11 months ago
(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

11 months ago
(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.
(Assignee)

Comment 59

11 months ago
(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.

Comment 61

11 months ago
(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.
Attachment #8790587 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 64

11 months ago
Created attachment 8794432 [details] [diff] [review]
Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Addressing review comments. Thanks, baku and arthur.
Attachment #8794432 - Flags: review?(arthuredelstein)
(Assignee)

Updated

11 months ago
Attachment #8790587 - Attachment is obsolete: true
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.");
Attachment #8794432 - Flags: review?(arthuredelstein) → review+
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?
Attachment #8790578 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 67

11 months ago
(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.
(Assignee)

Comment 68

11 months ago
Created attachment 8795612 [details] [diff] [review]
Part 2 : Make favicons loaded through XUL:image use the correct principal.

Addressing review comments. Thanks, tnikkel.
Attachment #8795612 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8790578 - Attachment is obsolete: true

Updated

11 months ago
Attachment #8790581 - Flags: review?(tnikkel) → review+

Comment 69

11 months ago
(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...
(Assignee)

Comment 70

11 months ago
Created attachment 8796323 [details] [diff] [review]
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.
Attachment #8796323 - Flags: review?(arthuredelstein)
(Assignee)

Updated

11 months ago
Attachment #8794432 - Attachment is obsolete: true
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
Attachment #8796323 - Flags: review?(arthuredelstein) → review+
(Assignee)

Updated

11 months ago
Blocks: 1305592
(Assignee)

Comment 72

11 months ago
Created attachment 8797344 [details] [diff] [review]
Part 8 : Add a test case of favicon loading of private browsing.
Attachment #8797344 - Flags: review?(ehsan)
(Assignee)

Comment 73

11 months ago
Created attachment 8797345 [details] [diff] [review]
Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Addressing the review comments. Thanks, Arthur.
Attachment #8797345 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8796323 - Attachment is obsolete: true
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
Attachment #8797344 - Flags: review?(ehsan) → review+
(Assignee)

Comment 75

11 months ago
Created attachment 8797866 [details] [diff] [review]
Part 2 : Make favicons loaded through XUL:image use the correct principal. r=Gijs, tnikkel

Update commit message.
Attachment #8797866 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8795612 - Attachment is obsolete: true
(Assignee)

Comment 76

11 months ago
Created attachment 8797868 [details] [diff] [review]
Part 3 : Make the image library uses the correct originAttributes and triggering principal when opening a image channel for favicon loading.

Update commit message.
Attachment #8797868 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8790581 - Attachment is obsolete: true
(Assignee)

Comment 77

11 months ago
Created attachment 8797877 [details] [diff] [review]
Part 8 : Add a test case of favicon loading of private browsing.

Addressing the review comment. Thanks, ehsan.
Attachment #8797877 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8797344 - Attachment is obsolete: true
(Assignee)

Comment 78

11 months ago
Created attachment 8797932 [details] [diff] [review]
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.
Attachment #8797932 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8790586 - Attachment is obsolete: true
(Assignee)

Comment 79

11 months ago
Created attachment 8797933 [details] [diff] [review]
Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Same as above.
Attachment #8797933 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8797345 - Attachment is obsolete: true
(Assignee)

Comment 80

11 months ago
Created attachment 8797934 [details] [diff] [review]
Part 8 : Add a test case of favicon loading of private browsing.

Same.
Attachment #8797934 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8797877 - Attachment is obsolete: true
(Assignee)

Comment 81

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b71273e4cf7fae9d772a0a953997f2592f66ec0d
(Assignee)

Comment 82

11 months ago
try looks good.
Keywords: checkin-needed

Updated

11 months ago
No longer depends on: 1166891
See Also: → bug 1166891

Comment 83

11 months ago
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
Flags: needinfo?(tihuang)
Keywords: checkin-needed
(Assignee)

Comment 84

11 months ago
Created attachment 8798335 [details] [diff] [review]
Part 4 : Make the NS_CompareLoadInfoAndLoadContext() skiping test if the request is the favicon loading from the XUL image.

Rebase to m-c.
Attachment #8798335 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8790583 - Attachment is obsolete: true
(Assignee)

Updated

11 months ago
Flags: needinfo?(tihuang)
Keywords: checkin-needed

Comment 85

11 months ago
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
Keywords: checkin-needed

Comment 86

11 months ago
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

Updated

11 months ago
Flags: needinfo?(tihuang)
(Assignee)

Comment 88

11 months ago
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?
Flags: needinfo?(tihuang)
(Assignee)

Comment 89

11 months ago
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.
(Assignee)

Updated

11 months ago
Flags: needinfo?(ckerschb)
(Assignee)

Comment 90

11 months ago
Created attachment 8798771 [details] [diff] [review]
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.
Attachment #8798771 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8797933 - Attachment is obsolete: true
(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!
Flags: needinfo?(ckerschb)
(Assignee)

Comment 92

11 months ago
Created attachment 8798842 [details] [diff] [review]
Part 5 : A test to verify the loadingPrincipal of favicon loads.

Skip this test if it is Android.
Attachment #8798842 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8790584 - Attachment is obsolete: true
(Assignee)

Comment 93

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab943d749cb9aadba84e24d10105585067cd4c23
Keywords: checkin-needed

Comment 94

11 months ago
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
Keywords: checkin-needed
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.

Comment 97

11 months ago
philor, are both bug 1308684 and bug 1308680 maybe also fallout from this change that have gone away since it was backed out?
Flags: needinfo?(philringnalda)
Yep.
Flags: needinfo?(philringnalda)

Updated

11 months ago
Duplicate of this bug: 1308684

Updated

11 months ago
Duplicate of this bug: 1308680
(Assignee)

Comment 101

11 months ago
Created attachment 8799399 [details] [diff] [review]
Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Fixing the eslint error.
Attachment #8799399 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8798771 - Attachment is obsolete: true
(Assignee)

Comment 102

11 months ago
Created attachment 8799401 [details] [diff] [review]
Part 8 : Add a test case of favicon loading of private browsing.

Fixing the intermittent failure.
Attachment #8799401 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8797934 - Attachment is obsolete: true
(Assignee)

Comment 103

11 months ago
Created attachment 8799655 [details] [diff] [review]
Part 8 : Add a test case of favicon loading of private browsing.

Fixing the timeout problem.
Attachment #8799655 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8799401 - Attachment is obsolete: true
(Assignee)

Comment 104

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0905136d046fd2d0938e0567bd42f34488e51c82
(Assignee)

Comment 105

11 months ago
Created attachment 8799738 [details] [diff] [review]
Part 6 : Add a test case for favicon loading in different userContextIds.

Fixing timeout issue.
Attachment #8799738 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8797932 - Attachment is obsolete: true
(Assignee)

Comment 106

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=639c8e1c6534aaa896018ceaf90f532836d85704
(Assignee)

Comment 107

11 months ago
Created attachment 8799812 [details] [diff] [review]
Part 6 : Add a test case for favicon loading in different userContextIds.

Still has timeout problem. Trying to fix.
Attachment #8799812 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8799738 - Attachment is obsolete: true
(Assignee)

Comment 108

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=694496038f1722723e59a71ef5883dc2147e5362
(Assignee)

Comment 109

11 months ago
Created attachment 8800154 [details] [diff] [review]
Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Fixing intermittent failures.
Attachment #8800154 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8799399 - Attachment is obsolete: true
(Assignee)

Comment 110

11 months ago
Created attachment 8800156 [details] [diff] [review]
Part 8 : Add a test case of favicon loading of private browsing.

Fixing intermittent failures.
Attachment #8800156 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8799655 - Attachment is obsolete: true
(Assignee)

Comment 111

11 months ago
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

11 months ago
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

Comment 113

11 months ago
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
Flags: needinfo?(tihuang)
(Assignee)

Comment 114

10 months ago
Thanks, I will take a look.
Flags: needinfo?(tihuang)
(Assignee)

Comment 115

10 months ago
Created attachment 8800546 [details] [diff] [review]
Part 7 : Add a test case of favicon loading for first party isolation. r=baku, arthuredelstein

Fixing the intermittents.
Attachment #8800546 - Flags: review+
(Assignee)

Updated

10 months ago
Attachment #8800154 - Attachment is obsolete: true
(Assignee)

Comment 116

10 months ago
Created attachment 8800547 [details] [diff] [review]
Part 8 : Add a test case of favicon loading of private browsing.

Fixing the intermittents.
Attachment #8800547 - Flags: review+
(Assignee)

Updated

10 months ago
Attachment #8800156 - Attachment is obsolete: true
(Assignee)

Comment 117

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

Run more times for testing intermittents.

Comment 118

10 months ago
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

Comment 119

10 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Comment 120

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

Updated

10 months ago
Depends on: 1311237
Depends on: 1310092
Depends on: 1319908
Whiteboard: [OA][userContextId][domsecurity-active][tor] → [OA][userContextId][domsecurity-active][tor][tor 13670.1]

Updated

5 months ago
Depends on: 1351084
You need to log in before you can comment on or make changes to this bug.