Closed Bug 1242472 Opened 4 years ago Closed 4 years ago

Tracking protection: Tracking pixel not blocked in e10s

Categories

(Toolkit :: Safe Browsing, defect)

44 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
e10s m8+ ---
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: mwobensmith, Assigned: mrbkap)

References

()

Details

Attachments

(2 files, 1 obsolete file)

On cnn.com's home page, a tracking pixel from a domain on the Disconnect blocklist loads, but only when e10s is enabled.

This appears to be the behavior from Fx42 onwards. 

If e10s is disabled, the resource is correctly blocked.

Resource URL:
http://b.scorecardresearch.com/r?c2=6035748&d.c=gif&d.o=cnn-adbp-intl&d.x=38362791&d.t=page&d.u=http%3A%2F%2Fedition.cnn.com%2F
Assignee: francois → nobody
Blocks: 1029886
Assignee: nobody → mrbkap
tracking-e10s: --- → m8+
Attached file jsmd.beautified.js
I spent a few hours debugging this with NSPR_LOG_MODULES="nsHttp:5" and also looking at the Initiator column in the Chrome devtools.

I found that the offending gif from scorecardresearch is the result of a 302:

[0x7fbf7776a340]: I/nsHttp http response [
[0x7fbf7776a340]: I/nsHttp   HTTP/1.1 302 Found
[0x7fbf7776a340]: I/nsHttp   Date: Mon, 25 Jan 2016 18:57:06 GMT
[0x7fbf7776a340]: I/nsHttp   Server: Omniture DC/2.0.0
[0x7fbf7776a340]: I/nsHttp   Access-Control-Allow-Origin: *
[0x7fbf7776a340]: I/nsHttp   Set-Cookie: s_vi=[CS]v1|2B533801051D3D22-4000190F8000197D[CE]; Expires=Wed, 24 Jan 2018 18:57:06 GMT; Domain=cnn.com; Path=/
[0x7fbf7776a340]: I/nsHttp   X-C: ms-4.9.10
[0x7fbf7776a340]: I/nsHttp   Expires: Sun, 24 Jan 2016 18:57:06 GMT
[0x7fbf7776a340]: I/nsHttp   Last-Modified: Tue, 26 Jan 2016 18:57:06 GMT
[0x7fbf7776a340]: I/nsHttp   Cache-Control: no-cache, no-store, max-age=0, no-transform, private
[0x7fbf7776a340]: I/nsHttp   Pragma: no-cache
[0x7fbf7776a340]: I/nsHttp   Etag: "56A67002-9294-1E2388CD"
[0x7fbf7776a340]: I/nsHttp   Vary: *
[0x7fbf7776a340]: I/nsHttp   P3P: policyref="/w3c/p3p.xml", CP="NOI DSP COR NID PSA OUR IND COM NAV STA"
[0x7fbf7776a340]: I/nsHttp   Location: http://b.scorecardresearch.com/r?c2=6035748&d.c=gif&d.o=cnn-adbp-domestic&d.x=48376774&d.t=page&d.u=http%3A%2F%2Fwww.cnn.com%2F
[0x7fbf7776a340]: I/nsHttp   xserver: www931
[0x7fbf7776a340]: I/nsHttp   Content-Length: 0
[0x7fbf7776a340]: I/nsHttp   Keep-Alive: timeout=15
[0x7fbf7776a340]: I/nsHttp   Connection: Keep-Alive
[0x7fbf7776a340]: I/nsHttp   Content-Type: text/plain
[0x7fbf7776a340]: I/nsHttp ]

I'm not 100% sure, but I think the source of that 302 is another 302:

[0x7fbf7776a340]: D/nsHttp nsHttpTransaction::HandleContentStart [this=7fbf3e721000]
[0x7fbf7776a340]: I/nsHttp http response [
[0x7fbf7776a340]: I/nsHttp   HTTP/1.1 302 Found
[0x7fbf7776a340]: I/nsHttp   Date: Mon, 25 Jan 2016 18:57:06 GMT
[0x7fbf7776a340]: I/nsHttp   Server: Omniture DC/2.0.0
[0x7fbf7776a340]: I/nsHttp   Access-Control-Allow-Origin: *
[0x7fbf7776a340]: I/nsHttp   Set-Cookie: s_vi=[CS]v1|2B533801051D3D22-4000190F8000197D[CE]; Expires=Wed, 24 Jan 2018 18:57:06 GMT; Domain=cnn.com; Path=/
[0x7fbf7776a340]: I/nsHttp   Location: http://metrics.cnn.com/b/ss/cnn-adbp-domestic/1/H.26.1/s61817340381443?AQB=1&pccr=true&vidn=2B533801051D3D22-4000190F8000197D&&ndh=1&t=25%2F0%2F2016%2010%3A57%3A5%201%20480&fid=1813C5000C9ECF0C-3B9B8DB4EACA3F44&ce=UTF-8&ns=cnn&pageName=cnn%3Ain%3A%2F&g=http%3A%2F%2Fwww.cnn.com%2F&cc=USD&ch=cnn%20homepage&server=cnn.com&events=event26&c5=nvs&v5=D%3Dc5&c8=new%3A1&v8=D%3Dc8&c9=nvs&v9=D%3Dc9&c13=section&v13=D%3Dc13&c17=anonymous&v17=D%3Dc17&c20=11&v20=D%3Dc20&v22=0&c25=nvs&v25=D%3Dc25&c26=www.cnn.com%2F&v26=D%3DpageName&v27=D%3Dch&c28=cnn%20homepage%3Anvs&v28=D%3Dc28&v29=cnn.com&c30=cnn%20domestic&v30=D%3Dc30&c32=adbp%3Aindex&v32=D%3Dc32&c33=none&v33=D%3Dc33&c35=cnnexpan.126.5518.20160112%3A0&v35=D%3Dc35&c37=desktop&v37=D%3Dc37&c46=14537482252292655977683703&v46=D%3Dc46&c47=56a670000d1a180a3c8ef73f32008596&v47=D%3Dc47&c56=portrait&v56=D%3Dc56&c64=cnn%20news&v64=D%3Dc64&c65=nvs&c73=956%20X%201086&v73=D%3Dc73&h1=news%7Ccnn%7Ccnn%20domestic%7Ccnn.com%7Ccnn%20homepage%7Ccnn%20homepage%3Anvs&l1=D%3Dc65&s=1920x1200&c=24&j=1.8.5&v=N&k=Y&bw=956&bh=1086&AQE=1
[0x7fbf7776a340]: I/nsHttp   X-C: ms-4.9.10
[0x7fbf7776a340]: I/nsHttp   Expires: Sun, 24 Jan 2016 18:57:06 GMT
[0x7fbf7776a340]: I/nsHttp   Last-Modified: Tue, 26 Jan 2016 18:57:06 GMT
[0x7fbf7776a340]: I/nsHttp   Cache-Control: no-cache, no-store, max-age=0, no-transform, private
[0x7fbf7776a340]: I/nsHttp   Pragma: no-cache
[0x7fbf7776a340]: I/nsHttp   P3P: policyref="/w3c/p3p.xml", CP="NOI DSP COR NID PSA OUR IND COM NAV STA"
[0x7fbf7776a340]: I/nsHttp   xserver: www924
[0x7fbf7776a340]: I/nsHttp   Content-Length: 0
[0x7fbf7776a340]: I/nsHttp   Keep-Alive: timeout=15
[0x7fbf7776a340]: I/nsHttp   Connection: Keep-Alive
[0x7fbf7776a340]: I/nsHttp   Content-Type: text/plain
[0x7fbf7776a340]: I/nsHttp ]

