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

Upgrade from Chromium 99 to Chromium 100 #12483

Merged
merged 125 commits into from
Mar 19, 2022
Merged

Upgrade from Chromium 99 to Chromium 100 #12483

merged 125 commits into from
Mar 19, 2022

Conversation

emerick
Copy link
Contributor

@emerick emerick commented Mar 4, 2022

Resolves brave/brave-browser#21470

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, npm run lint, 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:

@github-actions github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false labels Mar 4, 2022
@emerick emerick marked this pull request as ready for review March 5, 2022 18:47
@emerick emerick requested review from a team, samartnik, goodov, iefremov and simonhong as code owners March 5, 2022 18:47
@emerick emerick requested review from bsclifton and deeppandya March 5, 2022 18:47
@emerick emerick marked this pull request as draft March 5, 2022 18:48
@emerick emerick requested a review from mkarolin March 8, 2022 14:20
@emerick emerick marked this pull request as ready for review March 8, 2022 15:06
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

iOS++

@kylehickinson kylehickinson requested review from a team March 8, 2022 16:11
@@ -21,13 +21,12 @@ namespace {

WebUIConfigList CreateConfigs() {
WebUIConfigList config_list;
auto register_config =
[[maybe_unused]] auto register_config =
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure I understand this one, register_config does get used, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the original use of ALLOW_UNUSED_LOCAL was not needed to begin with, fixed in 9fd1d75

@@ -13,6 +13,7 @@ OVERRIDE_FEATURE_DEFAULT_STATES({{
#if !defined(OS_ANDROID)
{kCopyLinkToText, base::FEATURE_DISABLED_BY_DEFAULT},
#endif
{kDestroyProfileOnBrowserClose, base::FEATURE_DISABLED_BY_DEFAULT},
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to have different behavior than Chrome here? @diracdeltas @pes10k ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is effectively private browsing mode, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

From https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/flag_descriptions.cc;l=1692:

    "Release memory and other resources when a Profile's last browser window "
    "is closed, rather than when Chrome closes completely.";

Also from https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/browser_features.cc;l=28:

// Destroy profiles when their last browser window is closed, instead of when
// the browser exits.

So it doesn't seem related to private browsing per-se, but more like not waiting until the browser binary exits to destroy the profiles used by its windows, as that could happen way after the last window is closed (e.g. on Windows, if you have Chrome running in the background, it will be running even if no windows are open, see the icon in the taskbar).

Cc @emerick in any case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this feature destroys the profile as soon as the browser closes vs. waiting until the browser exits. With this feature enabled, ClearOnExit()'s call to GetLoadedProfiles() returns no profiles because they've already been destroyed earlier, so ClearOnExit ends up deleting nothing.

It seems like we might want to tie clearing the data to the profile lifetime, but I wasn't too sure what the scope of that change might be. I can enter an issue for that work. It doesn't seem like it needs to hold up this PR, but let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@emerick what doesn't work when this is enabled? If the call to GetLoadedProfiles returns nothing, doesn't that mean we've already destroyed them? cc @yrliou

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bridiver Yeah, GetLoadedProfiles returns no profiles. It does mean that we've destroyed the profiles, but my understanding is that "clear data on exit" is currently tied to browser exit and not profile destruction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, just to clarify, the feature name is referring to destruction of the profile in the C++ sense via a destructor (not deleting the profile from disk).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@yrliou @darkdh shouldn't we be setting kForceEphemeralProfiles on tor/session profiles so they automatically get deleted at the correct time? Or call ScheduleProfileForDeletion when the profile is created?

// Schedules the profile at the given path to be deleted on shutdown. If we're
  // deleting the last profile, a new one will be created in its place, and in
  // that case the callback will be called when profile creation is complete.
  void ScheduleProfileForDeletion(const base::FilePath& profile_dir,

samartnik and others added 16 commits March 18, 2022 20:04
Chromium change:
chromium/chromium@a5e2b32

[Glowup] Update Chromium app icons and add adaptive icons
Bug: 1286045
Chromium change:
chromium/chromium@a5e2b32

[Glowup] Update Chromium app icons and add adaptive icons
Bug: 1286045# Please enter the commit message for your changes. Lines starting
Chromium change:

https://source.chromium.org/chromium/chromium/src/+/7af9b25ea15c99177eeb7e1286c2bef002c70440

commit 7af9b25ea15c99177eeb7e1286c2bef002c70440
Author: sauski <[email protected]>
Date:   Tue Feb 1 09:59:45 2022 +0000

    Privacy Sandbox Settings: Introduce new un-synced primary preference

    This CL introduces V2 of the PrivacySandboxApisEnabled preference, which
    will replace the initial version as the primary control for Privacy
    Sandbox APIs.

    A migration is required as post Kartoffel release 3, the primary
    control will not be synced across devices. The new pref will be init
    appropriately during the confirmation moment of Kartoffel 3, and so
    is default off.

    All locations that consult or set the V1 preference have been updated
    to consult the new pref as required. The exception is the FLoC
    generated preference, which as FLoC has been discontinued will be
    removed and does not need to be updated.

    The PrivacySandboxSettings unit tests have been parameterized based
    on the value of the feature, with test utils updated to only set the
    appropriate pref.

    Bug: 1286276
…ng ScopedTempDir

Maintain a list of the temporary extension directories and post a task to delete
them in our destructor. This will make their lifetime and ownership more
obvious.
This is a second pass after commit fe9b3d9 to finally migrate away
from this deprecated methods, which required changing the signature
of some methods to avoid replacing use of base::Value::[Const]ListView
with the new types instead (e.g. base::Value::List).

[1] fe9b3d9e36

Chromium change:

https://source.chromium.org/chromium/chromium/src/+/60e6b2d9538d743e7aaac58f46e996e6031b8a86

commit 60e6b2d9538d743e7aaac58f46e996e6031b8a86
Author: Daniel Cheng <[email protected]>
Date:   Sat Feb 5 01:08:46 2022 +0000

    base::Value: Remove GetList(), TakeList(), and TakeDict().

    Also remove (a lot of) new uses of GetList() that crept in. There is
    also one use of GetList() in a third-party code generator that was
    missed in previous iterations. Since the file is checked directly into
    Chrome, the usage is removed in this CL, but upstream will need to be
    updated before the next roll.

    Bug: 1291666, 1294416
We want bottom tab groups toolbar and bottom toolbar colors to match.

Chromium change:
chromium/chromium@36b698c

Reland "[GMNext] Migrate default_bg_color_dark_elev_N"
This is a reland of 391590f

The underlying issue has been fixed in https://crrev.com/c/3414069 and
this change is now safe to reland without any modifications.

Original change's description:
> [GMNext] Migrate default_bg_color_dark_elev_N
>
>
> [GMNext] Migrate default_bg_color_light_elev_N to dynamic colors
>
> Migrate default_bg_color_light_elev_N to dynamic colors.
>
> Only usage can be handled with the SurfaceColorDrawable.
>
> Will handle night mode version in followup CL migrating
> default_bg_color_dark_elev_N.
>
> Bug: 1270911
> Change-Id: Id2931d815de83f3d0a57bbd104a65d2cd85c8f8b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3343448
> Reviewed-by: Sky Malice <[email protected]>
> Reviewed-by: Sinan Sahin <[email protected]>
> Reviewed-by: Theresa Sullivan <[email protected]>
> Reviewed-by: Colin Blundell <[email protected]>
> Commit-Queue: Neil Coronado <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#957734}

Bug: 1270911, 1287000
Related Chromium change:

https://source.chromium.org/chromium/chromium/src/+/9321000e5ad8a564f5f8b9907a198093c1319a02

remove py2 compatiblity from content_shell_crash_test

The test is now running in python3.

Bug: 1288172
Change-Id: I36da2b6269681d590c45de95bda3ed917cb9cea1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3428841
Auto-Submit: Takuto Ikuta <[email protected]>
Reviewed-by: Lei Zhang <[email protected]>
Commit-Queue: Lei Zhang <[email protected]>
Cr-Commit-Position: refs/heads/main@{#965520}
@emerick
Copy link
Contributor Author

emerick commented Mar 19, 2022

CI passed on all platforms except Windows; going to rebase and rerun for Windows.

@emerick emerick force-pushed the cr100 branch 4 times, most recently from 733a14f to a18bac3 Compare March 19, 2022 19:51
@emerick emerick removed CI/skip-android Do not run CI builds for Android 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 Mar 19, 2022
@emerick
Copy link
Contributor Author

emerick commented Mar 19, 2022

CI passed on Windows; ready for merge.

@emerick emerick added this to the 1.38.x - Nightly milestone Mar 19, 2022
@emerick emerick merged commit e80c1ab into master Mar 19, 2022
@emerick emerick deleted the cr100 branch March 19, 2022 21:45
emerick added a commit that referenced this pull request Mar 21, 2022
Upgrade from Chromium 99 to Chromium 100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/run-network-audit Run network-audit potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade from Chromium 99 to Chromium 100