1514340 - Move content blocking notifications off on onSecurityChange

Status: RESOLVED FIXED

(Firefox :: Protections UI, defect, P1)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

Let's call the new notification onContentBlockingEvent().

Comment 2

[(no longer active)]

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!

Comment 3

[Eugen Sawin [: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?

Comment 4

[:sdaswani]

I'm putting this into our P1 list.

Comment 5

[:sdaswani]

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

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 8

[:sdaswani]

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

Comment 9

[(no longer active)]

(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!

Comment 10

[(no longer active)]

Attachment: Bug 1514340 - Part 1: Add the nsISecurityEventSink::OnContentBlockingEvent() helper method;

Comment 11

[(no longer active)]

Attachment: Bug 1514340 - Part 2: Break out the content blocking related notifications into nsIWebProgressListener.onContentBlockingEvent();

Depends on D16051

Comment 12

[(no longer active)]

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...

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Petru, is this something your team can help with?

Comment 14

[:sdaswani]

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).

Comment 15

[(no longer active)]

Thank you!

Comment 16

[Petru-Mugurel Lingurar [:petru]]

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

Comment 17

[Petru-Mugurel Lingurar [:petru]]

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.

Comment 18

[(no longer active)]

(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...

Comment 19

[Petru-Mugurel Lingurar [:petru]]

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

[:sdaswani]

(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?

Comment 21

[Eugen Sawin [:esawin]]

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

Comment 22

[Nick Alexander :nalexander [he/him]]

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?

Comment 23

[Petru-Mugurel Lingurar [:petru]]

(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.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3b1381f65203
https://hg.mozilla.org/mozilla-central/rev/f2b15591bb21