Replace handwritten adblock rust FFI with CXX based FFI #17368

DJAndries · 2023-02-24T22:41:45Z

Resolves brave/brave-browser#22985

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:

Standard regression testing.

components/brave_shields/adblock/rs/Cargo.toml

ios/browser/api/brave_shields/adblock_engine.mm

ios/browser/api/brave_shields/adblock_engine.h

ios/browser/api/brave_shields/adblock_engine.mm

antonok-edm

Thanks for working on this @DJAndries!! Love to see a diff with over ~~700~~ 1500! lines net removed 😁

I'm gonna submit a followup review for specific APIs I think we can simplify further by removing more of the UTF-8 checks in places they shouldn't be necessary, but this should be enough to work with for now.

components/brave_shields/adblock/rs/Cargo.toml

components/brave_shields/adblock/rs/rustfmt.toml

components/brave_shields/adblock/rs/src/engine.rs

components/brave_shields/adblock/rs/src/lib.rs

components/brave_shields/adblock/rs/src/result.rs

components/brave_shields/common/BUILD.gn

ios/browser/api/brave_shields/adblock_engine.mm

ios/browser/api/brave_shields/adblock_engine.h

components/brave_component_updater/browser/dat_file_util.h

components/brave_shields/adblock/rs/Cargo.lock

DJAndries · 2023-03-11T01:27:59Z

will squash before future merge

antonok-edm

Followed up commenting on the safety of all the APIs returning a result type - should be ok to clean up most of them!

components/brave_shields/adblock/rs/src/engine.rs

components/brave_shields/adblock/rs/Cargo.toml

components/brave_shields/adblock/rs/src/lib.rs

components/brave_shields/adblock/rs/src/result.rs

components/brave_shields/adblock/rs/src/engine.rs

DJAndries · 2023-07-05T06:45:47Z

Rebased and squashed

antonok-edm · 2023-07-13T03:37:33Z

looks like this is just about ready to go - @DJAndries do you mind rebasing? it looks like the only CI failure is from unrelated npm audits.

cc @iefremov, we'll need your +1 too

