Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Chromium 89.0.4389.82 #1416

Merged
merged 5 commits into from
Mar 10, 2021
Merged

Conversation

tangalbert919
Copy link
Contributor

Resolves #1415. Currently working on getting a successful build on Arch Linux.

@tangalbert919 tangalbert919 marked this pull request as ready for review March 3, 2021 14:02
@tangalbert919
Copy link
Contributor Author

I was able to get a successful build on Arch Linux.

Some notes regarding changes in these patches:

  • Three Iridium Browser patches were pulled from upstream to update the ones we have.
  • DuckDuckGo is already included as a search engine for most countries. Moving them to first in the list seems unnecessary.
  • The last fingerprinting patch from Bromite has changes from this commit in a PR that has yet to be merged.

@perfect7gentleman
Copy link

On Gentoo with "chrome_pgo_phase=1" I got

ld.lld: error: undefined symbol: safe_browsing::RealTimeUrlLookupServiceFactory::GetForProfile(Profile*)
>>> referenced by chrome_content_browser_client.cc
>>>               thinlto-cache/Thin-d78b63.tmp.o:(ChromeContentBrowserClient::GetUrlLookupService(content::BrowserContext*, bool, bool))
clang-11: error: linker command failed with exit code 1 (use -v to see invocation)

@git-bruh
Copy link

git-bruh commented Mar 4, 2021

Get the same error on a musl build (pure GNU/GCC toolchain):

/usr/bin/ld: obj/chrome/browser/browser/chrome_content_browser_client.o: in function `ChromeContentBrowserClient::GetUrlLookupService(content::BrowserContext*, bool, bool)':
chrome_content_browser_client.cc:(.text+0x314e): undefined reference to `safe_browsing::RealTimeUrlLookupServiceFactory::GetForProfile(Profile*)'
collect2: error: ld returned 1 exit status

Moving the #endif like so "fixes" this:

---   chrome/browser/chrome_content_browser_client.cc
+++ chrome/browser/chrome_content_browser_client.cc
@@ -5333,12 +5333,13 @@
     return safe_browsing::ChromeEnterpriseRealTimeUrlLookupServiceFactory::
         GetForProfile(profile);
   }
-#endif

   if (is_consumer_lookup_enabled) {
     return safe_browsing::RealTimeUrlLookupServiceFactory::GetForProfile(
         profile);
   }
+#endif
+
   return nullptr;
 }

@git-bruh
Copy link

git-bruh commented Mar 4, 2021

I also get this error along with a few more "$foo is not a member of 'base'" erors in the same file:

../../chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc: In member function 'virtual void ChromeBrowserMainExtraPartsMetrics::PostBrowserStart()':
../../chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:620:18: error: 'IsUsingOzonePlatform' is not a member of 'features'
  620 |   if (!features::IsUsingOzonePlatform()) {
      |                  ^~~~~~~~~~~~~~~~~~~~

I'm building with use_ozone=true, but i dont think this particular error is related to ungoogled patches.

@perfect7gentleman
Copy link

perfect7gentleman commented Mar 4, 2021

The patchfix helped with linking. But some pages and extensions are getting crashed.
I build with Ozone too, 88 was okay.

Error code: RESULT_CODE_KILLED_BAD_MESSAGE

@Ahrotahn
Copy link
Contributor

Ahrotahn commented Mar 4, 2021

Builds fine with chrome_pgo_phase=0, but visiting Github crashes the tab for me:
Screenshot_20210304_063923

ERROR:render_process_host_impl.cc(4959)] Terminating render process for bad Mojo message: Received bad user message: No binder found for interface blink.mojom.ReportingServiceProxy for the frame/document scope

enable_reporting=false is in the build flags, so ReportingServiceProxy might be slipping through somewhere.

I'll dig further into that as I get some time later today.

@git-bruh
Copy link

git-bruh commented Mar 4, 2021

I get the same error when visiting github too:

[2532:2532:0304/130006.200813:ERROR:render_process_host_impl.cc(4959)] Terminating render process for bad Mojo message: Received bad user message: No binder found for interface blink.mojom.ReportingServiceProxy for the frame/document scope
[2532:2532:0304/130006.200838:ERROR:bad_message.cc(28)] Terminating renderer for bad IPC message, reason 123

My flags can be found here.

EDIT: enable_reporting=true seems to fix it.

@kramred
Copy link
Contributor

