Closed Bug 1511477 Opened 5 years ago Closed 5 years ago

GeckoView API no longer reports all blocked trackers

Categories

(GeckoView :: General, defect, P1)

Unspecified
Android
defect

Tracking

(geckoview64 wontfix, firefox64 wontfix, firefox65 fixed, firefox66 fixed)

RESOLVED FIXED
mozilla66
Tracking Status
geckoview64 --- wontfix
firefox64 --- wontfix
firefox65 --- fixed
firefox66 --- fixed

People

(Reporter: j, Assigned: esawin)

References

Details

(Whiteboard: [geckoview:fenix:p1])

Attachments

(3 files, 1 obsolete file)

Attached file trackers.txt
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Steps to reproduce:

Visit huffingtonpost.com


Actual results:

only 18 trackers blocked


Expected results:

WV reported 40+ trackers blocked. Something closer to that number
Jeff says that Focus+WebView bundles its own blocklist based on Disconnect. Gecko[View] uses a subset of Disconnect's blocklist (with site-breaking blocks removed), so Gecko will have a shorter blocklist. However, I would not expect a 18/40 tracker difference!
OS: Unspecified → Android
Priority: -- → P1
Summary: WV/GV tracking protection discrepancies → WV/GV tracking protection discrepancies: WV block 40+ trackers but GV only 18
Assignee: nobody → esawin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
There was a regression on the reporting of blocked trackers in bug 1508944. We're only reporting a small subset of the trackers that we block because of [1].

Ehsan, is this expected behaviour and do you see a way to maintain consistent tracker reporting for GeckoView (onTrackerBlocked) without perf regressions?

[1] https://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindowOuter.cpp#5090
Blocks: 1508944
Flags: needinfo?(ehsan)
Sorry, I think you may need to give me a bit more information to work with here.  I'm not familiar in any way with what GeckoView does.

But to your main question, bug 1508944 should have been a non-functional change for any non-buggy listener of onSecurityChange, since the only thing that it changed was that it made us stop reporting the same notifications over and over again for the same document.  IOW if we previously notified onSecurityChange with aState set to value foo 5 times, now we only report it one time.  A non-buggy consumer should be able to properly handle the first notification and not need the next 4 so if something has broken, in theory it should be a bug in the onSecurityChange callback...

But that being said this code has had a number of bugs in it that we've been ironing out gradually so I'm not ruling out the possibility of there being more bugs, but I need more information to be able to tell what's going wrong and why.  Things that would help are for example, which onSecurityChange listener is the interesting consumer here, what exactly does it expect to see and whether there are any assumptions in it around seeing the same state reported multiple times.

Thanks!
Flags: needinfo?(ehsan) → needinfo?(esawin)
We're listening for onSecurityChange in GeckoViewTrackingProtection.jsm [1] to track and report each blocked tracker through our GeckoView API (GeckoSession.TrackingProtectionDelegate.onTrackerBlocked [2]).
For that, we're retrieving the blocked URI and matched blocklist from the channel in the callback.

[1] https://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/GeckoViewTrackingProtection.jsm#23
[2] https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java#3740
Flags: needinfo?(esawin) → needinfo?(ehsan)
The TP GeckoView API was introduced in bug 1423229.
Eugen says we are blocking but not getting the right events we need to be able to report all of them.
64=wontfix because we don't need to uplift this fix to GV 64 relbranch.
The patch above would fix the situation for GeckoView by dispatching onSecurityChange when triggered by blocked trackers.
I anticipate that this would still mean a Talos perf regression.

Alternatively, we could add a dedicated notification for content blocking instead to avoid spamming all onSecurityChange listeners that are not interested in blocked tracker data.
OK, now we're getting closer to the problem, I think.  But still something is missing from my understanding, so I need a bit more help to find out how to fix this bug correctly (your current patch isn't the correct fix yet!).

Based on reading your onSecurityChange function, I think what's going wrong is that you're not getting the matchedList you expect, is that correct?  In other words, |matchedList| here <https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/mobile/android/modules/geckoview/GeckoViewTrackingProtection.jsm#35> is null?

But I'm thinking, why would that be the case?  That member should be set from nsChannelClassifier::SetBlockedContent().  That function is called here: <https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/netwerk/base/nsChannelClassifier.cpp#1209> where mList is passed as the thing that ends up being matchedList.

But mList is only being set under this condition here:

https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/netwerk/base/nsChannelClassifier.cpp#963,966

Is the problem that on line 1209, for the same document sometimes SetBlockedContent() is called with one value of mList and sometimes with another value?  In other words, if you added a printf() in nsGlobalWindowOuter::NotifyContentBlockingState() to print the value of matchedList that you read from nsIClassifiedChannel in your patch when the STATE_BLOCKED_TRACKING_CONTENT notifications are dispatched, do you see the values change over time?

If the values change over time, that is a bug which is the source of the underlying issue...
Flags: needinfo?(ehsan) → needinfo?(esawin)
(In reply to :Ehsan Akhgari from comment #10)
> Based on reading your onSecurityChange function, I think what's going wrong
> is that you're not getting the matchedList you expect, is that correct?  In
> other words, |matchedList| here
> <https://searchfox.org/mozilla-central/rev/
> fd32b3a6fa3eff1468311f6fcf32b45c117136df/mobile/android/modules/geckoview/
> GeckoViewTrackingProtection.jsm#35> is null?

No, matchedList is set correctly, that's not the issue.
We're simply not getting the onSecurityChange callback at all because we return early in nsGlobalWindowOuter::NotifyContentBlockingState [1] before calling eventSink->OnSecurityChange because of bug 1508944.

My patch (or just removing the early return) would fix this regression for us, but it will most likely mean a Talos regression, which is why I've been wondering if we want to decouple tracker reporting from onSecurityChange.

[1] https://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindowOuter.cpp#5086
Flags: needinfo?(esawin) → needinfo?(ehsan)
(In reply to Eugen Sawin [:esawin] from comment #11)
> (In reply to :Ehsan Akhgari from comment #10)o
> > Based on reading your onSecurityChange function, I think what's going wrong
> > is that you're not getting the matchedList you expect, is that correct?  In
> > other words, |matchedList| here
> > <https://searchfox.org/mozilla-central/rev/
> > fd32b3a6fa3eff1468311f6fcf32b45c117136df/mobile/android/modules/geckoview/
> > GeckoViewTrackingProtection.jsm#35> is null?
> 
> No, matchedList is set correctly, that's not the issue.

Ah, OK.  :-/

> We're simply not getting the onSecurityChange callback at all because we
> return early in nsGlobalWindowOuter::NotifyContentBlockingState [1] before
> calling eventSink->OnSecurityChange because of bug 1508944.

But that is *not* the source of the bug.  Let me try to explain how this is expected to work, if everything else fits together (before we get to that early return.)

  * A top-level document starts its life.  There are no trackers loaded in it by definition.
  * The first tracker is loaded in it, and it gets blocked.  nsChannelClassifier::SetBlockedContent() gets called.
  * We call NotifyChannelBlocked() with STATE_BLOCKED_TRACKING_CONTENT here <https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/netwerk/base/nsChannelClassifier.cpp#856> for the first time in that top-level document.
  * That brings us here: <https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/netwerk/base/nsChannelClassifier.cpp#899> where we call nsGlobalWindowOuter::NotifyContentBlockingState() for the first time on this top-level document.
  * Now, firstly we check to ensure we're on the correct document <https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/dom/base/nsGlobalWindowOuter.cpp#5010>.  (This check is historical and I am not sure why it exists, it's been there since before I've looked at this code!!)
  * Then we read the current security state from the docshell: <https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/dom/base/nsGlobalWindowOuter.cpp#5026>.  This gives us the current security state flags from the top-level document.  If this is the first time we're dispatching STATE_BLOCKED_TRACKING_CONTENT, the STATE_BLOCKED_TRACKING_CONTENT bit must be cleared in the state flag.
  * Then we enter this block: <https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/dom/base/nsGlobalWindowOuter.cpp#5033-5036>.  There we first remember that origin foo was blocked because it was a tracker, and we set unblocked to true if some other tracker was previously blocked and vice versa (when we're removing a blocked code, not relevant here since we're adding a blocked code, IOW aBlocked is true).  The first time this notification is being dispatched, unblocked will be false.
  * Then we enter this block: <https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/dom/base/nsGlobalWindowOuter.cpp#5077-5082>.  The first time we call this, state will be oldState plus the STATE_BLOCKED_TRACKING_CONTENT.  Therefore, it is guaranteed that state != oldState, so the first time the early return will not be taken and we will dispatch the onSecurityChange notification.

One thing to note is that nsSecureBrowserUIImpl::GetState() <https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/security/manager/ssl/nsSecureBrowserUIImpl.cpp#61> actually queries the docshell (which in turn queries its document) to populate the state bitfield it returns.  Therefore:

  * If you follow the above steps again, you'll see that because of the fact that the security state read from the docshell the second time will include the STATE_BLOCKED_TRACKING_CONTENT bit this time, in the second and the following executions for a top-level document that has already had its trackers blocked, we end up taking that early return.

So basically that early return was only added to avoid dispatching the *same* notification with the same bitfields on the same top-level document multiple times.

Now, I'm trying to understand why this doesn't work well for GeckoView.  Looking at the onSecurityChange notification listener you linked to in comment 4, that listener isn't stateful so it shouldn't care whether it's called once or multiple times per top-level document.  But following the event dispatched from there I found this callback <https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java#878> which seems to be trying to count how many trackers are found on the top-level document.  Now, if I've followed the sequence of execution correctly here, *that* would obviously break with bug 1508944...  Is that the problem you're trying to solve?

> My patch (or just removing the early return) would fix this regression for
> us, but it will most likely mean a Talos regression, which is why I've been
> wondering if we want to decouple tracker reporting from onSecurityChange.

Yes, we definitely want to decouple the two, but as far as I understand you'd like a fix that is backportable to 65, correct?  If yes, then decoupling the two should be done separately from fixing this particular bug.
Flags: needinfo?(ehsan) → needinfo?(esawin)
(In reply to :Ehsan Akhgari from comment #12)
> Now, I'm trying to understand why this doesn't work well for GeckoView. 
> Looking at the onSecurityChange notification listener you linked to in
> comment 4, that listener isn't stateful so it shouldn't care whether it's
> called once or multiple times per top-level document.  But following the
> event dispatched from there I found this callback
> <https://searchfox.org/mozilla-central/rev/
> fd32b3a6fa3eff1468311f6fcf32b45c117136df/mobile/android/geckoview_example/
> src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java#878>
> which seems to be trying to count how many trackers are found on the
> top-level document.  Now, if I've followed the sequence of execution
> correctly here, *that* would obviously break with bug 1508944...  Is that
> the problem you're trying to solve?

That's correct, the GeckoView API reports every blocked tracker (URI and type), which is why having a single onSecurityChange callback per document means a regression for us.
 
> Yes, we definitely want to decouple the two, but as far as I understand
> you'd like a fix that is backportable to 65, correct?  If yes, then
> decoupling the two should be done separately from fixing this particular bug.

That's right, do you have a better idea on how to handle that without regressing Talos for desktop or would that be acceptable if only limited to 65?

I'll open a new bug to discuss the decoupling of the tracker reporting, unless you want to drive that.
Flags: needinfo?(esawin) → needinfo?(ehsan)
Summary: WV/GV tracking protection discrepancies: WV block 40+ trackers but GV only 18 → GeckoView API no longer reports all blocked trackers
(In reply to Eugen Sawin [:esawin] from comment #13)
> (In reply to :Ehsan Akhgari from comment #12)
> > Now, I'm trying to understand why this doesn't work well for GeckoView. 
> > Looking at the onSecurityChange notification listener you linked to in
> > comment 4, that listener isn't stateful so it shouldn't care whether it's
> > called once or multiple times per top-level document.  But following the
> > event dispatched from there I found this callback
> > <https://searchfox.org/mozilla-central/rev/
> > fd32b3a6fa3eff1468311f6fcf32b45c117136df/mobile/android/geckoview_example/
> > src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java#878>
> > which seems to be trying to count how many trackers are found on the
> > top-level document.  Now, if I've followed the sequence of execution
> > correctly here, *that* would obviously break with bug 1508944...  Is that
> > the problem you're trying to solve?
> 
> That's correct, the GeckoView API reports every blocked tracker (URI and
> type), which is why having a single onSecurityChange callback per document
> means a regression for us.
>  
> > Yes, we definitely want to decouple the two, but as far as I understand
> > you'd like a fix that is backportable to 65, correct?  If yes, then
> > decoupling the two should be done separately from fixing this particular bug.
> 
> That's right, do you have a better idea on how to handle that without
> regressing Talos for desktop or would that be acceptable if only limited to
> 65?

OK, thanks.  Yes, I think I can write a better fix now, equipped with this information.  New patch coming up soon.

> I'll open a new bug to discuss the decoupling of the tracker reporting,
> unless you want to drive that.

We've discussed this before in the anti-tracking project too but we've not filed a bug for it yet, so I went ahead and did that for now (bug 1514340).  It's unclear when this will happen but yes I plan to get to it someday soon hopefully.
Flags: needinfo?(ehsan)
Comment on attachment 9031556 [details]
Bug 1511477 - Ensure that the GeckoView API always reports the URI and type of all blocked trackers

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1508944

User impact if declined: See comment 0

Is this code covered by automated tests?: Unknown

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: Eugen, I'd appreciate if you can help with providing steps for QE to verify how this should be tested.  I'm not sure whether there are automated tests for this...

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Pretty low risk change, reverts us back to the behaviour before bug 1508944.

String changes made/needed: None
Attachment #9031556 - Flags: approval-mozilla-beta?
Flags: needinfo?(esawin)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b643a8a7b328
Ensure that the GeckoView API always reports the URI and type of all blocked trackers r=esawin
The only way to reproduce this without building Focus is by monitoring logcat using the GeckoView example app (GVE).

STR
0. Attach to device/emulator logcat and filter by "Trackers blocked".
1. Launch GVE app.
2. Enable tracking protection in the options menu.
3. Open a major news site, e.g., cnn.com, and wait for the load to finish (may take a while).

Expected result: 10-40 trackers blocked in total.
Actual result: 1 tracker blocked in total.
Flags: needinfo?(esawin)
Is it possible to write an automated test for this?
Flags: in-testsuite+
Comment on attachment 9031556 [details]
Bug 1511477 - Ensure that the GeckoView API always reports the URI and type of all blocked trackers

[Triage Comment]
Improves tracker blocking on Android, approved for 65.0b5. If possible, please post a link to the GVE app to make life easier for QA when trying to verify this bug.
Flags: needinfo?(esawin)
Attachment #9031556 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
64=wontfix because we don't need to uplift this tracker counter fix to the GV64 relbranch.
https://hg.mozilla.org/mozilla-central/rev/b643a8a7b328
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Tested on Firefox Focus 8.0.4 following the steps from comment #18 and the issue is still reproducible but seems that the fix is not landed yet. We saw the same issue in GitHub and is not fixed. We'll keep an eye and check again when the Focus is with the patch.
Attachment #9031262 - Attachment is obsolete: true
(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> Comment on attachment 9031556 [details]
> Bug 1511477 - Ensure that the GeckoView API always reports the URI and type
> of all blocked trackers
> 
> [Triage Comment]
> Improves tracker blocking on Android, approved for 65.0b5. If possible,
> please post a link to the GVE app to make life easier for QA when trying to
> verify this bug.

This is the debug GVE build for the m-c commit: https://queue.taskcluster.net/v1/task/KMOGrwHBRbSXvaNt3sBGzA/runs/0/artifacts/public/build/geckoview_example.apk

I'm also going to extend our JUnit test to cover this case in this bug (doesn't need uplifting).
Flags: needinfo?(esawin)
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52ee186f9550
[2.0] Extend tracking protection test. r=snorp
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 66 → mozilla66
Whiteboard: [geckoview:fenix:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: