Add FXIOS-10667 Content blocklist support via Remote Settings #23342

mattreaganmozilla · 2024-11-23T00:05:40Z

📜 Tickets

💡 Description

Initial updates to support Remote Settings for content blocking lists in iOS client. Changes:

Removes our previous Shavar shell script and related code in bootstrap.sh
Fixes old Xcode project references to disconnect-* files
Updates older code that was loading the JSON data directly from bundle to instead route that through the Remote Settings APIs so we have more control over how that data is vended

This is still a work-in-progress and there are a few areas that will need to be updated further (I have not added or updated tests etc.).

Result

2024-11-22 15:52:23.738 💙 INFO [adblock] ContentBlocker - Compiling any lists not already in rule store...
2024-11-22 15:52:23.738 💙 INFO [adblock] ContentBlocker - Will compile list: disconnect-block-advertising
2024-11-22 15:52:23.745 💙 INFO [adblock] ContentBlocker - Will compile list: disconnect-block-analytics
2024-11-22 15:52:23.745 💙 INFO [adblock] ContentBlocker - Will compile list: disconnect-block-social
2024-11-22 15:52:23.745 💙 INFO [adblock] ContentBlocker - Will compile list: disconnect-block-cryptomining
2024-11-22 15:52:23.746 💙 INFO [adblock] ContentBlocker - Will compile list: disconnect-block-fingerprinting
2024-11-22 15:52:23.746 💙 INFO [adblock] ContentBlocker - Will compile list: disconnect-block-cookies-advertising
2024-11-22 15:52:23.746 💙 INFO [adblock] ContentBlocker - Will compile list: disconnect-block-cookies-analytics
2024-11-22 15:52:23.746 💙 INFO [adblock] ContentBlocker - Will compile list: disconnect-block-cookies-social
2024-11-22 15:52:24.147 💙 INFO [adblock] ContentBlocker - Compiled 8 of 8 lists checked. 0 errors.

📝 Checklist

You have to check all boxes before merging

Filled in the above information (tickets numbers and description of your work)
Updated the PR name to follow our PR naming guidelines
Wrote unit tests and/or ensured the tests suite is passing
When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
If needed, I updated documentation / comments for complex code and public methods
If needed, added a backport comment (example @Mergifyio backport release/v120)

…s well as old content blocker script, and update the bootstrap to no longer call it

…ault disconnect (blocklist) files

…ings APIs rather than directly as bundle resource

… new JSON utility func.

mattreaganmozilla · 2024-11-25T21:51:07Z

bootstrap.sh

