initial FB Click to Load (WIP) #329

ladamski · 2021-11-16T03:55:29Z

There are some temporary hacks in TrackerRadarManager and ContentBlockerRulesManager to handle issues at startup, these will have to be resolved before merging

There is some initial scaffolding to support loading of remote configuration but not yet enabled due to difference in config files for extension vs MacOS

Task/Issue URL: https://app.asana.com/0/72649045549333/1201196366848275/f
Tech Design URL: https://app.asana.com/0/72649045549333/1201230845721463
CC:

Description:

Steps to test this PR:

https://privacy-test-pages.glitch.me/privacy-protections/click-to-load/

Testing checklist:

Test with Release configuration

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

ladamski · 2021-11-16T03:56:39Z

@tomasstrba here is the WIP, as basis for our discussion

tomasstrba

Lucas, nice!!! 👏 👏 👏 The biggest issues are in comments below. Architecture looks pretty good. There are also details like debug prints, etc but I expect you have them just for debugging.

tomasstrba · 2021-11-16T20:54:08Z

DuckDuckGo/ContentBlocker/ClickToLoadUserScript.swift

+            replyHandler(nil, "Image not found")
+            return
+        }
+        let base64String = try? Data(contentsOf: imgURL!).base64EncodedString()


My advice is to use Assets.xcassets and static variables since there are ~6 images. The advantage is lazy loading and having images in memory instead of reading them from disk every time.

Please see PrivacyIconViewModel.swift for an example

BTW do you know of a way to convert an SVG represented as NSImage (or cgImage for that matter) to a Data or String representation? For PNG I can use NSImage.tiffRepresentation, but not for SVG.

Does it really make sense to use Assets.xcassets since the app itself never uses these images but just passes b64 encoded representations to the user script?

Or if there's a way load the raw files from Assets? I've tried NSDataAsset(name: "blocked_facebook_logo") but it just returns nil...?

Sorry Lucas, I didn't know it is tricky to get raw data if NSImage is SVG. Feel free to use your original approach, but please use lazy loading of images with some sort of cache. It means, images shouldn't be loaded immediately after start if they aren't used and also shouldn't be loaded from disk every time they are used.

tomasstrba · 2021-11-16T21:01:37Z

DuckDuckGo/ContentBlocker/PrivacyConfigurationManager.swift

    static func loadEmbeddedAsData() -> Data {
        let json = try? Data(contentsOf: embeddedUrl)
        return json!
    }

+    static func loadFBConfig() -> Data {


Please, is this used somewhere?

Not yet, as mentioned in the intro I started doing the plumbing for loading the remote config, but shelved it for now since our config diverged from the extension version. We'll be revising these configs shortly at which point we'll use this.

tomasstrba · 2021-11-16T21:03:00Z

DuckDuckGo/ContentBlocker/ScriptSourceProviding.swift

+        )
+        var data = ""
+        do {
+            data = try String(contentsOf: sdkUrl!)


Instead of forced unwrapping that causes crash if sdkUrl is nil, we usually unwrap using guard let .... It is more stable and you can react to nil in more appropriate way.

TBH we probably should crash if that file is missing? I don't see any way to recover & continue.

If the file is part of the bundle, then yeah crash makes sense. A slightly better practice is fatalError and appropriate error message.

There are some temporary hacks in TrackerRadarManager and ContentBlockerRulesManager to handle issues at startup, these will have to be resolved before merging There is some initial scaffolding to support loading of remote configuration but not yet enabled due to difference in config files for extension vs MacOS

…-rule-lists' into la/fb-click-to-load

There is a long delay before the complete event fires, meaning we were not blocking anything for ~10 seconds upon startup

bstandaert-ddg · 2022-01-03T18:02:08Z

@ladamski Do you have any suggestions for how to test this?

ladamski · 2022-01-03T18:12:32Z

@ladamski Do you have any suggestions for how to test this?

@bstandaert-ddg: Sure you can start with https://privacy-test-pages.glitch.me/privacy-protections/click-to-load/, comparing the behavior of this branch vs the current release build (from develop branch). You should see what is blocked, and be able to click through to unblock.

tomasstrba

@ladamski, I compiled and tried this and it is such a cool feature! Can't wait to have this in the released app and tell all my friends :)

I only found one issue - "Custom Facebook Login" button opens two dialogs. I needed to click "Go back" twice to get rid of it.

Other than that it is perfect and I only have some comments related to code below, which are not important and not blockers.

DuckDuckGo/BrowserTab/Model/Tab.swift

DuckDuckGo/ContentBlocker/ClickToLoadModel.swift

DuckDuckGo/ContentBlocker/ClickToLoadUserScript.swift

tomasstrba · 2022-01-19T21:07:59Z

DuckDuckGo/ContentBlocker/ContentBlockerRulesLists.swift

+
+    static var fbTrackerDataSet: TrackerRadarKit.TrackerData = {
+        let trackerData = (try? JSONDecoder().decode(TrackerData.self, from: fbTrackerDataFile)) ?? nil
+        return trackerData!


Just to be sure. Are we using just a local json file to decode tracker data?

Yes, right now there is no corresponding remote file. All of the CTL configs + TDS need to be refactored, which will come after this lands (these all diverged between extension / Youtube / Mac and need to be integrated again).

ladamski · 2022-01-19T23:52:10Z

I addressed the issue with the duplicate window opening

tomasstrba

LGTM! 👍 🥇

Tested again, works like a charm

tomasstrba · 2022-01-20T10:46:19Z

@ladamski, I can't merge because BSK points to feature/bartek/multiple-blocking-rule-lists. We can merge right after BSK changes land

bwaresiak · 2022-01-24T13:22:31Z

DuckDuckGo/ContentBlocker/ContentBlockerRulesLists.swift

+
+    static var fbTrackerDataFile: Data = {
+        let url = Bundle.main.url(forResource: "fb-tds", withExtension: "json")!
+        return (try? Data(contentsOf: url)) ?? Data()


Important question: do we have test to ensure that this file compiles? For every embedded rule list we should have tests that check if everything is ok before the release - both parsing to TrackerData format and compilation itself.

I've noticed that embedded rules compilation has not been done on BSK either, so I've added that on the click to load BSK branch - you can use that as a reference.

@bwaresiak not an explicit loading test but from my testing it does fail here if I specify an invalid FB TDS. It would blow up on a nil value at #37, but I just made it a bit more explicit so it will do so with:

Test Case '-[Unit_Tests.AppStateChangePublisherTests testWhenAllTabsExceptOneClosedThenStateChangePublished]' started. DuckDuckGo_Privacy_Browser/ContentBlockerRulesLists.swift:43: Fatal error: Failed to JSON decode FB-TDS 2022-01-24 20:55:46.322148-0800 DuckDuckGo[78928:4548503] DuckDuckGo_Privacy_Browser/ContentBlockerRulesLists.swift:43: Fatal error: Failed to JSON decode FB-TDS

And for compilation errors, I just added a test per your example, thank you!

* develop: Version 0.18.5 support privacy config for clickToLoad (#407) Automatically select available login (#405) initial FB Click to Load (WIP) (#329) onboarding updates (#398) Version 0.18.4 Configuration of Sparkle - Setting SUAllowsAutomaticUpdates to NO (#404) Hide downloads button if the popover is opened/closed manually (#397)

# By Alexey Martemyanov (20) and others # Via Tomas Strba (2) and others * develop: (63 commits) Tweaks of suggestions and autocomplete (#403) Bump privacy dashboard to latest version (#409) Point to the latest BrowserServicesKit branch. (#414) Move embedded TDS from BSK to platform repo (#412) Image of shield with dot replaced (#410) Expand Fireproofing to include Local Storage and IndexedDB (#408) Version 0.18.5 support privacy config for clickToLoad (#407) Automatically select available login (#405) initial FB Click to Load (WIP) (#329) onboarding updates (#398) Version 0.18.4 Configuration of Sparkle - Setting SUAllowsAutomaticUpdates to NO (#404) Hide downloads button if the popover is opened/closed manually (#397) Textfield of the homepage is empty and unfocused right after switching to the homepage (#400) Remove navigatorCredentials (#392) Remove GPC header if it exists when not needed (#366) Version 0.18.3 Fireproofing encrypted storage (#332) Fix Lock Screen UI issues (#399) ... # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo/Crash Reports/Model/CrashReportSender.swift

ladamski added the in progress label Nov 16, 2021

tomasstrba self-requested a review November 16, 2021 18:15

tomasstrba self-assigned this Nov 16, 2021

tomasstrba reviewed Nov 16, 2021

View reviewed changes

tomasstrba assigned ladamski and unassigned tomasstrba Nov 16, 2021

[email protected] added 8 commits December 3, 2021 13:53

refactor icon loading into model

28e9733

cleanup fb-sdk.js

b6d659b

cleanup clickToLoad.js

cead07f

rebase clickToLoad.js

77a83e2

Update fb-tds.json

ca33bd1

Update Package.resolved

d368d55

restore ClickToLoadModel.swift

8272d5e

ladamski force-pushed the la/fb-click-to-load branch from c8f8c85 to 8272d5e Compare December 6, 2021 06:26

bwaresiak and others added 9 commits December 17, 2021 20:07

Update to use multiple rules list API

91188ec

Merge remote-tracking branch 'origin/feature/bartek/multiple-blocking…

b3f603d

…-rule-lists' into la/fb-click-to-load

multi list support WIP

e05fa22

Update to Content Blocking API

eae78b0

support multiple rule lists

beeb3f6

cleanup debug

4b12665

fix CTL userscript race condition

607aedc

There is a long delay before the complete event fires, meaning we were not blocking anything for ~10 seconds upon startup

support exceptions for FB CTL

0324967

handle empty hostname, cleanup console

cbc2806

ladamski requested a review from tomasstrba January 3, 2022 06:53

tomasstrba assigned tomasstrba and unassigned ladamski Jan 3, 2022

tomasstrba requested changes Jan 19, 2022

View reviewed changes

tomasstrba assigned ladamski and unassigned tomasstrba Jan 19, 2022

[email protected] added 4 commits January 19, 2022 15:21

refactor ClickToLoadModel with tests

b1de248

fix dupe surrogate injection on slow page loads

683ac56

FBblocked -> fbBlockingEnabled

1b5223e

fix ClickToLoadModelTests method name

4a08a95

ladamski requested a review from tomasstrba January 19, 2022 23:48

ladamski assigned tomasstrba and unassigned ladamski Jan 19, 2022

ladamski added the merge on approve label Jan 19, 2022

tomasstrba approved these changes Jan 20, 2022

View reviewed changes

tomasstrba added don’t merge and removed merge on approve labels Jan 20, 2022

tomasstrba assigned ladamski and unassigned tomasstrba Jan 20, 2022

SlayterDev mentioned this pull request Jan 21, 2022

Enable ctl mac duckduckgo/privacy-configuration#113

Closed

bwaresiak reviewed Jan 24, 2022

View reviewed changes

[email protected] added 4 commits January 24, 2022 21:01

explicit failure on corrupt FB TDS

91c7af5

add CTL TDS parsing test

2498324

nit

0de1110

update BSK rev

1cf6f05

ladamski merged commit 1675e32 into develop Jan 25, 2022

ladamski deleted the la/fb-click-to-load branch January 25, 2022 23:40

ladamski mentioned this pull request Jan 27, 2022

enable CTL for macos duckduckgo/privacy-configuration#119

Merged

ayoy unassigned ladamski Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial FB Click to Load (WIP) #329

initial FB Click to Load (WIP) #329

ladamski commented Nov 16, 2021 •

edited

Loading

ladamski commented Nov 16, 2021

tomasstrba left a comment

tomasstrba Nov 16, 2021

ladamski Nov 19, 2021 •

edited

Loading

tomasstrba Nov 19, 2021

tomasstrba Nov 16, 2021

ladamski Nov 16, 2021

tomasstrba Nov 16, 2021

ladamski Nov 16, 2021

tomasstrba Nov 18, 2021

bstandaert-ddg commented Jan 3, 2022

ladamski commented Jan 3, 2022 •

edited

Loading

tomasstrba left a comment

tomasstrba Jan 19, 2022

ladamski Jan 19, 2022

ladamski commented Jan 19, 2022

tomasstrba left a comment

tomasstrba commented Jan 20, 2022

bwaresiak Jan 24, 2022

bwaresiak Jan 24, 2022

ladamski Jan 25, 2022 •

edited

Loading

initial FB Click to Load (WIP) #329

initial FB Click to Load (WIP) #329

Conversation

ladamski commented Nov 16, 2021 • edited Loading

Internal references:

ladamski commented Nov 16, 2021

tomasstrba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladamski Nov 19, 2021 • edited Loading

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

bstandaert-ddg commented Jan 3, 2022

ladamski commented Jan 3, 2022 • edited Loading

tomasstrba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladamski commented Jan 19, 2022

tomasstrba left a comment

Choose a reason for hiding this comment

tomasstrba commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ladamski Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

ladamski commented Nov 16, 2021 •

edited

Loading

ladamski Nov 19, 2021 •

edited

Loading

ladamski commented Jan 3, 2022 •

edited

Loading

ladamski Jan 25, 2022 •

edited

Loading