1485182 - Ad blocker add-ons can no longer block resources blocked by TP

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[(no longer active)]

I found this regression when I was thinking about bug 1484713.

The change of behavior is that previously we would be calling http-on-modify-request observers before the channel classifier runs, but now we are calling them after.  This means that when tracking protection causes the channels to be canceled, the http-on-modify-request observers aren't run on them any more!

Add-ons like ad-blockers use these observers in the form of the webRequest API to intercept and block HTTP requests, so as a result of this, uBlock Origin on https://www.kino.de/ for example only finds 11 elements with TP turned on.  After fixing this bug, it finds 25 elements.

Comment 1

[(no longer active)]

Attachment: Run http-on-modify-request observers when tracking protection cancels an HTTP channel

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 9002945 [details] [diff] [review]
Run http-on-modify-request observers when tracking protection cancels an HTTP channel

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

Good catch!

I only don't like the recursion and honestly splitting Cancel() this way is not the best way, IMO.  The flow should be a bit different way, see the inline comments. 

Or, we should have a special method on the channel that the classifier would call to cancel the channel from tracking-protection reasons and the channel would do the job to notify+wait+cancel correctly.  I would prefer that rather than hacking Cancel().

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6051,5 @@
> +      // done in PrepareToConnect()).  So do that now, before proceeding to
> +      // cancel.
> +      //
> +      // Note that running these observers can itself result in the channel
> +      // being canceled.  In that case, we accept that cancelation code as

and when called with NS_ERROR_TRACKING_URI we have a nice stack overflow

@@ +6056,5 @@
> +      // the cause of the cancelation, as if the classification of the channel
> +      // would have occurred past this point!
> +
> +      // notify "http-on-modify-request" observers
> +      CallOnModifyRequestObservers();

at this place I would instead do the following:
- Suspend() this channel
- dispatch a runnable to the main thread that will:
  - CallOnModifyRequestObservers()
  - Resume() this channel
  - if mCanceled -> return
  - if mSuspended -> assign mCallOnResume as you do and return
- return NS_OK

@@ +6058,5 @@
> +
> +      // notify "http-on-modify-request" observers
> +      CallOnModifyRequestObservers();
> +
> +      SetLoadGroupUserAgentOverride();

why?

@@ +6062,5 @@
> +      SetLoadGroupUserAgentOverride();
> +
> +      // Check if request was cancelled during on-modify-request or on-useragent.
> +      if (mCanceled) {
> +          return mStatus;

why not NS_OK?

@@ +6091,5 @@
> +        mRequestContext->CancelTailedRequest(this);
> +        CloseCacheEntry(false);
> +        Unused << AsyncAbort(status);
> +    }
> +    return CancelInternal(status);

omission?  CancelInternal is just doing what the block above is.

