Fix failing component auto-updates

[mherrmann]

Resolves brave/brave-browser#10464
Also resolves brave/brave-browser#10280

Submitter Checklist:

• There is a ticket for my issue.
• Used Github auto-closing keywords in the commit message.
• Wrote a good PR/commit description
• 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).
• Requested a security/privacy review as needed.

Reviewer Checklist:

• 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:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
  • https://github.com/brave/brave-browser/wiki/QA-Guide
  • https://github.com/brave/brave-browser/wiki/P3A

Test Plan:

[mherrmann]

@jumde the implementation is complete from my side. Can you review, or can you suggest someone else to review? (It's only my second PR to brave-core.)

[mherrmann]

@jumde just so it's visible here too, instead of hidden in the review conversation: the PR is ready to merge from my side (excl. squash, rebase and potentially removing log statements). I'd kindly ask that you review please and let me know what you think.

[mherrmann]

@mkarolin @bridiver can I also request your review on this? (@jumde suggested I ask you.) The short summary is: When you open brave://components and wait for 1 minute, then you see a bunch of update errors on the page. The reason for this is explained in the comment in my committed file update_checker.h. This is only my second PR to brave-core, so I would love to hear your thoughts.

(edit: I accidentally tagged @pes10k as reviewer at first)

[jumde]

@mherrmann - This fix also installs components (during auto-update) that shouldn't be shipped with Brave like Federated Learning Of Cohorts, Legacy TLS Deprecation Configuration, Subresource FIlter Rules. Only the components that are installed at the first run, should be updated with auto-update.

[mherrmann]

@jumde do you have an idea why those components are being installed?

[jumde]

@jumde do you have an idea why those components are being installed?

@mherrmann - I need to dig into the chromium code to see why this might be happening.

[mherrmann]

Okay, thank you. As a workaround, could we make go-update return "no update" responses for those components we do not wish to be installed?

[jumde]

Okay, thank you. As a workaround, could we make go-update return "no update" responses for those components we do not wish to be installed?

I think it would be easy to fix this on brave-core. If we do it on go-update, this will require updating go-update every time a component is added to the chromium code and it adds a testing burden as well.

[mherrmann]

I think it would be easy to fix this on brave-core. If we do it on go-update, this will require updating go-update every time a component is added to the chromium code and it adds a testing burden as well.

Ah, I see. That makes sense. Thank you. I'm very curious what you'll find about those additionally installed components.

[mherrmann]

@jumde I develop on Linux. I just started Brave Nightly v1.19.36 on a Windows VM I haven't touched in a month. I have the components you mentioned also on this Windows VM:

You wrote that my PR here installs these unwanted components. Do I presume correctly that you actually meant that my PR updates these components, possibly from a non-existent skeleton implementation?

I'll dig a little further how these components get there in the first place.

[mherrmann]

After some further digging, I have a better understanding how components are installed. Chromium installs its components in chrome/browser/component_updater/registration.cc. This is called from chrome_browser_main.cc in ChromeBrowserMainParts::PreMainMessageLoopRunImpl. Brave has a subclass BraveBrowserMainParts of ChromeBrowserMainParts that however does not override PreMainMessageLoopRunImpl.

What I don't understand is that apparently there is a wish to exclude certain Google components from being shipped with Brave. But besides some extra Brave components, my components in Chrome and Brave are basically the same.

The closest thing I could find to an implementation is IsBlacklisted in brave-core, which gets invoked from UserMayLoad. However, the associated classes refer to extensions, not components. I'm not sure yet whether that logic is then also used for components. Also potentially related: ExtensionInstallBlacklist on chrome://policy. If that blacklist applies to components as well as extensions, then we might be able to use it.

[mherrmann]

@jumde kindly pointed me to https://github.com/brave/brave-browser/wiki/Brave-Components, which lists the components officially supported by Brave, and those officially unsupported.

What surprises me is that my local Brave installation has many of the components that are supposed to be unsupported. For example, "Certificate Error Assistant" and "Crowd Deny".

@jumde also pointed out that components can be excluded from Brave by overriding their Register... methods. For example, RegisterSafetyTipsComponent. This fits with my analysis of registration.cc above. Brave does override some such registration functions in brave-core/chromium_src/chrome/browser/component_updater. However, as far as I can see, it does not actually stop any components from being installed.

Coming back to this PR: It doesn't seem to me like it installs any components that shouldn't be there. But yes, it does update all components, including the unwanted ones.

If updating unwanted components is a problem, then this PR shouldn't be merged yet and we have to prevent the components from being there in the first place. Otherwise, I would tackle removing these unwanted components in the separate existing issue brave/brave-browser#8709.

@jumde do you see an issue with merging this PR and tackling brave/brave-browser#8709 separately?

[mherrmann]

After reading brave/brave-browser#10607 (comment), I think I can answer my question myself. It probably would be a problem to merge this PR. The reason is that while Brave does not prevent Chromium from registering unwanted components, it does not install them. My suspicion is that Chromium installs components by registering them with version 0.0.0.0, then performing an automatic update check. In Brave, this automatic update check was previously broken (hence the present PR). Hence the components were not installed.

So yes, I do think that we should tackle brave/brave-browser#8709 before merging this PR.

[mherrmann]

Tested the latest CI build for this PR on Linux (brave-browser-nightly_1.20.47_amd64.deb). Works as expected for me. After 60 seconds on brave://components, I see:

Note how there are no update errors.

The only question mark is the new "Hyphenation" component. It seems this was introduced in a recent Chromium version. If Brave does not want to include it, then we should block it as in #7331.

[mihaiplesa]

Tested on macOS and works as expected. CI passed on all.

[simonhong]

Kindly ping to @brave/patch-reviewers :)

[mihaiplesa]

@bridiver said this looks fine, just waiting for @jumde's latest thoughts.

[jumde]

lgtm

[mherrmann]

I rebased onto master and force-pushed to get a CI build one last time before merging.

[mherrmann]

The post-init check fails because of what I believe is a bug in lint.py

/home/ubuntu/workspace/pr-brave-browser-mherrmann-fix-google-components-autoupdate-post-init/src
10:07:48  > ninja -C /home/ubuntu/workspace/pr-brave-browser-mherrmann-fix-google-components-autoupdate-post-init/src/out/Component_audit brave:audit_deps
10:07:48  ninja: Entering directory `/home/ubuntu/workspace/pr-brave-browser-mherrmann-fix-google-components-autoupdate-post-init/src/out/Component_audit'
10:07:50  Error running format check:
10:07:50  Traceback (most recent call last):
10:07:50    File "/home/ubuntu/workspace/pr-brave-browser-mherrmann-fix-google-components-autoupdate-post-init/src/brave/build/commands/scripts/lint.py", line 35, in RunFormatCheck
10:07:50      if HasFormatErrors():
10:07:50    File "/home/ubuntu/workspace/pr-brave-browser-mherrmann-fix-google-components-autoupdate-post-init/src/brave/build/commands/scripts/lint.py", line 26, in HasFormatErrors
10:07:50      print(diff)
10:07:50  UnicodeEncodeError: 'ascii' codec can't encode characters in position 265353-265360: ordinal not in range(128)

The person who recently committed the print(diff) line has been alerted. @mihaiplesa is also looking into this.

[mherrmann]

The post-init check succeeded now. I also tested the CI build on Linux and am getting the expected result. On brave://components, after 60 seconds, an automatic update check is performed and no more errors appear:

Merging this.

[mherrmann]

@bridiver or @mkarolin can I get your approval on this PR please? I can't merge without another (4th) approving review.

[mihaiplesa]

Merging as the last change was minor.