iefremov · 2023-07-28T12:32:08Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

      (ctx->method == "GET" || ctx->method == "HEAD" ||
       ctx->method == "OPTIONS")) {
-    ctx->new_url_spec = rewritten_url;
+    ctx->new_url_spec = std::string(adblock_result.rewritten_url.value);


maybe we can put this string into a variable

iefremov

browser/net lgtm

…ing that panics

bridiver · 2023-10-27T19:16:44Z

components/brave_shields/adblock/BUILD.gn

+# License, v. 2.0. If a copy of the MPL was not distributed with this file,
+# You can obtain one at https://mozilla.org/MPL/2.0/.
+
+group("rust_lib") {


this is not the correct places for this directory. If we're adding support for things that will be used in ios then you should be using browser/core and browser/content to split things that can/cannot be used on ios

DJAndries requested review from a team and iefremov as code owners February 24, 2023 22:41

github-actions bot assigned DJAndries Feb 24, 2023

github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/run-network-audit Run network-audit labels Feb 24, 2023

DJAndries commented Feb 24, 2023

View reviewed changes

components/brave_shields/adblock/rs/Cargo.toml Outdated Show resolved Hide resolved

kylehickinson reviewed Feb 24, 2023

View reviewed changes

ios/browser/api/brave_shields/adblock_engine.mm Outdated Show resolved Hide resolved

DJAndries commented Feb 24, 2023

View reviewed changes

ios/browser/api/brave_shields/adblock_engine.h Show resolved Hide resolved

DJAndries requested a review from antonok-edm February 24, 2023 22:47

DJAndries force-pushed the adblock-cxx branch 2 times, most recently from df4cc08 to 31ffdcc Compare February 24, 2023 23:50

DJAndries requested a review from bbondy February 25, 2023 00:23

kylehickinson reviewed Feb 27, 2023

View reviewed changes

ios/browser/api/brave_shields/adblock_engine.h Show resolved Hide resolved

kylehickinson reviewed Feb 27, 2023

View reviewed changes

ios/browser/api/brave_shields/adblock_engine.h Show resolved Hide resolved

kylehickinson reviewed Feb 27, 2023

View reviewed changes

ios/browser/api/brave_shields/adblock_engine.mm Outdated Show resolved Hide resolved

antonok-edm reviewed Mar 3, 2023

View reviewed changes

antonok-edm requested a review from rillian March 3, 2023 06:16

rillian reviewed Mar 10, 2023

View reviewed changes

components/brave_shields/adblock/rs/Cargo.lock Outdated Show resolved Hide resolved

DJAndries force-pushed the adblock-cxx branch from 31ffdcc to 816fb47 Compare March 11, 2023 00:54

antonok-edm reviewed Mar 11, 2023

View reviewed changes

DJAndries force-pushed the adblock-cxx branch from 816fb47 to 4bae933 Compare March 14, 2023 04:11

antonok-edm reviewed Mar 16, 2023

View reviewed changes

components/brave_shields/adblock/rs/Cargo.toml Outdated Show resolved Hide resolved

DJAndries force-pushed the adblock-cxx branch from 4bae933 to c2fa2c7 Compare March 17, 2023 21:40

rillian reviewed Apr 4, 2023

View reviewed changes

components/brave_shields/adblock/rs/src/lib.rs Outdated Show resolved Hide resolved

components/brave_shields/adblock/rs/src/result.rs Outdated Show resolved Hide resolved

rillian reviewed Apr 4, 2023

View reviewed changes

components/brave_shields/adblock/rs/src/engine.rs Show resolved Hide resolved

rillian mentioned this pull request Jun 21, 2023

Convert to rust_static_lib #18955

Merged

25 tasks

DJAndries force-pushed the adblock-cxx branch from c2fa2c7 to e318d03 Compare July 5, 2023 06:45

DJAndries force-pushed the adblock-cxx branch 2 times, most recently from a9a2053 to cbdb136 Compare July 6, 2023 01:05

antonok-edm approved these changes Jul 6, 2023

View reviewed changes

DJAndries force-pushed the adblock-cxx branch from cbdb136 to 0062be8 Compare July 7, 2023 02:58

kylehickinson requested a review from Brandon-T July 7, 2023 14:18

DJAndries force-pushed the adblock-cxx branch 2 times, most recently from 7c0b1d7 to 98a70f3 Compare July 9, 2023 22:09

DJAndries force-pushed the adblock-cxx branch 2 times, most recently from f37ab68 to f4b4f79 Compare July 14, 2023 21:18

antonok-edm mentioned this pull request Jul 27, 2023

Adding a custom subscription list with a nonexistent URL crashes on Android brave/brave-browser#31938

Closed

iefremov reviewed Jul 28, 2023

View reviewed changes

iefremov approved these changes Jul 28, 2023

View reviewed changes

DJAndries and others added 3 commits July 28, 2023 11:29

Replace handwritten adblock rust FFI with CXX based FFI

7d4ebe0

Add adblock content blocking rule conversion for iOS in cxx binding

233b46e

[iOS] Add support for new adblock engine API signatures

16ada88

DJAndries force-pushed the adblock-cxx branch from f4b4f79 to 53ceace Compare July 28, 2023 18:29

iccub approved these changes Jul 28, 2023

View reviewed changes

Add non-iOS implementation for adblock convert_rules_to_content_block…

527ce1c

…ing that panics

DJAndries force-pushed the adblock-cxx branch from 53ceace to 527ce1c Compare July 28, 2023 20:02

DJAndries merged commit 3b2a66a into master Jul 28, 2023

DJAndries deleted the adblock-cxx branch July 28, 2023 22:34

github-actions bot added this to the 1.58.x - Nightly milestone Jul 28, 2023

antonok-edm mentioned this pull request Aug 1, 2023

Adblock exception filters don't work properly anymore. brave/brave-browser#32001

Closed

brandonroth mentioned this pull request Aug 15, 2023

How to migrate to Brave-Core CXX bindings brave/adblock-rust-ffi#43

Open

bridiver reviewed Oct 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace handwritten adblock rust FFI with CXX based FFI #17368

Replace handwritten adblock rust FFI with CXX based FFI #17368

DJAndries commented Feb 24, 2023

antonok-edm left a comment •

edited

Loading

DJAndries commented Mar 11, 2023

antonok-edm left a comment

DJAndries commented Jul 5, 2023

antonok-edm commented Jul 13, 2023

iefremov Jul 28, 2023

iefremov left a comment

bridiver Oct 27, 2023

Replace handwritten adblock rust FFI with CXX based FFI #17368

Replace handwritten adblock rust FFI with CXX based FFI #17368

Conversation

DJAndries commented Feb 24, 2023

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

antonok-edm left a comment • edited Loading

Choose a reason for hiding this comment

DJAndries commented Mar 11, 2023

antonok-edm left a comment

Choose a reason for hiding this comment

DJAndries commented Jul 5, 2023

antonok-edm commented Jul 13, 2023

iefremov Jul 28, 2023

Choose a reason for hiding this comment

iefremov left a comment

Choose a reason for hiding this comment

bridiver Oct 27, 2023

Choose a reason for hiding this comment

antonok-edm left a comment •

edited

Loading