@@ +6112,5 @@
> +    return CancelInternal(NS_ERROR_TRACKING_URI);
> +}
> +
> +nsresult
> +nsHttpChannel::CancelInternal(nsresult status) {

{ on a new line

Comment 3

[(no longer active)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Good catch!
> 
> I only don't like the recursion and honestly splitting Cancel() this way is
> not the best way, IMO.  The flow should be a bit different way, see the
> inline comments. 
> 
> Or, we should have a special method on the channel that the classifier would
> call to cancel the channel from tracking-protection reasons and the channel
> would do the job to notify+wait+cancel correctly.  I would prefer that
> rather than hacking Cancel().
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +6051,5 @@
> > +      // done in PrepareToConnect()).  So do that now, before proceeding to
> > +      // cancel.
> > +      //
> > +      // Note that running these observers can itself result in the channel
> > +      // being canceled.  In that case, we accept that cancelation code as
> 
> and when called with NS_ERROR_TRACKING_URI we have a nice stack overflow

Well, NS_ERROR_TRACKING_URI is a reserved error code and no code besides the channel classifier should be canceling channels with that error code.  If that stops being true some day, we have much bigger problems than a stack overflow to worry about.  ;-)

(In fact, that we don't currently protect against this somehow is rather scary!)

> @@ +6056,5 @@
> > +      // the cause of the cancelation, as if the classification of the channel
> > +      // would have occurred past this point!
> > +
> > +      // notify "http-on-modify-request" observers
> > +      CallOnModifyRequestObservers();
> 
> at this place I would instead do the following:
> - Suspend() this channel
> - dispatch a runnable to the main thread that will:
>   - CallOnModifyRequestObservers()
>   - Resume() this channel
>   - if mCanceled -> return
>   - if mSuspended -> assign mCallOnResume as you do and return
> - return NS_OK

I'm happy to do this, because you're asking me to.  But just to document this on the bug at least, this *will* be changing behavior slightly compared to before bug 1478539 landing, since before that bug http-on-modify-request shouldn't (at least normally) be dispatched on a suspended channel (unless if it were suspended by a previous observer).  Now I guess the question is whether some code somewhere would depend on the suspended state of a channel (which nsIRequest sadly exposes through nsIRequest::IsPending()).

A cursory look through the current consumers in the tree suggests that no current consumer in that notification depends on isPending(), and https://searchfox.org/mozilla-central/source/toolkit/components/extensions/webrequest/ChannelWrapper.cpp isn't calling that method either, so this setup will hopefully work now...

> @@ +6058,5 @@
> > +
> > +      // notify "http-on-modify-request" observers
> > +      CallOnModifyRequestObservers();
> > +
> > +      SetLoadGroupUserAgentOverride();
> 
> why?

This one dispatches http-on-useragent-request, similarly to http-on-modify-request.  There is the exact same regression about this notification as well (since these two are dispatched right after one another).  I decided to fix both, even though on-useragent-request *technically* isn't required for fixing bug 1484713.

> @@ +6062,5 @@
> > +      SetLoadGroupUserAgentOverride();
> > +
> > +      // Check if request was cancelled during on-modify-request or on-useragent.
> > +      if (mCanceled) {
> > +          return mStatus;
> 
> why not NS_OK?

I've already answered this question in my code comments: :-)

+      // Note that running these observers can itself result in the channel
+      // being canceled.  In that case, we accept that cancelation code as
+      // the cause of the cancelation, as if the classification of the channel
+      // would have occurred past this point!

Basically, this is so that we preserve the old behavior.

> @@ +6091,5 @@
> > +        mRequestContext->CancelTailedRequest(this);
> > +        CloseCacheEntry(false);
> > +        Unused << AsyncAbort(status);
> > +    }
> > +    return CancelInternal(status);
> 
> omission?  CancelInternal is just doing what the block above is.

Oops, I meant to have deleted that block above!  :-/

I'll submit a new patch with the new approach.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to :Ehsan Akhgari from comment #3)
> (In reply to Honza Bambas (:mayhemer) from comment #2)
> > Good catch!
> > 
> > I only don't like the recursion and honestly splitting Cancel() this way is
> > not the best way, IMO.  The flow should be a bit different way, see the
> > inline comments. 
> > 
> > Or, we should have a special method on the channel that the classifier would
> > call to cancel the channel from tracking-protection reasons and the channel
> > would do the job to notify+wait+cancel correctly.  I would prefer that
> > rather than hacking Cancel().
> > 
> > ::: netwerk/protocol/http/nsHttpChannel.cpp
> > @@ +6051,5 @@
> > > +      // done in PrepareToConnect()).  So do that now, before proceeding to
> > > +      // cancel.
> > > +      //
> > > +      // Note that running these observers can itself result in the channel
> > > +      // being canceled.  In that case, we accept that cancelation code as
> > 
> > and when called with NS_ERROR_TRACKING_URI we have a nice stack overflow
> 
> Well, NS_ERROR_TRACKING_URI is a reserved error code and no code besides the
> channel classifier should be canceling channels with that error code.  If
> that stops being true some day, we have much bigger problems than a stack
> overflow to worry about.  ;-)

OK, good to know!  I just want zero-possibility of any crash-like issues here.

> 
> (In fact, that we don't currently protect against this somehow is rather
> scary!)

You are adding a code that can directly do that :)  Not sure what you mean about a protection otherwise.

> 
> > @@ +6056,5 @@
> > > +      // the cause of the cancelation, as if the classification of the channel
> > > +      // would have occurred past this point!
> > > +
> > > +      // notify "http-on-modify-request" observers
> > > +      CallOnModifyRequestObservers();
> > 
> > at this place I would instead do the following:
> > - Suspend() this channel
> > - dispatch a runnable to the main thread that will:
> >   - CallOnModifyRequestObservers()
> >   - Resume() this channel
> >   - if mCanceled -> return
> >   - if mSuspended -> assign mCallOnResume as you do and return
> > - return NS_OK
> 
> I'm happy to do this, because you're asking me to.  

A dedicated method would be even better.

Either of those approaches are clearer and (more) correct.

OTOH, we always cancel channels from TP reasons before we open any of the pipes (http transaction or cache).  Hence, suspending the channel is probably going to be ineffective.  Still, I'd rather do it for correctness reasons - after Cancel() there MUST not be any further On* stream listener notification except OnStopRequest with the error code.

> But just to document
> this on the bug at least, this *will* be changing behavior slightly compared
> to before bug 1478539 landing, since before that bug http-on-modify-request
> shouldn't (at least normally) be dispatched on a suspended channel (unless
> if it were suspended by a previous observer).  Now I guess the question is
> whether some code somewhere would depend on the suspended state of a channel
> (which nsIRequest sadly exposes through nsIRequest::IsPending()).
> 
> A cursory look through the current consumers in the tree suggests that no
> current consumer in that notification depends on isPending(), and
> https://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> webrequest/ChannelWrapper.cpp isn't calling that method either, so this
> setup will hopefully work now...


Is pending is true between AsyncOpen (inclusive) and OnStopRequest (exclusive).  It will be true at this stage regardless suspend count.  There is no change to it.

There is no rule for the channel suspend state count when calling http-on-modify-request and it should never be.  It can be in whatever state, just before OnStartRequest.

> 
> > @@ +6058,5 @@
> > > +
> > > +      // notify "http-on-modify-request" observers
> > > +      CallOnModifyRequestObservers();
> > > +
> > > +      SetLoadGroupUserAgentOverride();
> > 
> > why?
> 
> This one dispatches http-on-useragent-request, similarly to
> http-on-modify-request.  There is the exact same regression about this
> notification as well (since these two are dispatched right after one
> another).  I decided to fix both, even though on-useragent-request
> *technically* isn't required for fixing bug 1484713.

Good.  Maybe just have comments here?  Tho, I'm not sure what effect can SetLoadGroupUserAgentOverride have on a canceled channel.

> 
> > @@ +6062,5 @@
> > > +      SetLoadGroupUserAgentOverride();
> > > +
> > > +      // Check if request was cancelled during on-modify-request or on-useragent.
> > > +      if (mCanceled) {
> > > +          return mStatus;
> > 
> > why not NS_OK?
> 
> I've already answered this question in my code comments: :-)

I had to miss that :)  A comment right above that like would be better ;)  still, this is now moot, if you agree on the async approach.

