Closed Bug 1485532 Opened Last year Closed Last year

Tracking Protection does not work if you disable uBlock Origin through its power button

Categories

(Core :: Networking: HTTP, defect, P2)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 --- verified
firefox64 --- verified

People

(Reporter: darkspirit, Assigned: ehsan)

References

()

Details

(Keywords: nightly-community, regression, Whiteboard: [necko-triaged])

Attachments

(3 files)

1. Tracking Protection on spiegel.de: Click on an article: We don't see ads and get their Adblock overlay.
2. Install uBlock Origin
3. Reload the article. uBlock blocks multiple items. Click on uBlock's icon, on the blue Power button and on the Reload button.
4. It's good if we see their Adblock overlay, it's bad if we see tracking ads and no Tracking Protection icon.

mozregression --good 2018-08-21 --bad 20180822221004 --pref privacy.trackingprotection.enabled:true browser.fastblock.enabled:true -a http://www.spiegel.de/ -a https://addons.mozilla.org/firefox/addon/ublock-origin/
> 10:27.36 INFO: Last good revision: 3b7a5715e699075b00ec90ad524b40a52202e059
> 10:27.36 INFO: First bad revision: 48213c5c25e9aa9389375e4e71c1a58567d37ca2
> 10:27.36 INFO: Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3b7a5715e699075b00ec90ad524b40a52202e059&tochange=48213c5c25e9aa9389375e4e71c1a58567d37ca2

> 48213c5c25e9	Ehsan Akhgari — Bug 1485182 - Part 1: Run http-on-modify-request observers when tracking protection cancels an HTTP channel; r=mayhemer
Flags: needinfo?(ehsan)
Hmm...

Just so that I understand correctly, this was working correctly *before* bug 1485182 landed, and broke when that landed?
Flags: needinfo?(ehsan)
Flags: needinfo?(jan)
Attached video 2018-08-23_02-51-39.mp4
Yes, bug 1485182 fixed bug 1484713, but introduced this one.
You can see both the fix (loading the with uBlock enabled is fast again) and the new regression in this video.

last good (left): mozregression --repo mozilla-inbound --launch 3b7a5715e699075b00ec90ad524b40a52202e059 --pref privacy.trackingprotection.enabled:true browser.fastblock.enabled:true -a http://www.spiegel.de/ -a https://addons.mozilla.org/firefox/addon/ublock-origin/

first bad (right): mozregression --repo mozilla-inbound --launch 48213c5c25e9aa9389375e4e71c1a58567d37ca2 --pref privacy.trackingprotection.enabled:true browser.fastblock.enabled:true -a http://www.spiegel.de/ -a https://addons.mozilla.org/firefox/addon/ublock-origin/
Flags: needinfo?(jan)
*loading the page
I get different situation that Tracking Protection doesn't work at all when uBlock Origin is enabled.
(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #5)
> (In reply to Toshihiro Yamada from comment #4)
> > I get different situation that Tracking Protection doesn't work at all when uBlock Origin is enabled.
> 
> That has been always the case, if I'm not wrong, because TP ran after uBlock and there was nothing left to block.

Yeah, if I uncheck all filter lists except for "Badware risks", reload the page, I see ads as well.
Thanks for the report!  I did some investigation to figure out what's going on here.  First, I looked at what uBlock Origin does when you turn off the extension for a site.

Its onBeforeRequest handler will run this code: <https://github.com/gorhill/uBlock/blob/3a78e73e4b07f5a010f41445fde93da10e0e2a4d/src/js/traffic.js#L157>.  filterRequest() will bail out early here by returning 0 <https://github.com/gorhill/uBlock/blob/51a4e9ccf4c0262baf13353ef325a73922992876/src/js/pagestore.js#L599> and as a result, onBeforeRequest returns undefined here: <https://github.com/gorhill/uBlock/blob/3a78e73e4b07f5a010f41445fde93da10e0e2a4d/src/js/traffic.js#L180>.

This is a bit puzzling to me, as our code, if I'm reading it right, upon seeing an undefined return value from the extension, should only resume the channel, I think: <https://searchfox.org/mozilla-central/rev/f2ac80ab7dbde5400a3400d463e07331194dec94/toolkit/modules/addons/WebRequest.jsm#789>

The good news is that I wrote a small test case very similar to browser_onModifyRequestNotificationForTrackingResources.js using the webRequest API, and I can easily reproduce the bug with the small test case.  Debugging it now to see what's causing the problem...
Assignee: nobody → ehsan
Priority: -- → P2
Whiteboard: [necko-triaged]
So there are at least two problems here:

1) Immediately after OnTrackerFound(), the channel callback of the classifier is called: <https://searchfox.org/mozilla-central/rev/f2ac80ab7dbde5400a3400d463e07331194dec94/netwerk/base/nsChannelClassifier.cpp#1220>.  This callback here calls BeginConnectActual(): <https://searchfox.org/mozilla-central/rev/f2ac80ab7dbde5400a3400d463e07331194dec94/netwerk/protocol/http/nsHttpChannel.cpp#6451>.  At this point, mCallOnResume is set to nsHttpChannel::HandleContinueCancelledByTrackingProtection(), but BeginConnectActual() doesn't know to wait for that, so it will just happily continue to go to the network, effectively bypassing the tracking protection classification that just happened.  This is the main root cause of this bug.

2) Once you fixed that bug, a canceled channel would fail to notify its OnStopRequest listener because AsyncAbort() would never be called for it, so it would continually sit in the loading state.
The first part adds a test that passes without the fix, since it doesn't actually check that anything is blocked by TP.  The second part fixes the problems in comment 8, and adds a check to both this test and the one added originally in bug 1485182 to ensure we're checking that things are getting blocked by TP (even though they would be in the other case, even without these fixes).

