Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#16983 closed defect (fixed)

Favicon requests not isolated if one opens the tab list dropdown

Reported by: someone_else Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-linkability, TorBrowserTeam201509R
Cc: arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When the tab list dropdown has expired favicon images, they are all fetched via the same circuit.

Socks user: "--unknown--"
Socks pw: "100"

TBB version is 5.0.2.

Child Tickets

Attachments (3)

Screen Shot 2015-09-15 at 11.34.57 AM.png (87.0 KB) - added by arthuredelstein 4 years ago.
Screen Shot 2015-09-15 at 11.35.50 AM.png (83.1 KB) - added by arthuredelstein 4 years ago.
stack.txt (36.7 KB) - added by mcs 4 years ago.
stack trace for AsElement() assertion failure

Download all attachments as: .zip

Change History (23)

comment:1 Changed 4 years ago by someone_else

Component: - Select a componentTor Browser
Owner: set to tbb-team

comment:2 Changed 4 years ago by gk

Cc: arthuredelstein added
Keywords: tbb-linkability added
Priority: normalmajor
Summary: Favicon requests not isolatedFavicon requests not isolated if one opens the tab list dropdown

For me they are fetched as soon as I open the tab list dropdown.

comment:3 Changed 4 years ago by arthuredelstein

Here's a fix for this bug, for review:

https://github.com/arthuredelstein/tor-browser/commit/16983

I changed the firstparty attribute semantics somewhat. With this patch, if a chrome XUL/HTML element has its firstparty attribute set with a URI, then that first party URI will apply to all descendant elements. So now we simply set firstparty on the tab element and the menuitem element and their child image elements will automatically receive the correct isolation domain.

comment:4 Changed 4 years ago by arthuredelstein

Keywords: TorBrowserTeam2015R added
Status: newneeds_review

comment:5 Changed 4 years ago by arthuredelstein

Keywords: TorBrowserTeam201509R added; TorBrowserTeam2015R removed

comment:6 Changed 4 years ago by gk

So, this actually means there won't be any fetches anymore at all as long as I have the tab open (at least that's what I am seeing)? If not how could I verify that your patch is working as expected?

comment:7 in reply to:  6 ; Changed 4 years ago by arthuredelstein

Replying to gk:

So, this actually means there won't be any fetches anymore at all as long as I have the tab open (at least that's what I am seeing)?

With this patch, the assigned isolation domain is the same for both tab and dropdown. So my interpretation is that the favicon.ico file is cached and doesn't need to be fetched again.

If not how could I verify that your patch is working as expected?

You can set torbutton's loglevel pref to 3 to see it each request and its assigned isolation domain. If you see the correct favicon appear in both the tab and the dropdown for a given site, and you don't see any no-first-party fetches for the favicon file, then I would argue that the problem is fixed.

comment:8 in reply to:  7 ; Changed 4 years ago by gk

Replying to arthuredelstein:

Replying to gk:

So, this actually means there won't be any fetches anymore at all as long as I have the tab open (at least that's what I am seeing)?

With this patch, the assigned isolation domain is the same for both tab and dropdown. So my interpretation is that the favicon.ico file is cached and doesn't need to be fetched again.

Yes, but what happens if the cache is full? Do the favicons get evicted so that they need to get refetched if you open the dropdown? If yes, what happens in this case wrt circuit usage? If no, this is good. But is that really the case?

If not how could I verify that your patch is working as expected?

You can set torbutton's loglevel pref to 3 to see it each request and its assigned isolation domain. If you see the correct favicon appear in both the tab and the dropdown for a given site, and you don't see any no-first-party fetches for the favicon file, then I would argue that the problem is fixed.

Sure. The problem is that I don't see any fetches at all if I click on the dropdown. What I'd like to see is fetches using the circuit bound to the respective domain if this can happen at all.

comment:9 in reply to:  8 Changed 4 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

Replying to gk:

So, this actually means there won't be any fetches anymore at all as long as I have the tab open (at least that's what I am seeing)?

With this patch, the assigned isolation domain is the same for both tab and dropdown. So my interpretation is that the favicon.ico file is cached and doesn't need to be fetched again.

Yes, but what happens if the cache is full? Do the favicons get evicted so that they need to get refetched if you open the dropdown? If yes, what happens in this case wrt circuit usage? If no, this is good. But is that really the case?

If not how could I verify that your patch is working as expected?

You can set torbutton's loglevel pref to 3 to see it each request and its assigned isolation domain. If you see the correct favicon appear in both the tab and the dropdown for a given site, and you don't see any no-first-party fetches for the favicon file, then I would argue that the problem is fixed.

Sure. The problem is that I don't see any fetches at all if I click on the dropdown. What I'd like to see is fetches using the circuit bound to the respective domain if this can happen at all.

Here's a simple manual test that replaces the first favicon image in the dropdown when the dropdown is opened. First set torbutton's loglevel pref to 3. Then open https://www.torproject.org/ in the leftmost tab. Open some additional tabs to the right so that the dropdown button is visible. Then enter the following in the Browser Console:

let popup = document.getElementById("alltabs-popup");
popup.addEventListener("popupshown", function () {
  console.log("firstparty found: " + popup.children[3].getAttribute("firstparty"));
  popup.children[3].setAttribute("image", "https://en.wikipedia.org/favicon.ico");
}, false);

Open the dropdown menu, and you should see the Wikipedia favicon immediately fetched via the SOCKS credential torproject.org:0. And the Wikipedia favicon will appear in the dropdown menu.

comment:10 Changed 4 years ago by mcs

I don't understand how we want this to work. Reading Arthur's description of the manual test in comment:9, it sounds like the first party domain associated with the active tab will be used for all favicon fetches when the menu is opened. And from reading the code plus this patch I get the same impression.

Is that the behavior we want? Or do we want the wikipedia favicon to be isolated as wikipedia.org no matter what tab is active?

Changed 4 years ago by arthuredelstein

Changed 4 years ago by arthuredelstein

comment:11 in reply to:  10 Changed 4 years ago by arthuredelstein

Replying to mcs:

I don't understand how we want this to work. Reading Arthur's description of the manual test in comment:9, it sounds like the first party domain associated with the active tab will be used for all favicon fetches when the menu is opened. And from reading the code plus this patch I get the same impression.

Is that the behavior we want? Or do we want the wikipedia favicon to be isolated as wikipedia.org no matter what tab is active?

The latter behavior is what is intended and what I believe is happening. In browser/base/content/tabbrowser.xml, there is one menuitem defined for each tab. Whenever a tab changes, whether it is active or not, _setMenuitemAttributes is triggered, to copy the image attribute from the tab to the corresponding menuitem. The line I am adding in _setMenuitemAttributes also copies the firstparty attribute from tab to corresponding menuitem.

Here's new version of the demo that sets each image attribute to an URL such as
http://dummyimage.com/16x16/fff/000&text=3.
That URL generates an image with an integer, like this: http://dummyimage.com/16x16/fff/000&text=3.

I opened several tabs with different sites. Here is what the drop down looked like:


Then I entered the following code in the Browser Console:

let popup = document.getElementById("alltabs-popup");
popup.addEventListener("popupshown", function () {
  for (var i = 3; i < popup.children.length; ++i) {
    console.log("firstparty found: " + popup.children[i].getAttribute("firstparty"));
    popup.children[i].setAttribute("image", "http://dummyimage.com/16x16/fff/000&text=" + i);
  }
}, false);

The console output was:

"firstparty found: https://www.wikipedia.org/"
"firstparty found: https://trac.torproject.org/projects/tor"
"firstparty found: https://github.com/"
"firstparty found: http://www.gnu.org/"
"firstparty found: http://elm-lang.org/"
[09-15 18:35:33] Torbutton INFO: tor SOCKS: http://dummyimage.com/16x16/fff/000&text=3 via wikipedia.org:0
[09-15 18:35:33] Torbutton INFO: tor SOCKS: http://dummyimage.com/16x16/fff/000&text=4 via torproject.org:0
[09-15 18:35:33] Torbutton INFO: tor SOCKS: http://dummyimage.com/16x16/fff/000&text=5 via github.com:0
[09-15 18:35:33] Torbutton INFO: tor SOCKS: http://dummyimage.com/16x16/fff/000&text=6 via gnu.org:0
[09-15 18:35:33] Torbutton INFO: tor SOCKS: http://dummyimage.com/16x16/fff/000&text=7 via elm-lang.org:0

And here is the dropdown after the icons loaded (rather slowly):


So each menu item has loaded a different favicon, through the circuit assigned to each tab.

comment:12 Changed 4 years ago by arthuredelstein

To clarify further: the point of the demo is to try to answer Georg's question in comment:7.

But under normal usage, we wouldn't be using a "popupshown" listener to modify the image attribute. Instead, the image attribute is copied from the tab to the menuitem. The menuitem should normally cause no extra image downloads, because the tab has already caused the image to be added to the cache, isolated to the appropriate first party.

Absent this patch, the menuitem does not find the image in the cache, because it is using -No-First-Party-. So it downloads the image again over the -No-First-Party- circuit.

comment:13 Changed 4 years ago by gk

This looks good to me. One thing I was wondering about is that inheriting firstparty is not necessary anymore. What did it (or was supposed to do) in the first place? Or, more direct, why did we need the inheritance there given that the icon of the tab is actually set elsewhere?

comment:14 Changed 4 years ago by mcs

Arthur, thanks for the detailed explanation. Kathy and I understand the code now. But we are seeing an assertion failure in the new code inside ThirdPartyUtil::GetFirstPartyURIInternal() during browser startup with a Mac OS debug build:

Assertion failure: IsElement(), at ../../../dist/include/mozilla/dom/Element.h:1369

I will attach a stack trace, but the short story is that ThirdPartyUtil::GetFirstPartyIsolationURI() is being called from nsContentUtils::LoadImage(). The image URI is moz-anno:favicon:https://www.torproject.org/images/favicon.ico#-moz-resolution=16,16

Maybe you need to add an IsElement() call before calling AsElement()?

Changed 4 years ago by mcs

Attachment: stack.txt added

stack trace for AsElement() assertion failure

comment:15 in reply to:  14 ; Changed 4 years ago by arthuredelstein

Replying to mcs:

Arthur, thanks for the detailed explanation. Kathy and I understand the code now. But we are seeing an assertion failure in the new code inside ThirdPartyUtil::GetFirstPartyURIInternal() during browser startup with a Mac OS debug build:

Assertion failure: IsElement(), at ../../../dist/include/mozilla/dom/Element.h:1369

I will attach a stack trace, but the short story is that ThirdPartyUtil::GetFirstPartyIsolationURI() is being called from nsContentUtils::LoadImage(). The image URI is moz-anno:favicon:https://www.torproject.org/images/favicon.ico#-moz-resolution=16,16

Maybe you need to add an IsElement() call before calling AsElement()?

Thanks for catching this mistake and for the stack trace. For some reason I'm not seeing an assertion failure when I run a debug build on OS X. I wonder what I'm doing differently.

In any case, here's a new version of the patch with an IsElement() check added:
https://github.com/arthuredelstein/tor-browser/commit/16983+1

I tested this on a debug build and it seems to be working properly.

comment:16 in reply to:  15 ; Changed 4 years ago by gk

Replying to arthuredelstein:

Replying to mcs:

Arthur, thanks for the detailed explanation. Kathy and I understand the code now. But we are seeing an assertion failure in the new code inside ThirdPartyUtil::GetFirstPartyURIInternal() during browser startup with a Mac OS debug build:

Assertion failure: IsElement(), at ../../../dist/include/mozilla/dom/Element.h:1369

I will attach a stack trace, but the short story is that ThirdPartyUtil::GetFirstPartyIsolationURI() is being called from nsContentUtils::LoadImage(). The image URI is moz-anno:favicon:https://www.torproject.org/images/favicon.ico#-moz-resolution=16,16

Maybe you need to add an IsElement() call before calling AsElement()?

Thanks for catching this mistake and for the stack trace. For some reason I'm not seeing an assertion failure when I run a debug build on OS X. I wonder what I'm doing differently.

In any case, here's a new version of the patch with an IsElement() check added:
https://github.com/arthuredelstein/tor-browser/commit/16983+1

I tested this on a debug build and it seems to be working properly.

Do you have this branch somewhere? It seems you forget to push it as I can't fetch it nor look at it in the browser.

comment:17 in reply to:  16 Changed 4 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

Replying to mcs:

Arthur, thanks for the detailed explanation. Kathy and I understand the code now. But we are seeing an assertion failure in the new code inside ThirdPartyUtil::GetFirstPartyURIInternal() during browser startup with a Mac OS debug build:

Assertion failure: IsElement(), at ../../../dist/include/mozilla/dom/Element.h:1369

I will attach a stack trace, but the short story is that ThirdPartyUtil::GetFirstPartyIsolationURI() is being called from nsContentUtils::LoadImage(). The image URI is moz-anno:favicon:https://www.torproject.org/images/favicon.ico#-moz-resolution=16,16

Maybe you need to add an IsElement() call before calling AsElement()?

Thanks for catching this mistake and for the stack trace. For some reason I'm not seeing an assertion failure when I run a debug build on OS X. I wonder what I'm doing differently.

In any case, here's a new version of the patch with an IsElement() check added:
https://github.com/arthuredelstein/tor-browser/commit/16983+1

I tested this on a debug build and it seems to be working properly.

Do you have this branch somewhere? It seems you forget to push it as I can't fetch it nor look at it in the browser.

Sorry -- it's pushed now.

comment:18 in reply to:  13 ; Changed 4 years ago by arthuredelstein

Replying to gk:

This looks good to me. One thing I was wondering about is that inheriting firstparty is not necessary anymore. What did it (or was supposed to do) in the first place? Or, more direct, why did we need the inheritance there given that the icon of the tab is actually set elsewhere?

I implemented inheriting firstparty because in both the tab and menuitem case, there is a child image element that is responsible for downloading and displaying the image. The image attributes of tab and menuitem that are set in tabbrowser.xml are copied to the src of the image element. So we also need to propagate the firstparty to that child element. Inheritance seems the most general way; otherwise we have to write code manually to copy the firstparty to the child for each case.

comment:19 in reply to:  18 ; Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to arthuredelstein:

Replying to gk:

This looks good to me. One thing I was wondering about is that inheriting firstparty is not necessary anymore. What did it (or was supposed to do) in the first place? Or, more direct, why did we need the inheritance there given that the icon of the tab is actually set elsewhere?

I implemented inheriting firstparty because in both the tab and menuitem case, there is a child image element that is responsible for downloading and displaying the image. The image attributes of tab and menuitem that are set in tabbrowser.xml are copied to the src of the image element. So we also need to propagate the firstparty to that child element. Inheritance seems the most general way; otherwise we have to write code manually to copy the firstparty to the child for each case.

Sorry, I think you got me wrong (and I totally could see that happening given my clumsy words :( ). To give you the context to my comment:13, I was just wondering about:

-          <xul:image xbl:inherits="src=image,fadein,pinned,selected,busy,crashed,firstparty"
+          <xul:image xbl:inherits="src=image,fadein,pinned,selected,busy,crashed"

Anyway, this is commit 0a287d3bcce4d9e75d1f77af355c19b9292a23c0 on tor-browser-38.3.0esr-5.5-1. I applied it optimistically to tor-browser-38.3.0esr-5.0-2, too (c15e86cd0f082d63d37fddff1efc35a428cb8eca), as this is worthwhile to have in a stable release as well.

comment:20 in reply to:  19 Changed 4 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

Replying to gk:

This looks good to me. One thing I was wondering about is that inheriting firstparty is not necessary anymore. What did it (or was supposed to do) in the first place? Or, more direct, why did we need the inheritance there given that the icon of the tab is actually set elsewhere?

I implemented inheriting firstparty because in both the tab and menuitem case, there is a child image element that is responsible for downloading and displaying the image. The image attributes of tab and menuitem that are set in tabbrowser.xml are copied to the src of the image element. So we also need to propagate the firstparty to that child element. Inheritance seems the most general way; otherwise we have to write code manually to copy the firstparty to the child for each case.

Sorry, I think you got me wrong (and I totally could see that happening given my clumsy words :( ). To give you the context to my comment:13, I was just wondering about:

-          <xul:image xbl:inherits="src=image,fadein,pinned,selected,busy,crashed,firstparty"
+          <xul:image xbl:inherits="src=image,fadein,pinned,selected,busy,crashed"

Sorry to misunderstand your question. :) Before this patch, the xul:image element, which is a child of the tab element, needed to explicitly inherit both the tab's image attribute and its firstparty attribute, in order to download the favicon through the correct circuit. Now, with this patch, the firstparty behavior propagates to children automatically, so we don't need inherits=...firstparty anymore.

Note: See TracTickets for help on using tickets.