> 
> +      // Note that running these observers can itself result in the channel
> +      // being canceled.  In that case, we accept that cancelation code as
> +      // the cause of the cancelation, as if the classification of the
> channel
> +      // would have occurred past this point!
> 
> Basically, this is so that we preserve the old behavior.
> 
> > @@ +6091,5 @@
> > > +        mRequestContext->CancelTailedRequest(this);
> > > +        CloseCacheEntry(false);
> > > +        Unused << AsyncAbort(status);
> > > +    }
> > > +    return CancelInternal(status);
> > 
> > omission?  CancelInternal is just doing what the block above is.
> 
> Oops, I meant to have deleted that block above!  :-/
> 
> I'll submit a new patch with the new approach.

Thank you!

Comment 5

[Honza Bambas (:mayhemer)]

Note that the async approach may have some perf regressing outcomes.  Doing one more loop before we do AsyncAbort and call OnStopRequest may delay some pending operations.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Note that the async approach may have some perf regressing outcomes.  Doing
> one more loop before we do AsyncAbort and call OnStopRequest may delay some
> pending operations.

In other words, if classifiers could call a specific method that can't reenter, then I would be fine with a sync approach.

Comment 7

[(no longer active)]

Attachment: Part 1: Run http-on-modify-request observers when tracking protection cancels an HTTP channel

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 9003187 [details] [diff] [review]
Part 1: Run http-on-modify-request observers when tracking protection cancels an HTTP channel

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