kramred commented Mar 4, 2021

I've tried to adjust the macOS patches and the build finished but I'm also getting a similar error message:

[ERROR:render_process_host_impl.cc(4959)] Terminating render process for bad Mojo message: Received bad user message: No binder found for interface blink.mojom.ReportingServiceProxy for the frame/document scope
[ERROR:bad_message.cc(28)] Terminating renderer for bad IPC message, reason 123
[ERROR:exception_ports.cc(122)] host_get_exception_ports: (os/kern) invalid argument (4)

A list of URLs that crash the tab for me:

@tangalbert919
Copy link
Contributor Author

I will correct these errors as quickly as I possibly can.

@perfect7gentleman
Copy link

confirm "enable_reporting=true" fixed the issue with crashing tabs and extensions

@Ahrotahn
Copy link
Contributor

Ahrotahn commented Mar 5, 2021

Previously defines were used for the ReportingServiceProxy header and the code that used it, but that's not the case now.

Adding this to fix-building-without-enabling-reporting.patch solves the issue for me while keeping reporting disabled:

diff
--- a/third_party/blink/renderer/core/frame/local_frame.h
+++ b/third_party/blink/renderer/core/frame/local_frame.h
@@ -38,6 +38,7 @@
 #include "mojo/public/cpp/bindings/pending_associated_receiver.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/unique_receiver_set.h"
+#include "net/net_buildflags.h"
 #include "third_party/blink/public/common/frame/transient_allow_fullscreen.h"
 #include "third_party/blink/public/mojom/blob/blob_url_store.mojom-blink.h"
 #include "third_party/blink/public/mojom/frame/back_forward_cache_controller.mojom-blink.h"
@@ -522,7 +523,9 @@
 
   SmoothScrollSequencer& GetSmoothScrollSequencer();
 
+#if BUILDFLAG(ENABLE_REPORTING)
   mojom::blink::ReportingServiceProxy* GetReportingService();
+#endif
 
   // Returns the frame host ptr. The interface returned is backed by an
   // associated interface with the legacy Chrome IPC channel.
@@ -903,9 +906,11 @@
   // const methods.
   //
   // LocalFrame can be reused by multiple ExecutionContext.
+#if BUILDFLAG(ENABLE_REPORTING)
   mutable HeapMojoRemote<mojom::blink::ReportingServiceProxy,
                          HeapMojoWrapperMode::kWithoutContextObserver>
       reporting_service_{nullptr};
+#endif
 
 #if defined(OS_MAC)
   // LocalFrame can be reused by multiple ExecutionContext.
--- a/third_party/blink/renderer/core/frame/reporting_context.h
+++ b/third_party/blink/renderer/core/frame/reporting_context.h
@@ -5,6 +5,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_FRAME_REPORTING_CONTEXT_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_FRAME_REPORTING_CONTEXT_H_
 
+#include "net/net_buildflags.h"
 #include "third_party/blink/public/mojom/frame/reporting_observer.mojom-blink.h"
 #include "third_party/blink/public/mojom/reporting/reporting.mojom-blink.h"
 #include "third_party/blink/renderer/core/core_export.h"
@@ -54,8 +55,10 @@
   // Counts the use of a report type via UseCounter.
   void CountReport(Report*);
 
+#if BUILDFLAG(ENABLE_REPORTING)
   const HeapMojoRemote<mojom::blink::ReportingServiceProxy>&
   GetReportingService() const;
+#endif
 
   void NotifyInternal(Report* report);
   // Send |report| via the Reporting API to |endpoint|.
@@ -67,8 +70,10 @@
 
   // This is declared mutable so that the service endpoint can be cached by
   // const methods.
+#if BUILDFLAG(ENABLE_REPORTING)
   mutable HeapMojoRemote<mojom::blink::ReportingServiceProxy>
       reporting_service_;
+#endif
 
   HeapMojoReceiver<mojom::blink::ReportingObserver, ReportingContext> receiver_;
 };
--- a/third_party/blink/renderer/core/frame/local_frame.cc
+++ b/third_party/blink/renderer/core/frame/local_frame.cc
@@ -38,6 +38,7 @@
 #include "base/unguessable_token.h"
 #include "mojo/public/cpp/bindings/self_owned_receiver.h"
 #include "mojo/public/cpp/system/message_pipe.h"
