New issue
Advanced search Search tips

Issue 780172 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 769970



Sign in to add a comment

support custom operations on UnregisterComponent

Project Member Reported by xiaochu@chromium.org, Oct 31 2017

Issue description

Solution #1:
add a callback to ComponentUpdateService::UnregisterComponent that executes customized operations on component removal.

bool CrxUpdateService::UnregisterComponent(const std::string& id) 
->
bool CrxUpdateService::UnregisterComponent(const std::string& id, base::Callback<void(bool)> callback) 

|callback| is executed right before bool CrxUpdateService::DoUnregisterComponent(const CrxComponent& component) return.

Solution #2:

add ComponentInstallerPolicy::OnCustomUninstall (to be implemented by all *_component_installer)

Call this function right before 'void ComponentInstaller::UninstallOnTaskRunner()' return.
 
Blocking: 769970
Description: Show this description
Summary: support custom operations on UnregisterComponent (was: support callback on UnregisterComponent)
Description: Show this description
Cc: waff...@chromium.org adlr@chromium.org
hi waffles@, can you please review this proposal? I prefer #2 since it's more coherent to current design.
Cc: sorin@chromium.org
I think this comes down to a question of whether full uninstallation behavior is within the scope of the component updater or not. To date, it has not been (the component updater simply allows unregistration of a component, which is invoked as a part of the unregistration flow.

I can see both possibilities:

A • Uninstallation is within the domain of ComponentUpdateService. In this case, we would have:
void ComponentUpdateService::UninstallComponent(const std::string& id, OnceCallback<void(update_client::Error error)> callback);
and probably
bool ComponentInstallerPolicy::OnCustomUninstall();

B • Uninstallation is the province of some other pieces of code, and the component updater is simply responsible for ceasing updates. In this case we just need to make Unregister asynchronous:
void ComponentUpdateService::UnregisterComponent(const std::string& id, OnceCallback<void(update_client::Error error)> callback);

I think B is likely more in-line with the existing component updater - but A is more likely the direction we want to go to build a full-fledged component manager. Sorin, what are your thoughts?
We are expecting to support larger components(>100M, like termina) and uninstallation functionality is critical to save space on device. 

Sorin, do you think it's a good idea to take this chance and enhance ComponentUpdateService to support OnCustomUninstall since it already supports OnCustomInstall?
Sorin and I discussed, we think this is the best way to go.
Thank you very much!
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5b3884a86754f37552b8d8d99dbb24f8c88656a5

commit 5b3884a86754f37552b8d8d99dbb24f8c88656a5
Author: Xiaochu Liu <xiaochu@chromium.org>
Date: Tue Nov 07 19:17:47 2017

add OnCustomUninstall to ComponentInstallerPolicy

Add OnCustomUninstall (similar to OnCustomInstall) that performs
component specific operations after uninstall is complete.

BUG= chromium:780172 
TEST=e2e test on DuT to remove comopnent.

Change-Id: I1437b7bbd55387f52560326619e56f489eaab2d0
Reviewed-on: https://chromium-review.googlesource.com/752001
Commit-Queue: Xiaochu Liu <xiaochu@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514533}
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/crl_set_component_installer.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/cros_component_installer.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/cros_component_installer.h
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/file_type_policies_component_installer.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/file_type_policies_component_installer.h
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/optimization_hints_component_installer.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/optimization_hints_component_installer.h
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/origin_trials_component_installer.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/origin_trials_component_installer.h
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/pnacl_component_installer.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/ssl_error_assistant_component_installer.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/ssl_error_assistant_component_installer.h
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/sth_set_component_installer.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/sth_set_component_installer.h
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/subresource_filter_component_installer.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/subresource_filter_component_installer.h
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/supervised_user_whitelist_installer.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/sw_reporter_installer_win.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/sw_reporter_installer_win.h
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/third_party_module_list_component_installer_win.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/chrome/browser/component_updater/third_party_module_list_component_installer_win.h
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/components/component_updater/component_installer.cc
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/components/component_updater/component_installer.h
[modify] https://crrev.com/5b3884a86754f37552b8d8d99dbb24f8c88656a5/components/component_updater/component_installer_unittest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c6fdf78313b462f20a27279e588820090b024b76

commit c6fdf78313b462f20a27279e588820090b024b76
Author: David Tu <dtu@chromium.org>
Date: Tue Nov 07 20:15:50 2017

Revert "add OnCustomUninstall to ComponentInstallerPolicy"

