inject scriplets on websites on Android as a part of cosmetic filters #6581

SergeyZhukovsky · 2020-09-04T17:22:49Z

Resolves brave/brave-browser#11599

Submitter Checklist:

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

browser/android/brave_cosmetic_resources_tab_helper.cc

antonok-edm · 2020-09-04T17:43:12Z

browser/android/brave_cosmetic_resources_tab_helper.cc

+std::unique_ptr<base::ListValue> GetUrlCosmeticResourcesOnTaskRunner(
+		const std::string& url) {
+  auto result_list = std::make_unique<base::ListValue>();
+
+  base::Optional<base::Value> resources = g_brave_browser_process->
+      ad_block_service()->UrlCosmeticResources(url);
+
+  if (!resources || !resources->is_dict()) {
+    return result_list;
+  }
+
+  base::Optional<base::Value> regional_resources = g_brave_browser_process->
+      ad_block_regional_service_manager()->UrlCosmeticResources(url);
+
+  if (regional_resources && regional_resources->is_dict()) {
+    ::brave_shields::MergeResourcesInto(
+        std::move(*regional_resources), &*resources, /*force_hide=*/false);
+  }
+
+  base::Optional<base::Value> custom_resources = g_brave_browser_process->
+      ad_block_custom_filters_service()->UrlCosmeticResources(url);
+
+  if (custom_resources && custom_resources->is_dict()) {
+    ::brave_shields::MergeResourcesInto(
+        std::move(*custom_resources), &*resources, /*force_hide=*/true);
+  }
+
+  result_list->Append(std::move(*resources));
+
+  return result_list;
+}


We recently moved the regional and custom filters services into the main adblock service; the majority of this function ought to be moved into AdBlockService::UrlCosmeticResources so we can share the same logic on desktop. I can handle this in a follow-up if you'd prefer.

yeah it would be good if you can handle, as I'm not certain of what should be done exactly. Should it be just a one call to something instead of regional and custom calls?

correct - don't worry about it, I'll assign an issue for myself

edit: brave/brave-browser#11600

browser/android/brave_cosmetic_resources_tab_helper.cc

SergeyZhukovsky · 2020-09-04T18:36:41Z

@antonok-edm I force repushed with requested changes. Re review it please when have a chance.

browser/android/brave_cosmetic_resources_tab_helper.cc

SergeyZhukovsky · 2020-09-04T19:21:07Z

corrected and force pushed

antonok-edm

👍

simonhong · 2020-09-05T00:26:45Z

browser/android/brave_cosmetic_resources_tab_helper.cc

