Subscription list metadata #13421

antonok-edm · 2022-05-20T23:20:46Z

Resolves brave/brave-browser#17910

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, npm run lint, 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:

Find a subscription list of your choice

Visit https://filterlists.com and use the filter icon on the Syntaxes column to select Adblock Plus lists. Click on the blue (i) icon at the left of an entry of your choice, then click the View button in the info panel. Look for a list that has a ! Title: ... and/or ! Homepage: ... comment near the top.

Subscribe to the list

Copy the URL of the list text from the previous step, then visit brave://adblock and create a new list subscription using that URL (Add filter list via URL button).

Verify updated state

Once the list has been downloaded, visit brave://local-state and identify the entry for the new subscription (under brave -> ad_block -> list_subscriptions). It should have title and homepage fields that match the ones you previously saw in the list text.

antonok-edm · 2022-05-20T23:25:29Z

@nullhook - added you as a reviewer since this is relevant to the brave://adblock UI refresh (#12827). The new JS interface should be self-explanatory from the changes in components/definitions/adBlock.d.ts. I haven't actually added either of these to the UI yet to minimize potential merge conflicts, but it should be straightforward.

@rillian - added you as a reviewer for changes to the Rust FFI. Sadly we don't use cxx here yet so I still had to write it all manually 😞

brave-builds · 2022-05-21T00:16:39Z

A Storybook has been deployed to preview UI for the latest push

rillian

Approach is good. Some comments and questions below.

BTW, the bug mentions exposing the version but this pr only adds title and homepage fields.

rillian · 2022-05-24T14:31:58Z

components/adblock_rust_ffi/cbindgen.toml

-include_guard = "ADBLOCK_RUST_FFI_H"
+include_guard = "BRAVE_COMPONENTS_ADBLOCK_RUST_FFI_SRC_LIB_H_"
+
+header = "/* Copyright (c) 2019 The Brave Authors. All rights reserved.\n * This Source Code Form is subject to the terms of the Mozilla Public\n * License, v. 2.0. If a copy of the MPL was not distributed with this file,\n * You can obtain one at http://mozilla.org/MPL/2.0/. */"


This would be better as a multi-line string. e.g.

header = """\ /* Copyright (c) 2019 The Brave Authors. All rights reserved. * This Source Code Form is ... */"""

Might as well use the current copyright year as well.

I think the policy is not to update the year on copyright headers, but the multi-line string is a good idea.

rillian · 2022-05-24T14:37:52Z

components/adblock_rust_ffi/src/lib.h

 /**
 * Create a new `Engine`.
 */
-struct C_Engine* engine_create(const char* rules);
 struct C_Engine* engine_create_from_buffer(const char* data, size_t data_size);


This should be merged with the new "Create a new Engine" section immediately below.

This file is autogenerated by cbindgen; those comments are taken directly from the doc comments in lib.rs. I can try to make them a little more useful at least.

rillian · 2022-05-24T14:47:39Z

components/adblock_rust_ffi/src/lib.rs

+
+#[no_mangle]
+pub unsafe extern "C" fn engine_create_with_metadata(rules: *const c_char, metadata: *mut *mut FilterListMetadata) -> *mut Engine {
+    let rules = CStr::from_ptr(rules).to_str().unwrap_or("");


Most of the functions use unwrap_or_else(|_| { eprinln!(); ""} to log something about the utf-8 validation failure. Seems like a good idea; is there a reason not to do it everywhere?

Probably just failure to read the docs and connect the two cases together 😅

rillian · 2022-05-24T14:58:39Z

components/adblock_rust_ffi/src/lib.rs

+#[no_mangle]
+pub unsafe extern "C" fn filter_list_metadata_homepage(metadata: *const FilterListMetadata, homepage: *mut *mut c_char) -> bool {
+    if let Some(this_homepage) = (*metadata).homepage.as_ref() {
+        *homepage = CString::new(this_homepage.as_str()).expect("Error: CString::new()").into_raw();


expect aborts here if the input contains a null byte. I assume this String comes from the adblock filter list; is that something we consider sufficiently "under our control" that failing fast is appropriate?

Good point! The lists may come from third-parties in the case of custom subscriptions. The list contents are passed in from C++, which I think handles the early null bytes to begin with. I can double-check on that.

But, either way, expect is probably not the best choice here.

rillian · 2022-05-24T15:04:44Z

components/adblock_rust_ffi/src/lib.rs

+    data_size: size_t,
+    metadata: *mut *mut FilterListMetadata,
+) -> *mut Engine {
+    let data: &[u8] = std::slice::from_raw_parts(data as *const u8, data_size);


I generally include an early return if data.is_null() or if data_size exceeds some reasonable maximum. There's not much to be done about invalid pointers, but I think it's good defensive programming.

That's obviously not directly related to this patch. Just a suggestion for the implementation in general.

Let's address this with the cxx port; it's already equally valid in the previous code and there's currently no handling for not returning an engine pointer from these functions.

Fine with me, although you could return a ptr::null_mut and check for that on the other side.

yea, I'd also like to see if we can get rid of the out param, but waiting for cxx is fine

rillian · 2022-05-24T16:09:21Z

components/adblock_rust_ffi/src/wrapper.h

@@ -5,8 +5,12 @@

 #ifndef BRAVE_COMPONENTS_ADBLOCK_RUST_FFI_SRC_WRAPPER_H_
 #define BRAVE_COMPONENTS_ADBLOCK_RUST_FFI_SRC_WRAPPER_H_
+
+#include <optional>


Please keep this sorted with the other libstd headers.

I had them grouped together and then sorted as per clang-format, but lint was complaining about the order of C vs. C++ headers being wrong. This was the only way I found to satisfy both.

Aha! This is something like a linter bug, since <optional> is definitely a C++ header.

However, std::optional is still banned in chromium code, based on hardening concerns. Best to replace this with absl::optional, and then the include order will make sense. 🙃

antonok-edm · 2022-05-24T22:52:20Z

BTW, the bug mentions exposing the version but this pr only adds title and homepage fields.

@rillian good catch, but yep, that's intended. The version field is supposed to be a numeric string according to the "official" ABP documentation, but I've seen several examples in the wild of lists using semantic versioning or even full human readable dates in the version string. I also haven't seen any adblockers actually using the version code for anything, even exposing it in the UI.

If there's a request for it that makes sense we can add it in pretty easily, but I don't see any good reason to add it for now.

brave-builds · 2022-05-24T23:54:20Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-06-02T02:26:57Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-06-03T19:41:37Z

A Storybook has been deployed to preview UI for the latest push

bridiver · 2022-06-06T17:08:55Z

components/brave_shields/browser/ad_block_subscription_service_manager.cc

+
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  auto info = GetInfo(sub_url);


I believe you are calling GetInfo on more than one thread now. Please add a thread check to it

components/brave_shields/browser/ad_block_subscription_service_manager.cc

brave-builds · 2022-06-06T17:48:01Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-06-06T18:05:53Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-06-06T18:29:21Z

A Storybook has been deployed to preview UI for the latest push

antonok-edm · 2022-06-06T19:55:33Z

Windows CI failures unrelated

antonok-edm added this to the 1.41.x - Nightly milestone May 20, 2022

antonok-edm requested review from bridiver, rillian and nullhook May 20, 2022 23:20

antonok-edm requested a review from a team as a code owner May 20, 2022 23:20

antonok-edm self-assigned this May 20, 2022

github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/storybook-url Deploy storybook and provide a unique URL for each build labels May 20, 2022

rillian approved these changes May 24, 2022

View reviewed changes

antonok-edm force-pushed the subscription-list-metadata branch from a6958c3 to dbfacbf Compare May 24, 2022 22:46

antonok-edm force-pushed the subscription-list-metadata branch from dbfacbf to 1ccc97b Compare May 24, 2022 23:20

antonok-edm force-pushed the subscription-list-metadata branch 2 times, most recently from 266ed7a to 554bc52 Compare June 2, 2022 01:56

antonok-edm mentioned this pull request Jun 3, 2022

Add GUARDED_BY annotations for regional and subscription locks #13610

Merged

25 tasks

update adblock-rust to v0.5.5

4a7db93

antonok-edm force-pushed the subscription-list-metadata branch from 7509127 to 73a3e44 Compare June 6, 2022 17:02

bridiver reviewed Jun 6, 2022

View reviewed changes

components/brave_shields/browser/ad_block_subscription_service_manager.cc Show resolved Hide resolved

antonok-edm added 2 commits June 6, 2022 10:12

read homepage and title metadata from list subscriptions

5fae59d

expose homepage and title to brave://adblock UI frontend code

40200f6

antonok-edm force-pushed the subscription-list-metadata branch from 73a3e44 to 8ce904f Compare June 6, 2022 17:51

bridiver approved these changes Jun 6, 2022

View reviewed changes

additional locking and thread checking fixes

ecbd1ed

antonok-edm force-pushed the subscription-list-metadata branch from 8ce904f to ecbd1ed Compare June 6, 2022 17:59

antonok-edm merged commit abc8f4d into master Jun 6, 2022

antonok-edm deleted the subscription-list-metadata branch June 6, 2022 19:55

antonok-edm mentioned this pull request Jun 6, 2022

Follow up issues on Shields adblock section in settings brave/brave-browser#23222

Closed

nullhook mentioned this pull request Jun 8, 2022

Update and extend List URLs table brave/brave-browser#23341

Open

srirambv mentioned this pull request Jul 7, 2022

Metadata from filter list subscriptions should be available in brave://adblock UI brave/brave-browser#17910

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscription list metadata #13421

Subscription list metadata #13421

antonok-edm commented May 20, 2022 •

edited

Loading

antonok-edm commented May 20, 2022 •

edited

Loading

brave-builds commented May 21, 2022

rillian left a comment

rillian May 24, 2022

antonok-edm May 24, 2022

rillian May 24, 2022

antonok-edm May 24, 2022

rillian May 24, 2022

antonok-edm May 24, 2022

rillian May 24, 2022

antonok-edm May 24, 2022

rillian May 24, 2022

antonok-edm May 24, 2022

rillian May 24, 2022

bridiver Jun 6, 2022

rillian May 24, 2022

antonok-edm May 24, 2022

rillian May 24, 2022

antonok-edm commented May 24, 2022

brave-builds commented May 24, 2022

brave-builds commented Jun 2, 2022

brave-builds commented Jun 3, 2022

bridiver Jun 6, 2022

brave-builds commented Jun 6, 2022

brave-builds commented Jun 6, 2022

brave-builds commented Jun 6, 2022

antonok-edm commented Jun 6, 2022


		DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

		auto info = GetInfo(sub_url);

Subscription list metadata #13421

Subscription list metadata #13421

Conversation

antonok-edm commented May 20, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Find a subscription list of your choice

Subscribe to the list

Verify updated state

antonok-edm commented May 20, 2022 • edited Loading

brave-builds commented May 21, 2022

rillian 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

antonok-edm commented May 24, 2022

brave-builds commented May 24, 2022

brave-builds commented Jun 2, 2022

brave-builds commented Jun 3, 2022

Choose a reason for hiding this comment

brave-builds commented Jun 6, 2022

brave-builds commented Jun 6, 2022

brave-builds commented Jun 6, 2022

antonok-edm commented Jun 6, 2022

antonok-edm commented May 20, 2022 •

edited

Loading

antonok-edm commented May 20, 2022 •

edited

Loading