This reverts commit 5b3884a86754f37552b8d8d99dbb24f8c88656a5.

Reason for revert: Failing on perf Mac Builder: https://build.chromium.org/p/chromium.perf/builders/Mac%20Builder/builds/135552

Original change's description:
> add OnCustomUninstall to ComponentInstallerPolicy
> 
> Add OnCustomUninstall (similar to OnCustomInstall) that performs
> component specific operations after uninstall is complete.
> 
> BUG= chromium:780172 
> TEST=e2e test on DuT to remove comopnent.
> 
> Change-Id: I1437b7bbd55387f52560326619e56f489eaab2d0
> Reviewed-on: https://chromium-review.googlesource.com/752001
> Commit-Queue: Xiaochu Liu <xiaochu@chromium.org>
> Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
> Reviewed-by: Sorin Jianu <sorin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#514533}

TBR=sorin@chromium.org,waffles@chromium.org,xiaochu@chromium.org

Change-Id: I7982f39b102b0fadc1ce42ede9e6bf8dc302b025
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:780172 
Reviewed-on: https://chromium-review.googlesource.com/757510
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514567}
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/crl_set_component_installer.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/cros_component_installer.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/cros_component_installer.h
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/file_type_policies_component_installer.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/file_type_policies_component_installer.h
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/optimization_hints_component_installer.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/optimization_hints_component_installer.h
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/origin_trials_component_installer.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/origin_trials_component_installer.h
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/pnacl_component_installer.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/ssl_error_assistant_component_installer.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/ssl_error_assistant_component_installer.h
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/sth_set_component_installer.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/sth_set_component_installer.h
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/subresource_filter_component_installer.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/subresource_filter_component_installer.h
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/supervised_user_whitelist_installer.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/sw_reporter_installer_win.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/sw_reporter_installer_win.h
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/third_party_module_list_component_installer_win.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/chrome/browser/component_updater/third_party_module_list_component_installer_win.h
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/components/component_updater/component_installer.cc
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/components/component_updater/component_installer.h
[modify] https://crrev.com/c6fdf78313b462f20a27279e588820090b024b76/components/component_updater/component_installer_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Nov 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e306a32b31517697cc6f1da2e406ff73abe3408

commit 1e306a32b31517697cc6f1da2e406ff73abe3408
Author: Xiaochu Liu <xiaochu@chromium.org>
Date: Wed Nov 08 18:57:34 2017

Reland "add OnCustomUninstall to ComponentInstallerPolicy"

This is a reland of 5b3884a86754f37552b8d8d99dbb24f8c88656a5
Original change's description:
> add OnCustomUninstall to ComponentInstallerPolicy
> 
> Add OnCustomUninstall (similar to OnCustomInstall) that performs
> component specific operations after uninstall is complete.
> 
> BUG= chromium:780172 
> TEST=e2e test on DuT to remove comopnent.
> 
> Change-Id: I1437b7bbd55387f52560326619e56f489eaab2d0
> Reviewed-on: https://chromium-review.googlesource.com/752001
> Commit-Queue: Xiaochu Liu <xiaochu@chromium.org>
> Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
> Reviewed-by: Sorin Jianu <sorin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#514533}

Bug:  chromium:780172 
Change-Id: I109cb60f4a037ed882730a0085c463e54c1a0afe
Reviewed-on: https://chromium-review.googlesource.com/756938
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Commit-Queue: Xiaochu Liu <xiaochu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514899}
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/crl_set_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/cros_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/cros_component_installer.h
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/file_type_policies_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/file_type_policies_component_installer.h
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/optimization_hints_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/optimization_hints_component_installer.h
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/origin_trials_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/origin_trials_component_installer.h
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/pepper_flash_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/pnacl_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/recovery_improved_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/recovery_improved_component_installer.h
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/ssl_error_assistant_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/ssl_error_assistant_component_installer.h
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/sth_set_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/sth_set_component_installer.h
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/subresource_filter_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/subresource_filter_component_installer.h
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/supervised_user_whitelist_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/sw_reporter_installer_win.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/sw_reporter_installer_win.h
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/third_party_module_list_component_installer_win.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/third_party_module_list_component_installer_win.h
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/chrome/browser/component_updater/widevine_cdm_component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/components/component_updater/component_installer.cc
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/components/component_updater/component_installer.h
[modify] https://crrev.com/1e306a32b31517697cc6f1da2e406ff73abe3408/components/component_updater/component_installer_unittest.cc

Status: Fixed (was: Untriaged)

Comment 14 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 15 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment