Skip to content

evergreen: customize installation of updates#10036

Draft
yuying-y wants to merge 1 commit intoyoutube:mainfrom
yuying-y:u4
Draft

evergreen: customize installation of updates#10036
yuying-y wants to merge 1 commit intoyoutube:mainfrom
yuying-y:u4

Conversation

@yuying-y
Copy link
Copy Markdown
Contributor

@yuying-y yuying-y commented Apr 14, 2026

This change introduces logic to finalize installations using the Starboard
API and ensures proper cleanup of installation artifacts, such as drain
files, if unpacking fails. It also persists the installed Evergreen and
Starboard API versions, enabling better tracking of system state.

Issue: 479816505

@yuying-y yuying-y requested a review from a team as a code owner April 14, 2026 21:58
@yuying-y yuying-y requested a review from andrewsavage1 April 14, 2026 21:58
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Gemini Suggested Commit Message


starboard: Integrate update installation via extension

Integrates the update client with the Starboard Installation Manager
Extension to handle platform-specific aspects of component updates.

This change introduces logic to finalize installations using the Starboard
API and ensures proper cleanup of installation artifacts, such as drain
files, if unpacking fails. It also persists the installed Evergreen and
Starboard API versions, enabling better tracking of system state.

Issue: 10036

💡 Pro Tips for a Better Commit Message:

  1. Influence the Result: Want to change the output? You can write custom prompts or instructions directly in the Pull Request description. The model uses that text to generate the message.
  2. Re-run the Generator: Post a comment with: /generate-commit-message

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces Starboard-specific installation logic into the update_client component, enabling Cobalt slot management and version persistence. The review feedback highlights several critical issues regarding object lifetime management, specifically the need to use scoped_refptr for the ref-counted PersistedData object to avoid potential use-after-free errors in asynchronous tasks. Additionally, the Starboard implementation currently leaks the unpack_path directory, which must be addressed to prevent disk space exhaustion. There are also several style-related improvements needed to align with Chromium standards, such as using base::NumberToString and correcting indentation.

scoped_refptr<CrxInstaller> installer,
CrxInstaller::ProgressCallback progress_callback,
#if BUILDFLAG(IS_STARBOARD)
PersistedData* metadata,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Since PersistedData is a ref-counted object (base::RefCountedThreadSafe), it should be passed as a scoped_refptr<PersistedData> rather than a raw pointer. This ensures that the object remains alive during the asynchronous installation process.

             scoped_refptr<PersistedData> metadata,

Comment on lines +201 to +206
if (metadata != nullptr) {
base::ThreadPool::PostTask(
FROM_HERE, base::BindOnce(
&PersistedData::SetLastInstalledEgAndSbVersion,
base::Unretained(metadata), id, next_version,
std::to_string(SB_API_VERSION)));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

There are several issues in this block:

  1. PersistedData is a ref-counted object. Using base::Unretained(metadata) in a PostTask is unsafe as metadata might be destroyed before the task executes. Passing it as a scoped_refptr ensures the object stays alive.
  2. Chromium style prefers base::NumberToString over std::to_string for numeric conversions.
      if (metadata) {
        base::ThreadPool::PostTask(
            FROM_HERE, base::BindOnce(
              &PersistedData::SetLastInstalledEgAndSbVersion,
              metadata, id, next_version,
              base::NumberToString(SB_API_VERSION)));
      }

install_error = InstallError::GENERIC_ERROR;
}
}
std::move(callback).Run(CrxInstaller::Result(install_error));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The Starboard implementation of Install appears to leak the unpack_path directory. In the standard implementation (lines 215-240), a CallbackChecker is used to ensure that result.unpack_path is deleted after the installation completes. The Starboard path should also ensure this directory is cleaned up to avoid leaking disk space on every update.

scoped_refptr<CrxInstaller> installer,
std::unique_ptr<CrxInstaller::InstallParams> install_params,
#if BUILDFLAG(IS_STARBOARD)
PersistedData* metadata,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

PersistedData is a ref-counted object (base::RefCountedThreadSafe). It should be passed as a scoped_refptr<PersistedData> rather than a raw pointer to ensure its lifetime is correctly managed across asynchronous boundaries.

    scoped_refptr<PersistedData> metadata,

std::string* crx_str,
#endif
#if BUILDFLAG(IS_STARBOARD)
PersistedData* metadata,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

As noted in op_install.h, PersistedData should be passed as a scoped_refptr<PersistedData> to ensure proper lifetime management across the asynchronous pipeline operations.

Suggested change
PersistedData* metadata,
scoped_refptr<PersistedData> metadata,

Comment on lines +161 to +163
const auto installation_api =
static_cast<const CobaltExtensionInstallationManagerApi*>(
SbSystemGetExtension(kCobaltExtensionInstallationManagerName));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The indentation of the wrapped static_cast and its arguments does not follow the Chromium style guide (which uses 4 spaces for wrapped lines). Additionally, per Chromium style, auto* should be used for pointer types when the type is clear from the context.

      const auto* installation_api =
          static_cast<const CobaltExtensionInstallationManagerApi*>(
              SbSystemGetExtension(kCobaltExtensionInstallationManagerName));

Comment on lines +198 to +199
} else if (CobaltFinishInstallation(installation_api, crx_operation_result.installation_index,
result.unpack_path.value(), app_key)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The indentation of the arguments for CobaltFinishInstallation is incorrect. Wrapped arguments should be aligned with the first argument or indented by 4 spaces relative to the start of the function call.

    } else if (CobaltFinishInstallation(
                   installation_api, crx_operation_result.installation_index,
                   result.unpack_path.value(), app_key)) {

@yuying-y yuying-y marked this pull request as draft April 14, 2026 23:07
@yuying-y yuying-y changed the title Install updates evergreen: customize installation of updates Apr 14, 2026
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.

1 participant