I couldn't actually find the source of that one but I found metrics.cnn.com in none of the CSS or HTML files loaded from the CNN homepage and only in a single JS file. It appears several times in
http://z.cdn.turner.com/analytics/cnnexpan/jsmd.min.js which is loaded by a script element on the CNN homepage:

<script src="http://z.cdn.turner.com/analytics/cnnexpan/jsmd.min.js"></script>

Attached is the version of jsmd.min.js that I ran through http://jsbeautifier.org/.
I noticed this when debugging.

Christoph, I hope you don't mind reviewing this. I don't know of a better reviewer.
Attachment #8713439 - Flags: review?(mozilla)
Attached patch FixSplinter Review
I haven't written a test for this yet, but comment 1 is right on – we lose our top window URI somewhere in the redirect chain and the tracking protection code bails. I wonder if the topWindowURI is something that we should stick on the loadinfo instead of on the channel.
Attachment #8713443 - Flags: review?(mozilla)
Comment on attachment 8713439 [details] [diff] [review]
Add missing newlines to log messages

Review of attachment 8713439 [details] [diff] [review]:
-----------------------------------------------------------------

The Channel Classifier is part of the URL Classifier (i.e. Safe Browsing) module, so I'm happy to review these for you.

Does the newline actually matter? If so, I'm gonna have to change a lot more of these because I've never put newlines in my PR_LOG messages either :(
Attachment #8713439 - Flags: review?(mozilla) → review+
Comment on attachment 8713443 [details] [diff] [review]
Fix

Review of attachment 8713443 [details] [diff] [review]:
-----------------------------------------------------------------

The changes look fine, and I agree, we should move the topWindowURI in the loadInfo at some point. Seems like a better fit, especially since we are already having the windowID in the loadInfo. The pixel tracking is apparently only an issue for http channels, right? Just want to make sure, because there is also the redirect within nsBaseChannel where we manually have to propagate some of the flags needed for the new channel [1].

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsBaseChannel.cpp#73
Attachment #8713443 - Flags: review?(mozilla) → review+
(In reply to François Marier [:francois] from comment #4)
> Does the newline actually matter? If so, I'm gonna have to change a lot more
> of these because I've never put newlines in my PR_LOG messages either :(

The logging code will add the newline for you if you don't have one.  (When we switched some things over to mozilla::LazyLogModule, we forgot the newline behavior, but we quickly corrected it...)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> apparently only an issue for http channels, right? Just want to make sure,
> because there is also the redirect within nsBaseChannel where we manually
> have to propagate some of the flags needed for the new channel [1].

Yeah, we only have mTopWindowURI on HttpBaseChannel so there's nothing to propagate in the other SetupRedirectChannel impl.

(In reply to François Marier [:francois] from comment #4)
> Does the newline actually matter? If so, I'm gonna have to change a lot more
> of these because I've never put newlines in my PR_LOG messages either :(

I was misreading the existing logging output, I've deleted this changeset locally.
Attachment #8713439 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/fc1de8384bdd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Is this still an m8 blocker? Should it uplift to 46? Thanks.
Flags: needinfo?(mrbkap)
Comment on attachment 8713443 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Users with tracking protection enabled will still be able to be tracked.
[Describe test coverage new/current, TreeHerder]: The feature has a bunch of tests on treeherder.
[Risks and why]: Low risk, propagates 
[String/UUID change made/needed]: n/a
Flags: needinfo?(mrbkap)
Attachment #8713443 - Flags: approval-mozilla-aurora?
Comment on attachment 8713443 [details] [diff] [review]
Fix

Please uplift to aurora; we'd like to make sure e10s users for 46 have tracking protection working.
Attachment #8713443 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.