thanks!

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6034,5 @@
> +    }
> +
> +    if (mWaitingForRedirectCallback) {
> +        LOG(("channel canceled during wait for redirect callback"));
> +    }

I don't think this may ever occur.

@@ +6045,5 @@
> +    //
> +    // Note that running these observers can itself result in the channel
> +    // being canceled.  In that case, we accept that cancelation code as
> +    // the cause of the cancelation, as if the classification of the channel
> +    // would have occurred past this point!

nit: can we assert here that we still keep ref to the classifier?  it may help identifying that someone is misusing this method.

please add MOZ_ASSERT(!mRequestObserversCalled);  

(maybe that could be added to https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/netwerk/protocol/http/HttpBaseChannel.h#453, but rather in a followup)

and maybe file a followup (or use the same followup) to add MOZ_ASSERT(mRequestObserversCalled); to OnStartRequest?  that would catch this bug!

@@ +6068,5 @@
> +    return CancelInternal(NS_ERROR_TRACKING_URI);
> +}
> +
> +nsresult
> +nsHttpChannel::ContinueCancelledByTrackingProtection()

looks like this can return void.

@@ +6085,5 @@
> +    return CancelInternal(NS_ERROR_TRACKING_URI);
> +}
> +
> +nsresult
> +nsHttpChannel::CancelInternal(nsresult status) {

{ on a new line

Comment 9

[(no longer active)]

Attachment: Part 2: Add a test case to ensure that http-on-modify-request will be dispatched by requests blocked by tracking protection

Comment 10

[(no longer active)]

Filed follow-up bug 1485427 for the assertions you suggested.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48213c5c25e9
Part 1: Run http-on-modify-request observers when tracking protection cancels an HTTP channel; r=mayhemer

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 9003214 [details] [diff] [review]
Part 2: Add a test case to ensure that http-on-modify-request will be dispatched by requests blocked by tracking protection

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

::: toolkit/components/antitracking/test/browser/browser_onModifyRequestNotificationForTrackingResources.js
@@ +1,2 @@
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +

few bullet points to list what the test does would be useful.

@@ +23,5 @@
> +  await SpecialPowers.pushPrefEnv({"set": [
> +    ["browser.contentblocking.enabled", true],
> +    ["privacy.trackingprotection.enabled", true],
> +    ["privacy.trackingprotection.pbmode.enabled", false],
> +    ["privacy.trackingprotection.annotate_channels", false],

why false?

@@ +33,5 @@
> +  info("Creating a new tab");
> +  let tab = BrowserTestUtils.addTab(gBrowser, TEST_EMBEDDER_PAGE);
> +  gBrowser.selectedTab = tab;
> +
> +  let promise = onModifyRequest();

what line actually starts the document load?  just to make sure we can't race (that the onmodify hook is made at the right place)

Comment 13

[(no longer active)]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> @@ +23,5 @@
> > +  await SpecialPowers.pushPrefEnv({"set": [
> > +    ["browser.contentblocking.enabled", true],
> > +    ["privacy.trackingprotection.enabled", true],
> > +    ["privacy.trackingprotection.pbmode.enabled", false],
> > +    ["privacy.trackingprotection.annotate_channels", false],
> 
> why false?

This test doesn't rely in any way on tracking annotations, so I might as well make it explicit here.

(Note that in mochitests, tracking.example.com is blocked by tracking *protection* and tracking.example.org is annotated by tracking annotations, and this test is using the former domain, so there is no doubts here!)

> @@ +33,5 @@
> > +  info("Creating a new tab");
> > +  let tab = BrowserTestUtils.addTab(gBrowser, TEST_EMBEDDER_PAGE);
> > +  gBrowser.selectedTab = tab;
> > +
> > +  let promise = onModifyRequest();
> 
> what line actually starts the document load?  just to make sure we can't
> race (that the onmodify hook is made at the right place)

Ah right.  It is the addTab() call.  We should mint this promise before that, I think.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf7cca192e7
Part 2: Add a test case to ensure that http-on-modify-request will be dispatched by requests blocked by tracking protection; r=mayhemer

Comment 15

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 4bf7cca192e7 (Bug 1485182) for ES linting failure on a CLOSED TREE

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4bf7cca192e7657ed0bdaa598b2a9ddb30ac5472&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=8c6a05c61d397cc70412052f36670b2c452c1ec1&selectedJob=195360416
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=195360416&repo=mozilla-inbound&lineNumber=260

[task 2018-08-22T19:36:03.158Z] 
[task 2018-08-22T19:36:03.158Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-08-22T19:42:03.547Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/antitracking/test/browser/browser_onModifyRequestNotificationForTrackingResources.js:14:5 | addObserver's third parameter can be omitted when it's false. (mozilla/no-useless-parameters)
[taskcluster 2018-08-22 19:42:04.143Z] === Task Finished ===
[taskcluster 2018-08-22 19:42:04.143Z] Unsuccessful task run with exit code: 1 completed in 623.983 seconds

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f5f240834b0
Backed out changeset 4bf7cca192e7 for ES linting failure on a CLOSED TREE

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40ee8078a913
Part 2: Add a test case to ensure that http-on-modify-request will be dispatched by requests blocked by tracking protection; r=mayhemer

Comment 18

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/48213c5c25e9

Comment 19

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/40ee8078a913

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to :Ehsan Akhgari from comment #13)
> This test doesn't rely in any way on tracking annotations, so I might as
> well make it explicit here.
> 
> (Note that in mochitests, tracking.example.com is blocked by tracking
> *protection* and tracking.example.org is annotated by tracking annotations,
> and this test is using the former domain, so there is no doubts here!)

Really?  I though that annotation is based on the same information we use for blocking.  I.e. - TP=off -> channels are only annotated, TP=on -> channels that would be annotated are blocked.  Or not?  That would be bad, actually.

Comment 21

[(no longer active)]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> (In reply to :Ehsan Akhgari from comment #13)
> > This test doesn't rely in any way on tracking annotations, so I might as
> > well make it explicit here.
> > 
> > (Note that in mochitests, tracking.example.com is blocked by tracking
> > *protection* and tracking.example.org is annotated by tracking annotations,
> > and this test is using the former domain, so there is no doubts here!)
> 
> Really?  I though that annotation is based on the same information we use
> for blocking.

That is still the case.

>  I.e. - TP=off -> channels are only annotated, TP=on ->
> channels that would be annotated are blocked.  Or not?  That would be bad,
> actually.

Your understanding is correct.  But until 3 weeks ago, inside Gecko we couldn't differentiate whether the classifier is working on something due to the annotations being active or due to TP being active.  Bug 1461515 split things up so now they are distinguishable (for example, NS_ERROR_TRACKING_URI is now only used for TP, and we have NS_ERROR_TRACKING_ANNOTATION_URI too.)

Comment 22

[Ciprian Georgiu, Desktop QA]

(In reply to :Ehsan Akhgari from comment #0)
...
> Add-ons like ad-blockers use these observers in the form of the webRequest
> API to intercept and block HTTP requests, so as a result of this, uBlock
> Origin on https://www.kino.de/ for example only finds 11 elements with TP
> turned on.  After fixing this bug, it finds 25 elements.

Hi Ehsan, I was trying to reproduce this issue on an affected Nightly build, but I cannot seem to figure it out where I can see those "11 elements" mentioned above on uBlock Origin, while loading the https://www.kino.de/ with TP turned on.

These elements are supposed to be displayed in the add-on interface, or somewhere else, could you please let me know?

Comment 23

[(no longer active)]

The number of blocked elements appears in the tooltip for the uBlock Origin icon in the toolbar after a page is finished loading (e.g., "uBlock Origin (11)".

Note that the number "11" or "25" here changes depending on what blockable resources the site serves at any given time.  Right now for example after I tested it I got "25".

In general I don't believe this bug requires manual verification since it has an automated test.  Manual verification wouldn't help discover something that our tests wouldn't...

Comment 24

[Ciprian Georgiu, Desktop QA]

Okay. Thank you for the explanations, Ehsan. In this case I'll remove the qe+ flag.