+    std::string to_inject;
+    resources_dict->GetString("injected_script", &to_inject);
+    if (to_inject.length() > 1) {
+      content::RenderFrameHost::AllowInjectingJavaScript();


I'm wondering we can call this api freely because upstream doesn't use like us.
I know we've used this api so far. Just curious this is safe. @bridiver ?

// Globally allows for injecting JavaScript into the main world. This feature // is present only to support Android WebView, WebLayer, Fuchsia web.Contexts, // and CastOS content shell. It must not be used in other configurations.

@SergeyZhukovsky since these scripts modify the DOM (which is shared across isolated worlds), can we use ExecuteJavaScriptInIsolatedWorld instead?

Remade it to use ExecuteJavaScriptInIsolatedWorld. @simonhong @bridiver could you please check that?

browser/android/brave_cosmetic_resources_tab_helper.cc

AlexeyBarabash

lgtm

simonhong · 2020-09-09T22:55:41Z

common/brave_isolated_worlds.h

@@ -11,6 +11,9 @@
 enum BraveIsolatedWorldIDs {
    // Isolated world ID for Greaselion (Google Translate reserves END + 1)
    ISOLATED_WORLD_ID_GREASELION = content::ISOLATED_WORLD_ID_CONTENT_END + 2,
+#if defined(OS_ANDROID)
+    ISOLATED_WORLD_ID_COSMETIC_SCRIPTS = content::ISOLATED_WORLD_ID_MAX - 1,


I think our id managing is broken now. We are using same id with chrome's.
It would be good to fix this also here? :)

enum ChromeIsolatedWorldIDs { // Isolated world ID for Chrome Translate. ISOLATED_WORLD_ID_TRANSLATE = content::ISOLATED_WORLD_ID_CONTENT_END + 1, // Isolated world ID for internal Chrome features. ISOLATED_WORLD_ID_CHROME_INTERNAL, #if defined(OS_MAC) // Isolated world ID for AppleScript. ISOLATED_WORLD_ID_APPLESCRIPT, #endif // defined(OS_MAC) // Numbers for isolated worlds for extensions are set in // extensions/renderer/script_injection.cc, and are are greater than or equal // to this number. ISOLATED_WORLD_ID_EXTENSIONS };

https://source.chromium.org/chromium/chromium/src/+/master:chrome/common/chrome_isolated_world_ids.h

@simonhong what do you mean what id do you propose to take? I took the max id - 1, because the latest in the enum you posted ISOLATED_WORLD_ID_EXTENSIONS is actually a first id in the list of unknown size for extensions.

We took it wrong for ISOLATED_WORLD_ID_GREASELION, because it's actually a ISOLATED_WORLD_ID_CHROME_INTERNAL but we don't use it at all.

Hmm, I think patching to upstream file is one method like below.

enum ChromeIsolatedWorldIDs { // Isolated world ID for Chrome Translate. ISOLATED_WORLD_ID_TRANSLATE = content::ISOLATED_WORLD_ID_CONTENT_END + 1, // Isolated world ID for internal Chrome features. ISOLATED_WORLD_ID_CHROME_INTERNAL, #if defined(OS_MAC) // Isolated world ID for AppleScript. ISOLATED_WORLD_ID_APPLESCRIPT, #endif // defined(OS_MAC) BRAVE_ISOLATED_IDS, // <====== Brave // Numbers for isolated worlds for extensions are set in // extensions/renderer/script_injection.cc, and are are greater than or equal // to this number. ISOLATED_WORLD_ID_EXTENSIONS };

and in chromium_src, BRAVE_ISOLATED_IDS is defined with ours. WDYT?

I can do that, no problem, but I don't know it's one extra patch, do we need that on this point? We can always add a patch, and every patch is making rebase longer, but in the other hand it's a small file. @bridiver wdyt?

there are no extensions support on Android

@SergeyZhukovsky Ah!, I forgot about it. Then I think we could use ISOLATED_WORLD_ID_MAX.

I'll pass this decision to @bridiver .
I think using ISOLATED_WORLD_ID_MAX for android seems fine now.
However, if we want to add ids on desktop in the future, this is not extensible and we should revisit this discussion again.

I don't think any of this is necessary including the existing greaselion id. Use ISOLATED_WORLD_ID_CHROME_INTERNAL

Made it to use ISOLATED_WORLD_ID_CHROME_INTERNAL

bridiver · 2020-09-10T14:31:19Z

browser/BUILD.gn

@@ -231,6 +231,8 @@ source_set("browser_process") {
    ]
  } else {
    sources += [
+      "android/brave_cosmetic_resources_tab_helper.cc",


there are a lot of deps in brave_cosmetic_resources_tab_helper and I don't see anything added here, but we need to separate these out or it's going to be impossible to manage. Can you please create brave/browser/android/BUILD.gn? We need to stop creating giant targets like this.

I created a follow up brave/brave-browser#11653 to move everything to a separate BUILD.gn file brave/brave-browser#11653

bridiver · 2020-09-10T14:32:50Z

browser/android/brave_cosmetic_resources_tab_helper.cc

+  return ::brave_shields::ShouldDoCosmeticFiltering(map, url);
+}
+
+std::unique_ptr<base::ListValue> GetUrlCosmeticResourcesOnTaskRunner(


cc @antonok-edm - we need to consolidate this logic

one step ahead of you 😄
#6581 (comment)

bridiver · 2020-09-10T14:33:26Z

browser/android/brave_cosmetic_resources_tab_helper.cc

+namespace {
+bool ShouldDoCosmeticFiltering(content::WebContents* contents,
+    const GURL& url) {
+  Profile* profile = static_cast<Profile*>(contents->GetBrowserContext());


Profile::FromBrowserContext

SergeyZhukovsky added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Sep 4, 2020

SergeyZhukovsky added this to the 1.15.x - Nightly milestone Sep 4, 2020

SergeyZhukovsky requested review from pes10k, simonhong, antonok-edm, AlexeyBarabash and samartnik September 4, 2020 17:22

SergeyZhukovsky self-assigned this Sep 4, 2020

antonok-edm reviewed Sep 4, 2020

View reviewed changes

antonok-edm mentioned this pull request Sep 4, 2020

Move UrlCosmeticResources methods into main adblock instance brave/brave-browser#11600

Closed

SergeyZhukovsky force-pushed the android_cosmetic_filters_js branch from 4def0be to de5dc15 Compare September 4, 2020 18:34

antonok-edm reviewed Sep 4, 2020

View reviewed changes

browser/android/brave_cosmetic_resources_tab_helper.cc Outdated Show resolved Hide resolved

browser/android/brave_cosmetic_resources_tab_helper.cc Outdated Show resolved Hide resolved

SergeyZhukovsky force-pushed the android_cosmetic_filters_js branch from de5dc15 to 69313f3 Compare September 4, 2020 19:20

antonok-edm approved these changes Sep 4, 2020

View reviewed changes

SergeyZhukovsky force-pushed the android_cosmetic_filters_js branch 2 times, most recently from a422b3a to 3bf423c Compare September 4, 2020 21:57

simonhong reviewed Sep 5, 2020

View reviewed changes

AlexeyBarabash reviewed Sep 7, 2020

View reviewed changes

browser/android/brave_cosmetic_resources_tab_helper.cc Show resolved Hide resolved

AlexeyBarabash approved these changes Sep 9, 2020

View reviewed changes

SergeyZhukovsky force-pushed the android_cosmetic_filters_js branch 2 times, most recently from ff34a63 to 96cfe63 Compare September 9, 2020 19:54

simonhong reviewed Sep 9, 2020

View reviewed changes

bridiver reviewed Sep 10, 2020

View reviewed changes

inject scriplets on websites on Android as a part of cosmetic filters

80b4456

SergeyZhukovsky force-pushed the android_cosmetic_filters_js branch from 96cfe63 to 80b4456 Compare September 10, 2020 15:50

bridiver approved these changes Sep 10, 2020

View reviewed changes

SergeyZhukovsky modified the milestones: 1.15.x - Beta, 1.16.x - Nightly Sep 10, 2020

SergeyZhukovsky merged commit b6469ee into master Sep 10, 2020

SergeyZhukovsky deleted the android_cosmetic_filters_js branch September 10, 2020 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inject scriplets on websites on Android as a part of cosmetic filters #6581

inject scriplets on websites on Android as a part of cosmetic filters #6581

SergeyZhukovsky commented Sep 4, 2020

antonok-edm Sep 4, 2020

SergeyZhukovsky Sep 4, 2020

antonok-edm Sep 4, 2020 •

edited

Loading

SergeyZhukovsky commented Sep 4, 2020

SergeyZhukovsky commented Sep 4, 2020

antonok-edm left a comment

simonhong Sep 5, 2020

bridiver Sep 8, 2020

SergeyZhukovsky Sep 9, 2020

AlexeyBarabash left a comment

simonhong Sep 9, 2020

SergeyZhukovsky Sep 9, 2020

SergeyZhukovsky Sep 9, 2020 •

edited

Loading

simonhong Sep 9, 2020

SergeyZhukovsky Sep 9, 2020

SergeyZhukovsky Sep 9, 2020

simonhong Sep 9, 2020 •

edited

Loading

simonhong Sep 9, 2020

bridiver Sep 10, 2020

SergeyZhukovsky Sep 10, 2020

bridiver Sep 10, 2020

SergeyZhukovsky Sep 10, 2020

bridiver Sep 10, 2020

antonok-edm Sep 10, 2020

bridiver Sep 10, 2020

SergeyZhukovsky Sep 10, 2020

inject scriplets on websites on Android as a part of cosmetic filters #6581

inject scriplets on websites on Android as a part of cosmetic filters #6581

Conversation

SergeyZhukovsky commented Sep 4, 2020

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonok-edm Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

SergeyZhukovsky commented Sep 4, 2020

SergeyZhukovsky commented Sep 4, 2020

antonok-edm 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

AlexeyBarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyZhukovsky Sep 9, 2020 • 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

simonhong Sep 9, 2020 • 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

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 Sep 4, 2020 •

edited

Loading

SergeyZhukovsky Sep 9, 2020 •

edited

Loading

simonhong Sep 9, 2020 •

edited

Loading