Bat.js fix (fix for installing content blocking rules multiple times) #779
Conversation
| self.add(ruleList) | ||
| guard contentRuleLists[.local(identifier)] == nil else { | ||
| assertionFailure("Local content rule list already installed: \(contentRuleLists[.local(identifier)]!)") | ||
| return |
There was a problem hiding this comment.
I think this is a problem.
I believe above implementation makes an implicit assumption: that every different rule lists set must have different identifier. This is not true, and it is also not something that is either expected on WebKit level.
The the scenario:
- Search for something -> navigate to vendor A. As a result we apply local rule list with "TemporaryAttributed" id, that contains exception for vendor A.
- Go back to results -> navigate to vendor B. At this point we want to replace rules tailored for vendor A, with the ones for vendor B, but both rules share the same identifier - "TemporaryAttributed".
It means in case identifiers clash (one is already installed), we need to remove the old rules first, and just then install the new ones.
We most likely should have a test case for this as it is basically a border condition of this API.
| @@ -36,6 +36,7 @@ extension OSLog { | |||
| }() | |||
|
|
|||
| public enum Categories: String, CaseIterable { | |||
| case contentBlocking = "Content Blocking" | |||
There was a problem hiding this comment.
Using the same name as in macOS app repo will shows "Ambiguous use of 'contentBlocking" when I drop BSK into macOS project.
|
I've tested this, and I believe it solves the problem. Still to be honest I'm concerned if there is anything else we might've missed. The overall side-effect driven nature of this code makes it quite tricky to follow or to foresee the possible issues - like the one we are solving here. I don't think we have a good unit test suite for this, would be good to add something that:
|
mallexxx
added
the
merge on approval
|
This PR has been inactive for more than 7 days and will be automatically closed 7 days from now. |
* main: Enable gzip compression for Sync PATCH payloads (#803) autofill: send current language in runtime config (#808) Password manager survey update (#811) on iOS allow bookmarks in top hits (#812) Rename tests to avoid conflicts (#813) Bat.js fix (fix for installing content blocking rules multiple times) (#779) Pixels automatic naming prefixing fixed (#810)
Task/Issue URL: https://app.asana.com/0/1177771139624306/1205259925028360/f
iOS PR: todo
macOS PR: todo
What kind of version bump will this require?: Patch
CC: @mallexxx
Steps to test this PR:
Internal references:
Software Engineering Expectations
Technical Design Template