Move content blocking notifications off on onSecurityChange

RESOLVED FIXED in Firefox 66

Status

()

defect
P1
normal
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
Firefox 66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

5 months ago
They've caused a number of problems already, such as:

  * Bug 1511477
  * Bug 1510275
  * Bug 1508944

Maybe it's time to have a new progress listener notification: onContentBlocked or something like that.
Assignee

Updated

5 months ago
Blocks: 1516540
Assignee

Comment 1

5 months ago
Let's call the new notification onContentBlockingEvent().
Priority: -- → P2
Assignee

Comment 2

5 months ago

Eugen, do you have some cycles to help me on this bug please? I need some help to rework the mobile code based on the changes I am planning to make here and I would really appreciate your help. This will help make the issue we saw in bug 1511477 more efficient.

You can see my current changes in this try push that I just pushed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de8c884089fda52cf560971f33c80d7651ed42f0

Here is part 1:
https://hg.mozilla.org/try/rev/fbf2586e97fd

Here is part 2:
https://hg.mozilla.org/try/rev/362c57087dfa

The gist is that I am adding a new nsIWebProgressListener notification called onContentBlockingEvent, with a corresponding notification code NOTIFY_CONTENT_BLOCKING (see part 2). This new notification only fires when there is a content blocking event (for example, STATE_BLOCKED_TRACKING_CONTENT). Previously, onSecurityChanged used to be dispatched for these. After these changes, onSecurityChanged will never include these notification codes.

These are all of the event codes that onContentBlockingEvent will now be dispatched for: https://hg.mozilla.org/try/rev/362c57087dfab8e0a6753a1bccb748e8e823c7b4#l73.81

As you can see on the try push, there are a few mobile test failures (I think everything else should be green on this push now!). I think these are caused by the code in the first two results of this query (I have already fixed the third result in my part 2 patch): https://searchfox.org/mozilla-central/search?q=STATE_BLOCKED_TR&case=false&regexp=false&path=mobile%2F

These are a bit difficult for me to fix, because for example there is an onSecurityChange handler in browser.js https://searchfox.org/mozilla-central/rev/f8de61826903996f6bdf41b11a2844dd59ac144f/mobile/android/chrome/content/browser.js#4745 which expects to be able to see both state codes related to content blocking as well as security https://searchfox.org/mozilla-central/rev/f8de61826903996f6bdf41b11a2844dd59ac144f/mobile/android/chrome/content/browser.js#5812 and sends this information in a single message to be consumed at the Java side https://searchfox.org/mozilla-central/rev/76fe4bb385348d3f45bbebcf69ba8c7283dfcec7/mobile/android/base/java/org/mozilla/gecko/toolbar/SecurityModeUtil.java#101, assuming I understand the setup correctly. There seems to be a GeckoView copy of the JS side of things also, which I don't quite understand...

I thought I would ask for your help instead of stumbling around trying to figure out how to fix this code myself. Would you be able to help me here please? Thanks a lot!

Flags: needinfo?(esawin)

Thanks for the details, Ehsan!

We can remove all trackingMode related code from GeckoView since it doesn't add any information on top of the TrackingProtection API (or the updated ContentBlocking API from bug 1499214).
I can write the patch to do that, if you prefer.

On Fennec, the content blocking state code is used to determine the shield icon in the UI. In order to make that work with the new onContentBlockingEvent, it needs to cache that information for the tab.

@Susheel, do you have someone who can do the rewiring in the Fennec browser.js?

Flags: needinfo?(esawin) → needinfo?(sdaswani)

Comment 4

4 months ago

I'm putting this into our P1 list.

Flags: needinfo?(sdaswani)
Priority: P2 → P1

Comment 5

4 months ago

Hmm, Liz, how does this get into our P1 list https://mzl.la/2QrIPGt ?

Flags: needinfo?(lhenry)

It wouldn't show up there since that's based on a search by particular components, so I'll just add it to the etherpad by hand if you like.

Flags: needinfo?(lhenry)

Or, Susheel, if you could file a new bug in a relevant component that might be best.

Flags: needinfo?(sdaswani)

Comment 8

4 months ago

Moving back down to P2. Will wait to hear on Product on whether we should prioritize this fix.

Flags: needinfo?(sdaswani)
Priority: P1 → P2
Assignee

Comment 9

4 months ago

(In reply to Eugen Sawin [:esawin] from comment #3)

Thanks for the details, Ehsan!

We can remove all trackingMode related code from GeckoView since it doesn't add any information on top of the TrackingProtection API (or the updated ContentBlocking API from bug 1499214).
I can write the patch to do that, if you prefer.

Great, the best code is code that can be deleted. :-)

I would very much appreciate your help if you can spare some cycles on this. Thanks very much!

(In reply to :sdaswani only needinfo from comment #8)

Moving back down to P2. Will wait to hear on Product on whether we should prioritize this fix.

Susheel, thanks for your help on this.

FWIW this work blocks our anti-tracking project for Firefox 66, and it needs to land much earlier than the end of cycle for 66 which is in about two weeks from now. The reason is that over in bug 1516540 I'm working on a series of fixes that all need to land so that we fix the really large performance regression that we have on our plate and this large refactoring project is a big part of it.

I'd appreciate if you can take that into consideration and please let me know when/if I can expect some help on the mobile side. If the answer turns out to be no (or too late for my needs) I would need to figure out a backup plan so I'd appreciate to know that as early as possible. Thanks in advance!

Assignee

Comment 12

4 months ago

Eugen, the patches that I just attached are rebased on top of the latest trunk, the ones that I had pushed to try no longer apply cleanly...

Petru, is this something your team can help with?

Priority: P2 → P1
Flags: needinfo?(petru.lingurar)

Comment 14

4 months ago

Sure, we will get this fixed ASAP.

Petru, this needs to land ASAP, so all three of you can work on this tomorrow if needs be (if pipelining or pairing helps).

Assignee: nobody → petru.lingurar
Depends on: 1518951
Assignee

Comment 15

4 months ago

Thank you!

Hi,
We've tried to investigate this and first understand the business logic behind the intended feature but we couldn't apply the patches from comment #10 and comment #11 to move forward.

Also, as we understand from comment #2 and comment #3 the issue here is that some test are failing [1] after the refactorization and the needed fix is to make sure that shield icon appears correctly depending on onContentBlockingEvent

:Ehsan could you rebase again against m-c?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=de8c884089fda52cf560971f33c80d7651ed42f0&selectedJob=220464119

Flags: needinfo?(petru.lingurar)

To continue on my previous message, because we are currently stuck and :esawin did some progress on geckoview side, can you help us with the modifications in browser.js and then we can pick up and do any other needed changes on Java side?
Also, our js expertise is limited so we might not be able to get this done in time.

Flags: needinfo?(esawin)
Assignee

Comment 18

4 months ago

(In reply to Petru-Mugurel Lingurar[:petru] from comment #16)

Hi,
We've tried to investigate this and first understand the business logic behind the intended feature but we couldn't apply the patches from comment #10 and comment #11 to move forward.

I rebased the patches on the latest mozilla-central.

Also, as we understand from comment #2 and comment #3 the issue here is that some test are failing [1] after the refactorization and the needed fix is to make sure that shield icon appears correctly depending on onContentBlockingEvent

Yes, the mochitest failure in particular, see this more recent try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9242c500e3004b723feb98d9c8e0997f58a017b

The failure log: https://queue.taskcluster.net/v1/task/GaI9fEOcQyq3RysARYy6xQ/runs/0/artifacts/public/logs/live_backing.log

This is rc3 on "Android 4.3 API16+ opt".

Please note that besides ensuring that the mochitest passes successfully, I would also like to ensure that the Site Identity UI which appears in Fennec after you click on the icon to the left of the URL inside the URL bar works correctly. I'm not sure if that component has good test coverage or not, so there may be problems caused by the refactoring that we don't have automated tests for.

To further clarify the needed changes for Fennec based on comment 2, right now the Java layer is notified with the SiteIdentity data structure which includes data about the security UI as well as the tracking protection state, which is used to update the Site Identity UI: https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/toolbar/SecurityModeUtil.java#98. This information is generated from the onSecurityChange notifications that my patch is refactoring into two notifications. When Gecko starts to send the tracking protection state in a separate notification (onContentBlockingEvent), the Java layer needs to change somehow in order to listen to two separate notifications coming from Gecko and receive the security UI data separately from the tracking protection state and later on combine them together to display them in the Site Identity UI. I wasn't sure how to do this part...

Thanks Ehsan, indeed the patches now apply cleanly.
Continued to investigate the business logic and how to adapt your patches but I could not do a full build on my machine because of

.mozbuild/android-ndk-r15c/toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/lib/gcc/arm-linux-androideabi/4.9.x/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol 'arc4random_buf' is not defined locally

I think totally unrelated to your patches but currently we are stuck.
We'll pick up on Monday but maybe Eugen can help us move this faster.

Comment 20

4 months ago

(In reply to Petru-Mugurel Lingurar[:petru] from comment #19)

Thanks Ehsan, indeed the patches now apply cleanly.
Continued to investigate the business logic and how to adapt your patches but I could not do a full build on my machine because of

.mozbuild/android-ndk-r15c/toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/lib/gcc/arm-linux-androideabi/4.9.x/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol 'arc4random_buf' is not defined locally

I think totally unrelated to your patches but currently we are stuck.
We'll pick up on Monday but maybe Eugen can help us move this faster.

Eugen any advice here?

There seems to be an issue building on Mac, maybe this bug has a workaround, otherwise building on Linux would be the alternative.

Flags: needinfo?(esawin)

Please guinea pig a thing for me: Apply the stack at https://bugzilla.mozilla.org/show_bug.cgi?id=1477487, follow the instructions in https://bugzilla.mozilla.org/show_bug.cgi?id=1477487#c14, and see if a “Mozilla clang” + “r17b” + x86_64 on macOS build succeeds?

(In reply to Nick Alexander :nalexander [he/him] from comment #22)

Please guinea pig a thing for me: Apply the stack at https://bugzilla.mozilla.org/show_bug.cgi?id=1477487, follow the instructions in https://bugzilla.mozilla.org/show_bug.cgi?id=1477487#c14, and see if a “Mozilla clang” + “r17b” + x86_64 on macOS build succeeds?

59:07.70 We know it took a while, but your build finally finished successfully!

Followed your instructions, just built for arm-linux-androideabi, all went great. Thanks!!
All went great until actually running the robocop test where I got an ADBTimeoutError. Will try on Linux.

// All 3 of us tried this today.
On both Mac and Linux for getting a full build we needed to apply the patch from bug 1477487 and target arm-linux-androideabi
On both Mac and Linux we got ADBTimeoutError while waiting for the test to start or after ./mach robocop --serve when trying to run on a real device.

We'll try again to get a build for the emulator and fix there.

Depends on: 1520520
Assignee

Updated

4 months ago
Blocks: 1520879

Comment 24

4 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1381f65203
Part 1: Add the nsISecurityEventSink::OnContentBlockingEvent() helper method; r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b15591bb21
Part 2: Break out the content blocking related notifications into nsIWebProgressListener.onContentBlockingEvent(); r=baku,johannh
Assignee

Updated

4 months ago
Assignee: petru.lingurar → ehsan
Assignee

Updated

4 months ago
Depends on: 1521582

Comment 25

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Assignee

Updated

3 months ago
Depends on: 1527151
Assignee

Updated

3 months ago
No longer depends on: 1527151
You need to log in before you can comment on or make changes to this bug.