Catalin reported this to me over email.

Catalin, can you give me some examples?

Hi, For example I found this 3 games that are not loading with the tracking protection enabled: https://apps.facebook.com/lets_fish/?fb_source=canvas_bookmark https://apps.facebook.com/laststand-deadzone/?fb_source=bookmark&ref=bookmarks&count=0&fb_bmpos=_0 https://apps.facebook.com/deer_hunt_two/?fb_source=canvas_bookmark

One more from cpeterso > 2. Load http://wa.people-roulette.com/peopleroulette/chat.php or https://apps.facebook.com/peopleroulette/chat.php

Francois, can you take a look please?

Looking at the lets_fish game, the following resources get blocked: https://www.googletagservices.com/tag/js/gpt.js https://stats.g.doubleclick.net/dc.js https://s-static.ak.facebook.com/connect/xd_arbiter/6Dg4oLkBbYq.js?version=41#channel=f23ca0fa66dd582&origin=https%3A%2F%2Ffish-fb.apps.tensquaregames.com https://www.facebook.com/connect/ping?client_id=328061820606223&domain=fish-fb.apps.tensquaregames.com&origin=5&redirect_uri=https%3A%2F%2Fs-static.ak.facebook.com%2Fconnect%2Fxd_arbiter%2F6Dg4oLkBbYq.js%3Fversion%3D41%23cb%3Df1972899224820c%26domain%3Dfish-fb.apps.tensquaregames.com%26origin%3Dhttps%253A%252F%252Ffish-fb.apps.tensquaregames.com%252Ff23ca0fa66dd582%26relation%3Dparent&response_type=token%2Csigned_request%2Ccode&sdk=joey https://www.facebook.com/plugins/like.php?app_id=328061820606223&channel=https%3A%2F%2Fs-static.ak.facebook.com%2Fconnect%2Fxd_arbiter%2F6Dg4oLkBbYq.js%3Fversion%3D41%23cb%3Dfef607678b254%26domain%3Dfish-fb.apps.tensquaregames.com%26origin%3Dhttps%253A%252F%252Ffish-fb.apps.tensquaregames.com%252Ff23ca0fa66dd582%26relation%3Dparent.parent&container_width=220&href=https%3A%2F%2Fapps.facebook.com%2Flets_fish%2F&locale=en_US&sdk=joey&send=false&show_faces=false&width=200 https://www.facebook.com/plugins/like_box.php?app_id=328061820606223&channel=https%3A%2F%2Fs-static.ak.facebook.com%2Fconnect%2Fxd_arbiter%2F6Dg4oLkBbYq.js%3Fversion%3D41%23cb%3Df1231e13deccf6c%26domain%3Dfish-fb.apps.tensquaregames.com%26origin%3Dhttps%253A%252F%252Ffish-fb.apps.tensquaregames.com%252Ff23ca0fa66dd582%26relation%3Dparent.parent&container_width=720&header=true&height=700&href=https%3A%2F%2Fwww.facebook.com%2Fpages%2FLets-fish-Community%2F463938473619424&locale=en_US&sdk=joey&show_faces=false&stream=true&width=720 The URL of the page is https://apps.facebook.com/328061820606223/ but it contains an iframe which loads https://fish-fb.apps.tensquaregames.com/sns/common/swf/fisherking_loader.swf:835fee2233129d3ac5fe6db770383215 In order to make this work under Request Policy, I had to whitelist the following: tensquaregames.com -> facebook.com So I guess the problem is that the iframe has tensquaregames.com as a first party and then loads facebook.com which is blocked in third-party contexts.

That makes sense. I think we can loosen our third party checks to fix this problem. From nsChannelClassifier::ShouldEnableTrackingProtection bool isThirdParty = true; (void)thirdPartyUtil->IsThirdPartyChannel(aChannel, nullptr, &isThirdParty); Instead of passing nullptr for the second URI, we should pass the URI of the top window for the channel.

The obvious "fix" which I mention in comment 7 doesn't work, because on first navigation to facebook.com the top window URI is the previous URI. So we need to do this only for non-location bar loads.

Oops, bzexport resets the assignee. Anyway, the patch above is not quite right. It turns out that IsThirdPartyChannel does all sorts of checks with the channel's window that we might want to skip, so comparing the channel URI and the top window URI with IsThirdPartyURI is more straightforward. It works in all cases except for top-window loads, because the window URI at that point is irrelevant. However, I'm not sure checking LOAD_DOCUMENT_INITIAL_URI is quite the right solution. If you try out the patch with NSPR_LOG_MODULES=nsChannelClassifier:5,thirdPartyUtil:5 it skips tracking protection checks on fatratgames on the FB app "Stick Ride". I'm not sure we want to do that. It doesn't seem to be set for all iframes, though -- the test page at http://people.mozilla.org/~mchew/test_tp.html seems to work fine.

Comment on attachment 8578779 [details] [diff] [review] Loosen third-party restrictions for tracking protection checks Review of attachment 8578779 [details] [diff] [review]: ----------------------------------------------------------------- I think the new logic works. The following would be allowed: facebook.com -> game.com (iframe) -> facebook.com (sub-resource) even if facebook.com is on the blocklist. I initially thought that this would incorrectly block same-origin sub-resources in iframes: facebook.com -> game.com (iframe) -> game.com (sub-resource) but in this case, if game.com were on the blocklist, then not only would the sub-resource be blocked, but the whole iframe would be blocked too, so it doesn't matter. ::: netwerk/base/nsChannelClassifier.cpp @@ +81,5 @@ > + // that situation we want to avoid comparing channelURI to the window, since > + // what's in the window will be replaced as the document loads. > + if (flags & nsIChannel::LOAD_INITIAL_DOCUMENT_URI) { > +#ifdef DEBUG > + nsCString topspec; unused variable

Comment on attachment 8580805 [details] [diff] [review] Loosen third-party restrictions for tracking protection checks Review of attachment 8580805 [details] [diff] [review]: ----------------------------------------------------------------- Just to be clear, this patch changes TP so that: 1. Top-level loads are ignored (via the flag LOAD_INITIAL_DOCUMENT_URI) 2. Third party loads are checked; a URI is considered third party if it does not share the same base domain as the top-level window load. Looks fine, but can you check what happens if a link is in an iframe and the load is targeted? Is LOAD_INITIAL_DOCUMENT_URI set there as well, or is that really only for the top-level window load? It's not clear to me from nsIChannel.idl whether it's just for top-level loads. ::: dom/base/ThirdPartyUtil.cpp @@ +181,5 @@ > ThirdPartyUtil::IsThirdPartyChannel(nsIChannel* aChannel, > nsIURI* aURI, > bool* aResult) > { > + LOG(("ThirdPartyUtil::IsThirdPartyChannel")); nit: These logs don't have a unique ID to match them. Can you add in a %p for aChannel.get() or something like that? It'll be easier for scanning logs later.

Comment on attachment 8581942 [details] [diff] [review] Loosen third-party restrictions for tracking protection checks Review of attachment 8581942 [details] [diff] [review]: ----------------------------------------------------------------- This avoids use of LOAD_INITIAL_DOCUMENT_URI, which was triggering for some(?) iframes. It is ugly, but it seems to work. There is some extraneous logging in the ThirdPartyUtil file that I will remove.

Comment on attachment 8581942 [details] [diff] [review] Loosen third-party restrictions for tracking protection checks Review of attachment 8581942 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsChannelClassifier.cpp @@ +99,5 @@ > + rv = aChannel->GetURI(getter_AddRefs(chanURI)); > + NS_ENSURE_SUCCESS(rv, rv); > + thirdPartyUtil->IsThirdPartyURI(chanURI, topWinURI, &isThirdParty); > + bool isThirdPartyChannel = true; > + thirdPartyUtil->IsThirdPartyChannel(aChannel, nullptr, &isThirdPartyChannel); The logic for IsThirdPartyURI seems to be included in IsThirdPartyChannel if you pass in a uri rather than nullptr. Am I missing something about the logic that means you need to call IsThirdPartyURI as well?

The only reason to call IsThirdPartyChannel is to detect the case where it's a toplevel page load (and the call to IsThirdPartyURI is invoked with the old window's URI). I tried fixing this with LOAD_DOCUMENT_INITIAL_URI and friends but it turns out that IsThirdPartyChannel already does these checks. If there's a cleaner way to do it, please let me know.

Basically, when looking at IsThirdPartyChannel, it looks like passing in topWinURI might work: thirdPartyUtil->IsThirdPartyChannel(aChannel, topWinURI, &isThirdPartyChannel); But if you've tried it and it doesn't fit this use case, then I'll approve it :)

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #21) > Basically, when looking at IsThirdPartyChannel, it looks like passing in > topWinURI might work: > > thirdPartyUtil->IsThirdPartyChannel(aChannel, topWinURI, > &isThirdPartyChannel); > > But if you've tried it and it doesn't fit this use case, then I'll approve > it :) Unfortunately it doesn't work for top-level loads, because topWinURI is stale :( That's why I was mucking about with the LOAD_DOCUMENT_URI flags.

Also, it obviously needs better comments.

Comment on attachment 8581942 [details] [diff] [review] Loosen third-party restrictions for tracking protection checks Review of attachment 8581942 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Monica! ::: netwerk/base/nsChannelClassifier.cpp @@ +93,5 @@ > } > > + // Third party checks don't work for chrome:// URIs in mochitests, so just > + // default to isThirdParty = true > + bool isThirdParty = true; s/isThirdParty/isThirdPartyURI/ just to differentiate from isThirdPartyChannel.

Yes, thanks. Matt, please play lots of Facebook games when you QA this, and check that no facebook domains appear in the security console as "The resource at <blah> is blocked because tracking protection is enabled".