@@ -46,6 +46,3 @@ cp -r .githooks/* .git/hooks/

 # Make the hooks are executable
 chmod +x .git/hooks/*
-
-# Run and update content blocker
-./content_blocker_update.sh


cc @clarmso Just a quick heads up that we are removing this content blocker update script from the bootstrap.sh.

I'm not aware of any issues this should cause on Bitrise but wanted to make sure you were looped in. Thanks

I just noticed that this script is also responsible for running the code that generates our MainFrameAtDocument*.js files. So I'm fixing this right now, should have an update pushed soon.

(Update: the above issue should be fixed now.)

mobiletest-ci-bot · 2024-11-25T22:05:28Z

	Messages
📖	Project coverage: 32.69%
📖	Edited 6 files
📖	Created 1 files

Client.app: Coverage: 31.58

File	Coverage
ContentBlocker.swift	2.68%	⚠️
TrackingProtectionPageStats.swift	53.19%	✅
RemoteDataType.swift	49.28%	⚠️

Generated by 🚫 Danger Swift against 6ccd1bb

mattreaganmozilla · 2024-11-25T22:22:41Z

I believe the way the script is removed currently will break the generation of our MainFrameAtDocument*.js files, adding Do Not Merge until a fix is pushed up for that. Taking a closer look.

issammani

This looks @mattreaganmozilla. Thanks. I see some stuff we can improve in the future, but this a really good place to start.

issammani · 2024-11-25T22:29:35Z

firefox-ios/Client/ContentBlocker/ContentBlocker.swift

+            guard let data = try? lists.loadLocalSettingsFileAsJSON(fileName: list) else { continue }
+            guard let newHash = calculateHash(for: data) else { continue }


Not really something we can support right now, but once we figure out caching we can also get the file digest from RS for attachments.

Yes and I believe there is some overlap between the current caching and RS so we may be able to clean up and/or remove some of this later down the road.

issammani · 2024-11-25T22:29:40Z

firefox-ios/Client/Application/RemoteSettings/RemoteDataType.swift

        }
    }

+    /// Loads the local settings for the given data type record, returning the
+    /// decoded objects.
+    /// - Returns: settings decoded to their RemoteDataTypeRecord.


Eventually I think we would want to converge to the desktop implementation where a record and attachment is a one to one mapping. So the API might look like record.get_attachment. But this also works.

The way this was sort of grafted onto the RS code isn't great and it's a bit clunky, but it's just the initial step in cleaning up the previous shavar scripts. I'm happy to revisit/update anything here. We can definitely sync further on this.

@issammani Once this lands, I can add a patch for you two to review the attachment portion where yes you can do some part in python however locally it should all work out in terms of record.getAttachment(...name)

issammani · 2024-11-25T22:31:28Z

bootstrap.sh

+# Install Node.js dependencies and build user scripts
+npm install
+npm run build


Nice catch. Not sure why npm was part of content_blocker_update.sh before 🤔

mattreaganmozilla · 2024-11-25T22:39:02Z

content_blocker_update.sh

-cd BrowserKit
-swift run || true
-swift run


Note: I believe removal of this also means we can clean up some of the related code in BrowserKit (e.g. ContentBlockerGenerator)?

Yes. I converted it to a much simpler python script. My initial plan was to use it as is and have docker multistage build ( like I did in this tmp repo ) to build a binary from swift and use it, but that overcomplicates the build process. We don't need that code anymore.

Ok thanks, I might create a follow-up task for that cleanup and just put up a quick separate PR to remove it.

mattreaganmozilla requested review from nbhasin2 and issammani November 23, 2024 00:05

nbhasin2 approved these changes Nov 25, 2024

View reviewed changes

mattreaganmozilla added 6 commits November 25, 2024 13:28

[FXIOS-10667] Remove the old Disconnect file references from Xcode, a…

31d546b

…s well as old content blocker script, and update the bootstrap to no longer call it

[FXIOS-10667] Add xcodeproj references to the new Remote Settings def…

bebb227

…ault disconnect (blocklist) files

[FXIOS-10667] Initial work to load blocklist JSON through remote sett…

a965e1c

…ings APIs rather than directly as bundle resource

[FXIOX-10667] Comment cleanup

eb1c8f5

[FXIOS-10667] Have more generalized RemoteSetting load func call into…

44a908e

… new JSON utility func.

[FXIOS-10667] Use RemoteSettings API for hash check for content lists

95569cf

mattreaganmozilla commented Nov 25, 2024

View reviewed changes

[FXIOS-10667] Add and update tests for RemoteDataTypeRecord

0634f95

mattreaganmozilla force-pushed the mr/10667 branch from a082a72 to 0634f95 Compare November 25, 2024 21:52

mattreaganmozilla marked this pull request as ready for review November 25, 2024 21:53

mattreaganmozilla requested a review from a team as a code owner November 25, 2024 21:53

mattreaganmozilla added the Do Not Merge ⛔️ This issue is a work in progress and is not ready to land label Nov 25, 2024

[FXIOS-10667] Fix missing node npm in bootstrap

6ccd1bb

mattreaganmozilla removed the Do Not Merge ⛔️ This issue is a work in progress and is not ready to land label Nov 25, 2024

issammani approved these changes Nov 25, 2024

View reviewed changes

issammani reviewed Nov 25, 2024

View reviewed changes

mattreaganmozilla commented Nov 25, 2024

View reviewed changes

mattreaganmozilla merged commit c527e91 into mozilla-mobile:main Nov 25, 2024
8 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FXIOS-10667 Content blocklist support via Remote Settings #23342

Add FXIOS-10667 Content blocklist support via Remote Settings #23342

mattreaganmozilla commented Nov 23, 2024

mattreaganmozilla Nov 25, 2024

mattreaganmozilla Nov 25, 2024

mattreaganmozilla Nov 25, 2024

mobiletest-ci-bot commented Nov 25, 2024 •

edited

Loading

mattreaganmozilla commented Nov 25, 2024

issammani left a comment

issammani Nov 25, 2024

mattreaganmozilla Nov 25, 2024

issammani Nov 25, 2024

mattreaganmozilla Nov 25, 2024

nbhasin2 Nov 25, 2024

issammani Nov 25, 2024

mattreaganmozilla Nov 25, 2024

issammani Nov 25, 2024 •

edited

Loading

mattreaganmozilla Nov 25, 2024

		guard let data = try? lists.loadLocalSettingsFileAsJSON(fileName: list) else { continue }
		guard let newHash = calculateHash(for: data) else { continue }

Add FXIOS-10667 Content blocklist support via Remote Settings #23342

Add FXIOS-10667 Content blocklist support via Remote Settings #23342

Conversation

mattreaganmozilla commented Nov 23, 2024

📜 Tickets

💡 Description

Result

📝 Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mobiletest-ci-bot commented Nov 25, 2024 • edited Loading

Client.app: Coverage: 31.58

mattreaganmozilla commented Nov 25, 2024

issammani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

issammani Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mobiletest-ci-bot commented Nov 25, 2024 •

edited

Loading

issammani Nov 25, 2024 •

edited

Loading