Note that these patches need to land together with bug 1485673.  Landing bug 1485673 without will cause uBO disabled + TP to fall into a case where requests never complete because of the missing call to AsyncAbort(), which makes the browser rather unusable!
Blocks: 1485673
Attachment #9003614 - Flags: review?(honzab.moz) → review+
Comment on attachment 9003615 [details] [diff] [review]
Part 2: Make sure that tracking requests that webRequest.onBeforeRequest handlers do not handle will still get blocked by tracking protection

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

:(  http channel is getting messier and messier...  please, no offense, this is a legacy code issue.

yes, this will work.  sorry, Ehsan, I didn't realize we need to do asyncAbort in this phase of channel opening (there is no mCachePump or mTransactionPump to cancel; cancelling them triggers OnStart/OnStop with the given status code; otherwise, we need asyncAbort - we should definitely make this more implicit)

and I also forgot that after canceling the channel (which your patches make pending/async - leaving mStatus == NS_OK) we continue with opening the channel.  there are so many code paths I'm sometimes loosing track myself.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +627,5 @@
>  
>      // the next authentication request can be sent on a whole new connection
>      uint32_t                          mAuthConnectionRestartable : 1;
>  
> +    // the tracking protection cancellation is pending

a more rich comment please.  a comment copying a member name is not a good comment.
Attachment #9003615 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #12)
> :(  http channel is getting messier and messier...  please, no offense, this
> is a legacy code issue.

None taken.  I share your concern.  :-(  Unfortunately I don't have any good suggestions for improving things.  I tried hard to not make things too much worse.

> yes, this will work.  sorry, Ehsan, I didn't realize we need to do
> asyncAbort in this phase of channel opening (there is no mCachePump or
> mTransactionPump to cancel; cancelling them triggers OnStart/OnStop with the
> given status code; otherwise, we need asyncAbort - we should definitely make
> this more implicit)

I see, thanks for the explanation!  I was wondering what are the conditions where AsyncAbort() has to be called...

> and I also forgot that after canceling the channel (which your patches make
> pending/async - leaving mStatus == NS_OK) we continue with opening the
> channel.  there are so many code paths I'm sometimes loosing track myself.

No worries, I was following the code execution under rr and I still managed to miss that when going over this the first time!
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ac05aafbd6
Part 1: Add a test case for the interaction of tracking protection and the webRequest onBeforeRequest API; r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f84be4d22bf
Part 2: Make sure that tracking requests that webRequest.onBeforeRequest handlers do not handle will still get blocked by tracking protection; r=mayhemer
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b5cae2a7a3
follow-up: Fix lint failures on a CLOSED TREE
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab24a5b6cd0a
follow-up: Update the comments in the test, DONTBUILD
Verified fixed in Nightly 63 x64 20180825100331 de_DE @ Debian Testing. Thank you! :)
Status: RESOLVED → VERIFIED
Thanks for verifying, Jan!
I managed to reproduce the issue with an older version of Nightly (2018-08-22) on Windows 10 x64.
I retested everything using latest Nightly 64.0a1 beta 63.0b11 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13. The bug is not reproducing anymore.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.