Waiting for ContentBlockingRules to be applied before navigation #402

mallexxx · 2022-01-21T10:32:27Z

Task/Issue URL: https://app.asana.com/0/72649045549333/1201190116754047
Tech Design URL: https://app.asana.com/0/inbox/1199237043628108/1201676989048031/1201672261001531
CC: @tomasstrba

Description:

Converts ContentBlockingRules + UserScripts into a single dependency (ContentBlockingAssets) which is injected into UserContentController
Navigation is paused until ContentBlockingAssets become available

see duckduckgo/BrowserServicesKit#54

Steps to test this PR:
0. Remove ~/Library/WebKit/com.duckduckgo.macos.browser[.debug]/ContentRuleLists/*

Open a browser with a restored nytimes.com Tab
Validate there's no ads on the restored Tab
Open privacy dashboard, turn protection off - validate protection is off for the reloaded webpage
Open privacy dashboard, turn protection on - validate it turns on instantly and works on reloaded webpage

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

DuckDuckGo/BrowserTab/Model/UserContentController.swift

tomasstrba · 2022-01-26T18:06:08Z

DuckDuckGo/BrowserTab/Model/Tab.swift

+
+        if navigationAction.isTargetingMainFrame && !url.isDuckDuckGo {
+            RulesCompilationMonitor.shared.tabWillWaitForRulesCompilation(self)
+            let waitTime = await webView.configuration.userContentController.awaitContentBlockingRulesInstalled()


Maybe it is me but in this particular case await is making this section pretty hard to read.

@tomasstrba you mean the whole method or this particular place?
Do you think getting read of isTargetingMainFrame, isDuckDuckGo check and moving waitTime to inout args will make it better?
like this:

RulesCompilationMonitor.shared.tabWillWaitForRulesCompilation(self) var waitTime: TimeInterval? await webView.configuration.userContentController.awaitContentBlockingRulesInstalled(waitTime: &waitTime) RulesCompilationMonitor.shared.tab(self, didFinishWaitingForRulesWithWaitTime: waitTime)

…sets

…ing-rules

tomasstrba

Alex, sorry for jumping in. I will leave the main review to Brindy or Bartek, I just looked to also understand the change. Here are few comments I did while looking at this

tomasstrba · 2022-01-31T10:43:35Z

DuckDuckGo/BrowserTab/Model/Tab.swift

+        }
+
+        if navigationAction.isTargetingMainFrame && !url.isDuckDuckGo {
+            RulesCompilationMonitor.shared.tabWillWaitForRulesCompilation(self)


In case rules are already compiled and ready, suspending until awaitContentBlockingAssetsInstalled is finished makes the loading unnecessary slower.

Also, can we inject RulesCompilationMonitor.shared? It makes it easier to create unit tests of Tab class + it is less spaghetti.

@tomasstrba, are you sure it's waiting when rules are already compiled? I'll check it but the intent here is it should complete instantly when the rules are in place
Good not about RulesCompilationMonitor.shared

Yeah I understand the intent, but to me it looks like there is a cost of the async/await even if it completes immediately. Since this is the critical section of the loading, I would refactor to speed this as much as possible

tomasstrba · 2022-01-31T10:46:07Z

DuckDuckGo/Common/Extensions/PublisherExtension.swift

@@ -27,4 +27,13 @@ extension Publisher where Failure == Never {
        }
    }

+    /// Buffers latest published value and replays it on subscription


Isn't the @published property doing the same? This seems a little over-engineered

Basically yes, it's mostly the same except for optionality: @published requires property to be present from the start, or making it Optional, ShareReplay is a concept from rxSwift missing in Combine for some reason, it basically waits for a published value and then buffers it for a next subscriptor
Why do you think it lacks clarity? For me it's more complex to have a @published value populated from elsewhere to be connected from elsewhere, shareReplay in contrary allows to have this processing be grouped in one place

tomasstrba · 2022-01-31T10:50:46Z

DuckDuckGo/Preferences/Model/PrivacySecurityPreferences.swift

        }
    }
+
+    private let gpcEnabledUpdatesSubject = PassthroughSubject<Bool, Never>()


Please, is there any specific reason why @Published isn't suitable?

I haven't found a good way to combine it with UserDefaultsWrapper, but would be better of course..

tomasstrba · 2022-01-31T10:52:30Z

DuckDuckGo/Statistics/Pixel.swift

@@ -58,7 +58,9 @@ final class Pixel {
        guard !dryRun else {
            os_log(.debug, log: .pixel, "%@", pixelName.replacingOccurrences(of: "_", with: "."))

-            onComplete(nil)
+            DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) {


Please, what is the reason for this delay?

To simulate a server request, it's a "dryrun" path

tomasstrba · 2022-01-31T10:56:06Z

DuckDuckGo/Statistics/RulesCompilationMonitor.swift

+
+import Foundation
+
+final class RulesCompilationMonitor: NSObject {


Names of methods of this class are describing events triggering them. Looking at this class without looking elsewhere makes it pretty hard to understand. Do you think methods should describe their purpose?

Making unit tests to cover this class should help to:

Verify the functionality

Improve the naming

reportWaitTimeForTab(_, waitedForRulesFor: timeInterval) - should be good instead of tab(didFinish).. other methods seem find
I'll add some tests and comments for the class; Considering also renaming it to ContentBlockingRulesCompilationTimeReporter

Thanks 👍 :)

tomasstrba · 2022-01-31T10:58:22Z

DuckDuckGo/TabBar/Model/TabCollection.swift

@@ -48,16 +48,21 @@ final class TabCollection: NSObject {
        }

        saveLastRemovedTab(at: index)
+        tabs[index].tabWillClose()


Just thinking. Is there a better way to do this so we don't forget to call tabWillClose() after adding more remove methods? What is on my mind is subclassing collection (array) and overriding remove.

TabCollection is basically the collection subclass :)
Maybe adding an "assert" to Tab.dealloc if "tabWillClose" (we may set tab content to .none to indicate this) was called would be enough?

Yeah why not, anything that would help us not to miss tabWillClose() in the future

DuckDuckGo/ContentBlocker/ContentBlocking.swift

DuckDuckGo/Statistics/PixelEvent.swift

…ing-rules

…onTime

bwaresiak

Great job so far - I've been testing rule compilation/recompilation and cache gives awesome boost of performance for already precompiled lists.

There are few bugs I've noticed (find in page, no trackers reported) but I supposed that these are because of the UserScripts reuse issue (mentioned in the comment) - once that is resolved I'll give this another test.

bwaresiak · 2022-02-11T00:27:33Z

DuckDuckGo/Statistics/RulesCompilationMonitor.swift

+        // report only once
+        isFinished = true
+
+        Pixel.fire(.compileRulesWait(onboardingShown: self.onboardingShown, waitTime: waitTime, result: result), onComplete: completionHandler)


Can we have exact time as a parameter?

bwaresiak · 2022-02-11T00:55:06Z

DuckDuckGo/ContentBlocker/ContentBlocking.swift

-        contentBlockingRulesSubject.eraseToAnyPublisher()
+        init(rulesUpdate: ContentBlockerRulesManager.UpdateEvent) {
+            self.rulesUpdate = rulesUpdate
+            self.userScripts = UserScripts(with: DefaultScriptSourceProvider())


This doesn't seem like a correct approach: here we create set of user scripts and sent these to all tabs - so whatever tab is created last will become the sole delegate of the scripts. Instead scripts should be created for every Tab separately.

oh, how could I miss it 🤦‍♂️

No worries - I've read that code like 5 times and didn't notice the problem until I started the testing 🙃

mallexxx · 2022-02-11T05:34:41Z

should be good to go now

bwaresiak

Great, scripts seems to work well now, I've left one comment related to performance inline.

One thing that bothers me is that I randomly get assertion from UserContentController.installUserScripts:

        guard let delegate = delegate else {
            assertionFailure("UserContentController delegate not set")
            return
        }

I tried to figure out a way to reproduce that but it seems random - I just leave browser working in background for a while, and sometimes when I come back and trigger protection on/off it causes assertion to fire.

DuckDuckGo/ContentBlocker/ContentBlocking.swift

DuckDuckGo/BrowserTab/Model/UserContentController.swift

…ing-rules

mallexxx · 2022-02-21T10:13:46Z

@bwaresiak I can swear I've caught this assertion once thinking I understand where it comes from, but no luck to reproduce it since then..
So, I added some #if DEBUG leak protection and validation that the delegate is set after init instead this assert, hope it works now

bwaresiak · 2022-02-21T12:54:34Z

DuckDuckGo/BrowserTab/Model/Tab.swift

+        DispatchQueue.main.asyncAfter(deadline: .now() + 1.0) { [weak self,
+                                                                 weak cc=webView.configuration.userContentController] in
+            assert(self == nil)
+            assert(cc == nil)


Wouldn't that fire randomly in some cases e.g when we call faviconManagement.handleFaviconLinks and have to wait for response, but tab is closed? We probably have more async blocks that retain the Tab instance temporarily that could cause this to misfire - but prolly we should change all these blocks to use weak self anyway.

I'll just remove this to let it pass now and make another TechDebt task for this to think about

bwaresiak

Great job @mallexxx!

…ing-rules

* develop: Waiting for ContentBlockingRules to be applied before navigation (#402) Cookie prompt management (#312) Pass config data to Autofill UserScript (#418)

* develop: Waiting for ContentBlockingRules to be applied before navigation (#402) Cookie prompt management (#312) Pass config data to Autofill UserScript (#418) Fix video fullscreen exit crash (#295)

…ntication # By Alexey Martemyanov (1) and others * sam/logins-filtering-and-sorting: Disable the search field when editing. Waiting for ContentBlockingRules to be applied before navigation (#402) Cookie prompt management (#312) Pass config data to Autofill UserScript (#418) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo/Common/Utilities/UserDefaultsWrapper.swift # DuckDuckGo/Preferences/Model/PreferenceSections.swift # DuckDuckGo/Preferences/View/PrivacySecurityPreferencesTableCellView.xib

* rm WebViewConfiguration typealias * async decidePolicyForNavigationAction + wait for content rules * Rules compilation monitor and reporting * Inject UserScripts+ContentBlockingRules as a single ContentBlockingAssets * PR review fixes * fix tests * fix proj * ContentBlockingUpdatingTests and fixes * cleanup * cleanup * fixing tests * fixing tests * Report no wait time on rules loading; Report contentBlockingCompilationTime * fix tests * still fixing tests * and more * linter, please * please.. * the tests * c‘mon! * get rid of it * what you say about no runloop waiting? * figuring out what's wrong * at last * bump BSK * UserScripts generated per-tab * fix unexpected Pixel during tests * compileRulesWait Pixel +waitTime argument * fixing PR issues * leaks prevention * fix tests * cleanup * set BSK to v.10.0.0 * final adjustment

mallexxx added the don’t merge label Jan 21, 2022

mallexxx changed the title ~~ContentBlockingRules sync loading~~ ContentBlockingRules sync loading (WIP) Jan 24, 2022

mallexxx added 2 commits January 26, 2022 14:17

rm WebViewConfiguration typealias

993ce1f

async decidePolicyForNavigationAction + wait for content rules

266ef22

mallexxx force-pushed the alex/content-blocking-rules branch from 5f75f05 to 266ef22 Compare January 26, 2022 07:21

Rules compilation monitor and reporting

d2de703

bwaresiak reviewed Jan 26, 2022

View reviewed changes

DuckDuckGo/BrowserTab/Model/UserContentController.swift Outdated Show resolved Hide resolved

tomasstrba reviewed Jan 26, 2022

View reviewed changes

mallexxx added 2 commits January 31, 2022 15:22

Inject UserScripts+ContentBlockingRules as a single ContentBlockingAs…

927287b

…sets

Merge remote-tracking branch 'origin/develop' into alex/content-block…

daf67f6

…ing-rules

tomasstrba reviewed Jan 31, 2022

View reviewed changes

mallexxx assigned bwaresiak Jan 31, 2022

bwaresiak reviewed Jan 31, 2022

View reviewed changes

DuckDuckGo/ContentBlocker/ContentBlocking.swift Outdated Show resolved Hide resolved

bwaresiak reviewed Jan 31, 2022

View reviewed changes

DuckDuckGo/Statistics/PixelEvent.swift Show resolved Hide resolved

PR review fixes

e7acf83

mallexxx requested a review from tomasstrba February 1, 2022 06:35

mallexxx added 3 commits February 1, 2022 18:57

fix tests

dd629da

fix proj

60f0e3a

ContentBlockingUpdatingTests and fixes

3883238

mallexxx changed the title ~~ContentBlockingRules sync loading (WIP)~~ Waiting for ContentBlockingRules to be applied before navigation Feb 2, 2022

mallexxx added 10 commits February 2, 2022 16:00

cleanup

b10dee4

cleanup

2dc6b87

fixing tests

8214972

fixing tests

74ac65e

Merge remote-tracking branch 'origin/develop' into alex/content-block…

a3bedac

…ing-rules

Report no wait time on rules loading; Report contentBlockingCompilati…

e9d2b3d

…onTime

fix tests

46cb241

still fixing tests

52b730b

and more

251a0c9

linter, please

e686cc3

bump BSK

34c9cdc

bwaresiak reviewed Feb 11, 2022

View reviewed changes

mallexxx added 2 commits February 11, 2022 12:05

UserScripts generated per-tab

5d37638

fix unexpected Pixel during tests

dd1c445

compileRulesWait Pixel +waitTime argument

97d988c

bwaresiak reviewed Feb 11, 2022

View reviewed changes

DuckDuckGo/ContentBlocker/ContentBlocking.swift Outdated Show resolved Hide resolved

mallexxx assigned mallexxx and unassigned bwaresiak Feb 11, 2022

bwaresiak reviewed Feb 17, 2022

View reviewed changes

DuckDuckGo/BrowserTab/Model/UserContentController.swift Outdated Show resolved Hide resolved

mallexxx added 3 commits February 21, 2022 16:33

fixing PR issues

d30ff4e

Merge remote-tracking branch 'origin/develop' into alex/content-block…

3d4cb73

…ing-rules

leaks prevention

00cd295

fix tests

8b79378

bwaresiak reviewed Feb 21, 2022

View reviewed changes

cleanup

9166fa7

bwaresiak approved these changes Feb 21, 2022

View reviewed changes

mallexxx added 4 commits February 22, 2022 16:09

Merge remote-tracking branch 'origin/develop' into alex/content-block…

ae99bb2

…ing-rules

set BSK to v.10.0.0

22e9f67

Merge remote-tracking branch 'origin/develop' into alex/content-block…

db08c0a

…ing-rules

final adjustment

eb96145

mallexxx merged commit 7509c21 into develop Feb 22, 2022

mallexxx deleted the alex/content-blocking-rules branch February 22, 2022 14:11

mallexxx mentioned this pull request Feb 24, 2022

Rename weakAssign to assign(to:onWeaklyHeld:) #442

Merged

1 task

ayoy unassigned mallexxx Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Waiting for ContentBlockingRules to be applied before navigation #402

Waiting for ContentBlockingRules to be applied before navigation #402

mallexxx commented Jan 21, 2022 •

edited

Loading

tomasstrba Jan 26, 2022 •

edited

Loading

mallexxx Jan 27, 2022

tomasstrba left a comment

tomasstrba Jan 31, 2022

tomasstrba Jan 31, 2022

mallexxx Jan 31, 2022

tomasstrba Jan 31, 2022

tomasstrba Jan 31, 2022

mallexxx Jan 31, 2022

tomasstrba Jan 31, 2022

mallexxx Jan 31, 2022

tomasstrba Jan 31, 2022

mallexxx Jan 31, 2022

tomasstrba Jan 31, 2022

tomasstrba Jan 31, 2022

mallexxx Jan 31, 2022

tomasstrba Jan 31, 2022

tomasstrba Jan 31, 2022

mallexxx Jan 31, 2022

tomasstrba Jan 31, 2022

bwaresiak left a comment

bwaresiak Feb 11, 2022

bwaresiak Feb 11, 2022

mallexxx Feb 11, 2022

bwaresiak Feb 11, 2022

mallexxx commented Feb 11, 2022

bwaresiak left a comment

mallexxx commented Feb 21, 2022

bwaresiak Feb 21, 2022

mallexxx Feb 21, 2022

bwaresiak left a comment


		import Foundation

		final class RulesCompilationMonitor: NSObject {

Waiting for ContentBlockingRules to be applied before navigation #402

Waiting for ContentBlockingRules to be applied before navigation #402

Conversation

mallexxx commented Jan 21, 2022 • edited Loading

Internal references:

tomasstrba Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

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

bwaresiak 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

mallexxx commented Feb 11, 2022

bwaresiak left a comment

Choose a reason for hiding this comment

mallexxx commented Feb 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwaresiak left a comment

Choose a reason for hiding this comment

mallexxx commented Jan 21, 2022 •

edited

Loading

tomasstrba Jan 26, 2022 •

edited

Loading