Include component versions in webcompat reports #25688

vadimstruts · 2024-09-22T21:55:58Z

Resolves brave/brave-browser#35674

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
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 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:

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

Test Plan:

Make sure that Webcompat Report contains the component versions information.
Steps:

Open Brave Shields pop up dialog
Switch off Brave Shields if It is ON
Click "Report broken site", then click "Submit"
Go to the webcompat reporter dashboard and see the component versions information.

goodov · 2024-10-11T15:00:19Z

browser/ui/webui/webcompat_reporter/webcompat_reporter_ui.cc

@@ -50,12 +52,14 @@ namespace {
 constexpr char kUISourceHistogramName[] = "Brave.Webcompat.UISource";
 constexpr int kMaxScreenshotPixelCount = 1280 * 720;

+const std::string BoolToString(bool value) {


Suggested change

const std::string BoolToString(bool value) {

std::string BoolToString(bool value) {

goodov · 2024-10-15T09:10:16Z

browser/webcompat_reporter/webcompat_reporter_service_factory.cc

+content::BrowserContext*
+WebcompatReporterServiceFactory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  return chrome::GetBrowserContextRedirectedInIncognito(context);


pls use ProfileKeyedServiceFactory configured for incognito appropriately.

now that you have ProfileKeyedServiceFactory, you need to remove GetBrowserContextToUse override.

goodov · 2024-10-15T09:11:27Z

browser/ui/webui/webcompat_reporter/webcompat_reporter_ui.h

@@ -63,7 +63,8 @@ class WebcompatReporterDOMHandler : public content::WebUIMessageHandler {

  raw_ptr<content::RenderWidgetHostView> render_widget_host_view_;
  scoped_refptr<base::SequencedTaskRunner> ui_task_runner_;
-  std::unique_ptr<webcompat_reporter::WebcompatReportUploader> uploader_;
+  std::unique_ptr<webcompat_reporter::WebcompatReporterService>
+      reporter_service_;


is this variable used somehow or is it a leftover? It looks wrong, because you should never own a KeyedService.

it was leftover

goodov · 2024-10-15T09:12:13Z

browser/webcompat_reporter/webcompat_reporter_service_delegate.h

+
+ private:
+  raw_ptr<component_updater::ComponentUpdateService> component_update_service_;
+  raw_ptr<brave_shields::AdBlockService> adblock_service_;


const raw_ptr<...> both members.

goodov · 2024-10-15T09:15:39Z

browser/webcompat_reporter/webcompat_reporter_service_factory.cc

+
+// static
+mojo::PendingRemote<mojom::WebcompatReporterHandler>
+WebcompatReporterServiceFactory::GetForContext(


GetMojoReportHandlerForContext

goodov · 2024-10-15T09:44:35Z

ios/brave-ios/Sources/Brave/Frontend/Shields/SubmitReportView.swift

+        format: "%@ (%@)",
+        AppInfo.appVersion,
+        AppInfo.buildNumber
+      ),


channel is missing?

no, channel has separate property: "channel": "developer"
mentioned code helps to combine version value. ex: "version": "1.73 (0)"

ah, I see it's filled later in WebcompatReporter.swift. Not sure why those are separated.. maybe everything can be filled in one place?

yeah, I removed WebcompatReporter.swift, so now all things are in the same place

goodov · 2024-10-15T09:45:55Z

ios/browser/api/webcompat_reporter/BUILD.gn

+  ]
+
+  deps = [
+    ":webcompat_reporter_mojom_wrappers",


shouldn't this be a public_deps instead?

goodov · 2024-10-15T09:47:06Z

ios/browser/api/webcompat_reporter/webcompat_reporter_service_delegate.h

+  std::optional<std::vector<ComponentInfo>> GetComponentInfos() const override;
+
+ private:
+  raw_ptr<component_updater::ComponentUpdateService> component_update_service_;


ios/browser/api/webcompat_reporter/webcompat_reporter_service_delegate.mm

goodov · 2024-10-15T09:51:04Z

ios/browser/api/webcompat_reporter/webcompat_reporter_service_delegate.mm

+    return std::nullopt;
+  }
+
+  return result;


seems like this exact code is duplicated. Pls create a base delegate impl that will be shared on iOS and desktop/android.

...oid/java/org/chromium/chrome/browser/webcompat_reporter/WebcompatReporterServiceFactory.java

AlexeyBarabash · 2024-10-16T20:52:59Z

...oid/java/org/chromium/chrome/browser/webcompat_reporter/WebcompatReporterServiceFactory.java

+    public WebcompatReporterHandler getWebcompatReporterHandler(
+            ConnectionErrorHandler connectionErrorHandler) {
+        Profile profile = Utils.getProfile(false); // Always use regular profile
+        if (profile == null) {


Utils.getProfile is marked as @NonNull and it tries to get the profile in several different ways, so I would not use the null check

AlexeyBarabash

Android Java code lgtm.

These imports look strange:

import org.chromium.chrome.browser.crypto_wallet.util.Utils;
at src/brave/android/java/org/chromium/chrome/browser/webcompat_reporter/WebcompatReporterServiceFactory.java

and

import org.chromium.chrome.browser.BraveRewardsHelper;
import org.chromium.chrome.browser.BraveRewardsNativeWorker;

at src/brave/android/java/org/chromium/chrome/browser/shields/BraveShieldsHandler.java

And this is an issue beyond the current PR, crypto_wallet.util.Utils and BraveRewardsHelper are widely used beyond rewards/wallet.

Follow-up issues:
brave/brave-browser#41689
brave/brave-browser#41690

goodov · 2024-10-17T05:25:21Z

ios/browser/api/webcompat_reporter/webcompat_reporter_service_factory_wrapper.mm

+  auto service =
+      webcompat_reporter::WebcompatReporterServiceFactory::GetForBrowserState(
+          browserState);
+  if (!service) {


this is a mojo interface, not a keyed service.

I think the variable and the method name GetForBrowserState should be renamed accordingly to not confuse people.

goodov · 2024-10-17T05:27:52Z

browser/webcompat_reporter/webcompat_reporter_service_factory.cc

+content::BrowserContext*
+WebcompatReporterServiceFactory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  return chrome::GetBrowserContextRedirectedInIncognito(context);


now that you have ProfileKeyedServiceFactory, you need to remove GetBrowserContextToUse override.

components/webcompat_reporter/browser/BUILD.gn

goodov · 2024-10-17T05:29:51Z

components/webcompat_reporter/browser/BUILD.gn

+  if (!is_ios) {
+    deps += [
+      "//brave/components/brave_shields/content/browser",
+      "//chrome/test:test_support",


what is it for? you shouldn't have //chrome in components.

goodov · 2024-10-17T05:43:34Z

components/webcompat_reporter/browser/webcompat_report_uploader.h

+  std::optional<std::string> brave_vpn_connected;
+  std::optional<std::string> details;
+  std::optional<std::string> contact;
+  std::optional<base::Value> ad_block_components;


you added a mojo structure that's just similar to this one. Do we really need to have both? Why not just use mojo structure everywhere from now on? Seems like the right thing to do.

goodov · 2024-10-18T09:30:38Z

components/webcompat_reporter/browser/webcompat_report_uploader.cc

+        ConvertCompsToValue(report_info->ad_block_components_version.value());
+    if (components) {
+      report_details_dict.Set(kAdBlockComponentsVersionField,
+                              components->Clone());


you can let ConvertCompsToValue always return a value and simplify things here a bit:

if (report_info->ad_block_components_version && !report_info->ad_block_components_version->empty()) { report_details_dict.Set(kAdBlockComponentsVersionField, ConvertCompsToValue(report_info->ad_block_components_version.value())); }

goodov · 2024-10-18T09:35:16Z

components/webcompat_reporter/browser/webcompat_report_uploader_unittest.cc

+  std::unique_ptr<WebcompatReportUploader> webcompat_report_uploader_;
+  network::TestURLLoaderFactory url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
+  base::test::TaskEnvironment task_environment_{


task environment should go first, otherwise you might hit uaf on test destruction.

goodov · 2024-10-18T09:38:33Z

components/webcompat_reporter/browser/webcompat_reporter_service_unittest.cc

+        std::move(delegate), nullptr);
+
+    webcompat_reporter_service_->SetReportUploaderForTest(
+        std::unique_ptr<WebcompatReportUploader>(


why not pass this directly into constructor and remove SetReportUploaderForTest?

also std::make_unique<>.

goodov · 2024-10-18T09:42:48Z

ios/browser/api/webcompat_reporter/webcompat_reporter_service_factory_wrapper.mm

+ (nullable id)serviceForBrowserState:(ChromeBrowserState*)browserState {
+  auto service = webcompat_reporter::WebcompatReporterServiceFactory::
+      GetMojoReportHandlerForContext(browserState);
+  if (!service) {


goodov · 2024-10-18T09:43:17Z

ios/browser/api/webcompat_reporter/webcompat_reporter_service_factory_wrapper.mm

+#include "ios/chrome/browser/shared/model/profile/profile_ios.h"
+
+@implementation WebcompatReporterServiceFactory
+ (nullable id)serviceForBrowserState:(ChromeBrowserState*)browserState {


shouldn't this be handlerForBrowserState?

goodov · 2024-10-18T09:49:16Z

ios/brave-ios/Sources/BraveShields/WebcompatReporter.swift

-      Logger.module.error(
-        "Failed to setup webcompat request payload: \(error.localizedDescription)"
+  public static func send(
+    braveVersion: String?,


I'm not sure why this function should list all these parameters? Can it just receive the mojo struct type?

goodov · 2024-10-18T09:52:14Z

components/webcompat_reporter/browser/webcompat_reporter_utils.cc

+namespace webcompat_reporter {
+
+bool NeedsToGetComponentInfo(const std::string& component_id) {
+  static const base::NoDestructor<base::flat_set<std::string>> kComponentIds({


can it just be base::MakeFixedFlatSet<std::string_view>{...}? everything here should be constexpr.

the function can also receive std::string_view instead of std::string.

goodov · 2024-10-18T09:54:53Z

browser/webcompat_reporter/webcompat_reporter_service_factory.cc

+
+// static
+mojo::PendingRemote<mojom::WebcompatReporterHandler>
+WebcompatReporterServiceFactory::GetMojoReportHandlerForContext(


hm.. let's actually call it GetHandlerForContext.

goodov · 2024-10-18T10:10:25Z

ios/brave-ios/Sources/Brave/Frontend/Shields/SubmitReportView.swift

+        format: "%@ (%@)",
+        AppInfo.appVersion,
+        AppInfo.buildNumber
+      ),


ah, I see it's filled later in WebcompatReporter.swift. Not sure why those are separated.. maybe everything can be filled in one place?

goodov · 2024-10-18T10:12:52Z

ios/brave-ios/Sources/BraveShields/WebcompatReporter.swift

-  private static var currentLanguageCode: String? {
-    return Locale.current.language.languageCode?.identifier
-  }
+public class WebcompatReporterUploader {


do we need this class at all? can't we just call

privateMode: false )! webcompatReporterAPI.submitWebcompatReport

in the frontend?

ios/brave-ios/Sources/Brave/Frontend/Shields/SubmitReportView.swift

ios/browser/api/webcompat_reporter/webcompat_reporter_service_factory_wrapper.mm

ios/browser/api/webcompat_reporter/webcompat_reporter_service_factory.mm

StephenHeaps

iOS just needs a format for presubmit check, but lgtm 👍.

Signed-off-by: Vadym Struts <vstruts@brave.com>

…d to add unittests Signed-off-by: Vadym Struts <vstruts@brave.com>

Signed-off-by: Vadym Struts <vstruts@brave.com>

Signed-off-by: vadym <vadym@vadyms-iMac-Pro.local>

Signed-off-by: Vadym Struts <vstruts@brave.com>

github-actions · 2024-10-21T09:45:46Z

[puLL-Merge] - brave/brave-core@25688

Description

This pull request implements a new WebcompatReporterService for reporting web compatibility issues in Brave browser. It adds functionality to submit reports about website issues, including details about the browser's configuration and any user-provided information. The implementation spans across Android, iOS, and desktop platforms.

Changes

Changes

android/brave_java_sources.gni:
- Added a new Java source file for WebcompatReporterServiceFactory.
android/java/org/chromium/chrome/browser/shields/BraveShieldsHandler.java:
- Integrated the new WebcompatReporterHandler for submitting reports.
- Removed AsyncTask usage and replaced it with the new service.
android/java/org/chromium/chrome/browser/webcompat_reporter/WebcompatReporterServiceFactory.java:
- Added a new file to create and manage WebcompatReporterHandler instances.
browser/browser_context_keyed_service_factories.cc:
- Added WebcompatReporterServiceFactory to the list of service factories.
browser/sources.gni:
- Included new webcompat reporter sources and dependencies.
browser/ui/BUILD.gn:
- Added dependency on the new webcompat reporter mojom.
browser/ui/webui/webcompat_reporter/webcompat_reporter_ui.cc:
- Updated to use the new WebcompatReporterService for report submission.
browser/webcompat_reporter/:
- Added new files for the WebcompatReporterService implementation, including service delegate and factory.
components/webcompat_reporter/browser/:
- Updated and added files for the core implementation of the WebcompatReporterService.
components/webcompat_reporter/common/:
- Added new mojom file defining the WebcompatReporterHandler interface.
ios/:
- Added implementations specific to iOS for the WebcompatReporterService.
test/BUILD.gn:
- Added unit tests for the new webcompat reporter component.

Possible Issues

The removal of NTPBackgroundImagesBridge in the Android implementation might affect functionality related to New Tab Page background images.
The change from AsyncTask to the new service in Android might introduce threading issues if not properly managed.

Security Hotspots

The handling of user-provided data in report submissions should be carefully reviewed to ensure proper sanitization and to prevent potential injection attacks.
The use of API keys and sensitive browser information in reports should be audited to ensure no unnecessary data exposure.

This PR significantly refactors the web compatibility reporting system in Brave, centralizing the functionality and making it more consistent across platforms. It also introduces a more modular and maintainable architecture for handling these reports.

Signed-off-by: Vadym Struts <vstruts@brave.com>

brave-builds · 2024-10-21T16:05:07Z

Released in v1.73.31

cdesouza-chromium · 2024-11-05T12:59:45Z

browser/webcompat_reporter/webcompat_reporter_service_factory.cc

+KeyedService* WebcompatReporterServiceFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  auto* default_storage_partition = context->GetDefaultStoragePartition();
+  if (!default_storage_partition) {
+    return nullptr;
+  }
+
+  auto report_uploader = std::make_unique<WebcompatReportUploader>(
+      default_storage_partition->GetURLLoaderFactoryForBrowserProcess());
+  return new WebcompatReporterService(
+      std::make_unique<WebcompatReporterServiceDelegateImpl>(
+          g_browser_process->component_updater(),
+          g_brave_browser_process->ad_block_service()),
+      std::move(report_uploader));
+}


BuildServiceInstanceFor has been deprecated for a while now, and all instances of it in brave have been removed in the past. This PR has introduced a new case though. Leaving a comment to as a PSA.

[1] https://issues.chromium.org/issues/40249337
[2] #25833

github-actions bot assigned vadimstruts Sep 22, 2024

vadimstruts force-pushed the 35674-include-component-versions-webcompat-reports branch 3 times, most recently from 9a6a930 to dc56223 Compare October 2, 2024 11:54

vadimstruts marked this pull request as ready for review October 3, 2024 11:49

vadimstruts requested review from a team as code owners October 3, 2024 11:49

vadimstruts marked this pull request as draft October 3, 2024 20:11

vadimstruts force-pushed the 35674-include-component-versions-webcompat-reports branch from ea63de7 to b54f416 Compare October 10, 2024 12:55

vadimstruts marked this pull request as ready for review October 11, 2024 13:38

vadimstruts requested a review from a team as a code owner October 11, 2024 13:38

StephenHeaps self-requested a review October 11, 2024 14:03

goodov reviewed Oct 15, 2024

View reviewed changes

github-actions bot added the puLL-Merge label Oct 16, 2024

AlexeyBarabash reviewed Oct 16, 2024

View reviewed changes

...oid/java/org/chromium/chrome/browser/webcompat_reporter/WebcompatReporterServiceFactory.java Outdated Show resolved Hide resolved

AlexeyBarabash reviewed Oct 16, 2024

View reviewed changes

AlexeyBarabash approved these changes Oct 16, 2024

View reviewed changes

goodov reviewed Oct 17, 2024

View reviewed changes

AlexeyBarabash mentioned this pull request Oct 17, 2024

org.chromium.chrome.browser.crypto_wallet.Utils.getProfile() should be moved to some common utils brave/brave-browser#41689

Open

goodov reviewed Oct 18, 2024

View reviewed changes

StephenHeaps reviewed Oct 18, 2024

View reviewed changes

ios/brave-ios/Sources/Brave/Frontend/Shields/SubmitReportView.swift Show resolved Hide resolved

ios/brave-ios/Sources/Brave/Frontend/Shields/SubmitReportView.swift Show resolved Hide resolved

StephenHeaps reviewed Oct 18, 2024

View reviewed changes

ios/brave-ios/Sources/Brave/Frontend/Shields/SubmitReportView.swift Outdated Show resolved Hide resolved

ios/browser/api/webcompat_reporter/webcompat_reporter_service_factory_wrapper.mm Outdated Show resolved Hide resolved

StephenHeaps reviewed Oct 18, 2024

View reviewed changes

ios/browser/api/webcompat_reporter/webcompat_reporter_service_factory.mm Outdated Show resolved Hide resolved

StephenHeaps approved these changes Oct 18, 2024

View reviewed changes

goodov approved these changes Oct 21, 2024

View reviewed changes

vadimstruts added 5 commits October 21, 2024 11:44

adding mojo interface

4ded489

Signed-off-by: Vadym Struts <vstruts@brave.com>

adding webcompat service

41fedf8

Signed-off-by: Vadym Struts <vstruts@brave.com>

added using of the same code for sending webcompat report

f732d8b

Signed-off-by: Vadym Struts <vstruts@brave.com>

changed to use just common c++ uploader for desktop + android, starte…

97a493a

…d to add unittests Signed-off-by: Vadym Struts <vstruts@brave.com>

adding tests for webcompat service

5577dbb

Signed-off-by: Vadym Struts <vstruts@brave.com>

vadimstruts and others added 20 commits October 21, 2024 11:44

removed unneded callback

9911fe2

Signed-off-by: Vadym Struts <vstruts@brave.com>

added iOS part

7b9114b

Signed-off-by: Vadym Struts <vstruts@brave.com>

added fix for deps

ef1e0de

Signed-off-by: Vadym Struts <vstruts@brave.com>

fix for dep #2

4f2a20a

Signed-off-by: vadym <vadym@vadyms-iMac-Pro.local>

fix unittest

679bbef

Signed-off-by: Vadym Struts <vstruts@brave.com>

fix format

a772fff

Signed-off-by: Vadym Struts <vstruts@brave.com>

fix ios build

815e81c

Signed-off-by: Vadym Struts <vstruts@brave.com>

fix android build

d84c07d

Signed-off-by: Vadym Struts <vstruts@brave.com>

removed unneeded comments

d99b53f

Signed-off-by: Vadym Struts <vstruts@brave.com>

removed deps

6b7080a

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixes according suggestions

5eccbfa

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixes according suggestions

a5477fd

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixes according suggestions

4fddc5a

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixes according suggestions

9f83c15

Signed-off-by: Vadym Struts <vstruts@brave.com>

removed unneeded comment

b8e318b

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixed for review

7f9c2c3

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixed for review

63c7c2a

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixed iOS build

740ced7

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixed iOS build

32cf883

Signed-off-by: Vadym Struts <vstruts@brave.com>

fixed format

396c47c

Signed-off-by: Vadym Struts <vstruts@brave.com>

vadimstruts force-pushed the 35674-include-component-versions-webcompat-reports branch from 45ae0dd to 396c47c Compare October 21, 2024 09:45

fixed presubmit issue

ef050b2

Signed-off-by: Vadym Struts <vstruts@brave.com>

vadimstruts merged commit e7bec70 into master Oct 21, 2024
17 checks passed

vadimstruts deleted the 35674-include-component-versions-webcompat-reports branch October 21, 2024 15:33

github-actions bot added this to the 1.73.x - Nightly milestone Oct 21, 2024

cdesouza-chromium reviewed Nov 5, 2024

View reviewed changes

bsclifton modified the milestones: 1.73.x - Nightly, 1.74.x - Nightly Nov 6, 2024

	const std::string BoolToString(bool value) {
	std::string BoolToString(bool value) {

Include component versions in webcompat reports #25688

Include component versions in webcompat reports #25688

Conversation

vadimstruts commented Sep 22, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

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

AlexeyBarabash left a comment • 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

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

StephenHeaps left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 21, 2024

Description

Changes

Possible Issues

Security Hotspots

brave-builds commented Oct 21, 2024

Choose a reason for hiding this comment

vadimstruts commented Sep 22, 2024 •

edited

Loading

AlexeyBarabash left a comment •

edited

Loading