+#include "net/net_buildflags.h"
 #include "services/data_decoder/public/mojom/resource_snapshot_for_web_bundle.mojom-blink.h"
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/mojom/content_security_policy.mojom-blink.h"
@@ -551,7 +552,9 @@
   visitor->Trace(raw_system_clipboard_);
   visitor->Trace(virtual_keyboard_overlay_changed_observers_);
   visitor->Trace(pause_handle_receivers_);
+#if BUILDFLAG(ENABLE_REPORTING)
   visitor->Trace(reporting_service_);
+#endif
 #if defined(OS_MAC)
   visitor->Trace(text_input_host_);
 #endif
@@ -2273,6 +2276,7 @@
   return base::UnguessableToken::Null();
 }
 
+#if BUILDFLAG(ENABLE_REPORTING)
 mojom::blink::ReportingServiceProxy* LocalFrame::GetReportingService() {
   if (!reporting_service_.is_bound()) {
     GetBrowserInterfaceBroker().GetInterface(
@@ -2281,6 +2285,7 @@
   }
   return reporting_service_.get();
 }
+#endif
 
 // static
 void LocalFrame::NotifyUserActivation(
--- a/third_party/blink/renderer/core/frame/reporting_context.cc
+++ b/third_party/blink/renderer/core/frame/reporting_context.cc
@@ -4,6 +4,7 @@
 
 #include "third_party/blink/renderer/core/frame/reporting_context.h"
 
+#include "net/net_buildflags.h"
 #include "third_party/blink/public/common/browser_interface_broker_proxy.h"
 #include "third_party/blink/public/platform/platform.h"
 #include "third_party/blink/public/platform/task_type.h"
@@ -54,7 +55,9 @@
 ReportingContext::ReportingContext(ExecutionContext& context)
     : Supplement<ExecutionContext>(context),
       execution_context_(context),
+#if BUILDFLAG(ENABLE_REPORTING)
       reporting_service_(&context),
+#endif
       receiver_(this, &context) {}
 
 // static
@@ -118,7 +121,9 @@
   visitor->Trace(observers_);
   visitor->Trace(report_buffer_);
   visitor->Trace(execution_context_);
+#if BUILDFLAG(ENABLE_REPORTING)
   visitor->Trace(reporting_service_);
+#endif
   visitor->Trace(receiver_);
   Supplement<ExecutionContext>::Trace(visitor);
 }
@@ -140,6 +145,7 @@
   UseCounter::Count(execution_context_, feature);
 }
 
+#if BUILDFLAG(ENABLE_REPORTING)
 const HeapMojoRemote<mojom::blink::ReportingServiceProxy>&
 ReportingContext::GetReportingService() const {
   if (!reporting_service_.is_bound()) {
@@ -149,6 +155,7 @@
   }
   return reporting_service_;
 }
+#endif
 
 void ReportingContext::NotifyInternal(Report* report) {
   // Buffer the report.
@@ -171,6 +178,7 @@
 
 void ReportingContext::SendToReportingAPI(Report* report,
                                           const String& endpoint) const {
+#if BUILDFLAG(ENABLE_REPORTING)
   const String& type = report->type();
   if (!(type == ReportType::kCSPViolation || type == ReportType::kDeprecation ||
         type == ReportType::kFeaturePolicyViolation ||
@@ -227,6 +235,7 @@
         "Document policy violation", body->sourceFile(), line_number,
         column_number);
   }
+#endif
 }
 
 }  // namespace blink

@perfect7gentleman
Copy link

Confirm successful building with enable_reporting=false & chrome_pgo_phase=1

Copy link
Member

@Eloston Eloston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but will wait a little bit longer for more feedback.

@Eloston Eloston modified the milestones: 88.x.x.x, 89.x.x.x Mar 7, 2021
@perfect7gentleman
Copy link

The Stable channel has been updated to 89.0.4389.82

@PF4Public
Copy link
Contributor

PF4Public commented Mar 7, 2021

At least pruning lists need to be updated for .82:

 * Pruning binaries ...
ERROR: 1 files could not be pruned.

@kramred
Copy link
Contributor

kramred commented Mar 7, 2021

I've updated pruning.list for .82 here and could build .82 for macOS_x86-64 successfully
The updated file can be included in this PR or I can open a new PR when this one is merged. No other adjustments to patches were necessary, if I remember correctly.

@wchen342
Copy link
Contributor

wchen342 commented Mar 8, 2021

Builds successfully on Fedora and CentOS.

@tangalbert919
Copy link
Contributor Author

I was able to get a successful build on Windows. Hopefully, this PR will be merged before Google pushes another update to Google Chrome.

@wchen342
Copy link
Contributor

wchen342 commented Mar 9, 2021

The domain substitution regex caused a problem in android build. In the pattern of android.*com, can you add a negative lookahead to exclude the pattern "org.chromium.android.*com"?

@PF4Public
Copy link
Contributor

PF4Public commented Mar 9, 2021

Did anyone notice, that 89 Chromium takes up twice the space of 88? Or is it just me?

@wchen342
Copy link
Contributor

wchen342 commented Mar 9, 2021

It's 109M here. Do you link to system libs on Gentoo?

@PF4Public
Copy link
Contributor

Do you link to system libs on Gentoo?

Yes, with the exception of libvpx. Chromium vaapi code isn't friendly to system libvpx.

@wchen342
Copy link
Contributor

wchen342 commented Mar 9, 2021

That's strange. Maybe you can try to find out which files are taking the most spaces?

@PF4Public
Copy link
Contributor

which files are taking the most spaces

I did compare the chrome binary itself on core2 and haswell (before updating core2 to 89):

haswell 89:
-rwxr-xr-x   1 root root 659480424 Mar  8 15:40 chrome
core2 88:
-rwxr-xr-x   1 root root 165681472 Feb 18 07:30 chrome

Even though architectures are different, sizes differ significantly.

It's 109M here.

I assume you do not enable PGO at all as profdata are supposed to be pruned by scripts, do you?
Unlike my usual building with full PGO, I did pgo_phase=1 this time instead of pgo_phase=2. This might've changed something… I should probably investigate, what "PGI (instrumentation) phase" and "PGO (optimization) phase" entail. Should someone happen to know, please kindly advise me on the difference :)

@wchen342
Copy link
Contributor

wchen342 commented Mar 9, 2021

The difference is almost 3 times bigger. That doesn't looks trivial..

I assume you do not enable PGO at all as profdata are supposed to be pruned by scripts

Yes, pgo_phase is 0 here. PGO may change something but I never tried that so unfortunately I cannot tell.

@csagan5
Copy link
Contributor

csagan5 commented Mar 9, 2021

  • The last fingerprinting patch from Bromite has changes from this commit in a PR that has yet to be merged.

That is an experiment, I advise to not use any patch from that branch because some of them are known to be broken. The patches on master are instead safe.

@tangalbert919 tangalbert919 changed the title Update to Chromium 89.0.4389.72 Update to Chromium 89.0.4389.82 Mar 10, 2021
Copy link
Member

@Eloston Eloston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks 👍

@Eloston Eloston merged commit 6f98d56 into ungoogled-software:master Mar 10, 2021
@csagan5
Copy link
Contributor

csagan5 commented May 16, 2021

@Eloston in this PR the patch patches/core/ungoogled-chromium/use-local-devtools-files.patch is rendered non-working, it could as well be deleted now as it is doing nothing.

@PF4Public
Copy link
Contributor

as it is doing nothing

@csagan5 Do you imply that devtools are local or they never were otherwise?

@csagan5
Copy link
Contributor

csagan5 commented May 16, 2021

@PF4Public I do not know, but the changes of this PR make the patch a no-op.

@Ahrotahn
Copy link
Contributor

I had thought the same thing and attempted to remove it in one of the following updates. Doing so causes the build to fail with:

ninja: error: '../../src/chromium/third_party/devtools-frontend/src/test/e2e/resources/media/fisch.webm', needed by 'gen/third_party/devtools-frontend/src/test/e2e/resources/media/fisch.webm', missing and no known rule to make it

We could remove the patch but would have to whitelist the binaries under third_party/devtools-frontend/src/test/e2e/resources/ in the pruning list.

Granted use-local-devtools-files.patch is no longer doing what it says on the tin, so the patch itself could still be removed but the change within would need to moved to another patch if we don't want to whitelist the files.

csagan5 pushed a commit to bromite/bromite that referenced this pull request Sep 26, 2021
Added `ungoogled-chromium-Fix-building-without-enabling-reporting.patch`
to fix build issues. See the following PR for more details:
ungoogled-software/ungoogled-chromium#1416
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updating to Chromium 89.0.4389.72
10 participants