New issue
Advanced search Search tips

Issue 869663 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 17
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[Update client] Race condition when component states are mutated

Project Member Reported by mxnguyen@chromium.org, Jul 31

Issue description

This rare race condition can happen with the unified extension updater (update service) in which user/sync uninstalls an extension when an extension update check is in progress.
If the uninstall of an extension occurs before the update_client work flow starts, the update client won't be able to obtain any metadata about the extension (because it's been removed); eventually, the update client would emit a COMPONENT_UPDATE_ERROR event to the update service. Upon receiving the update error event, the update service will try to get more detail error from the update client through GetCrxUpdateState, which would fail because the update client doesn't have any metadata about the extension.

Update client should have tests to verify that it still works correctly when some components states are mutated by an external source during an update check session.

 
When this happens, should GetCrxUpdateState return something more meaningful in the return value? Like why it fails to obtain the update state?
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 8

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

commit 2e8384697adee0656a913038d97400acf2f2ddfb
Author: Minh X. Nguyen <mxnguyen@chromium.org>
Date: Wed Aug 08 22:14:42 2018

[Extensions] Fix a crash due to race condition in the update service.

Because of a change in the way update client handles extensions that
were remove during the update process UpdateService can crash when
handling update events emitted from UpdateClient.

Bug:  868906 ,  869663 
Change-Id: I78cf5075fab84075f8472edcb2428348794307fe
Reviewed-on: https://chromium-review.googlesource.com/1155749
Commit-Queue: Minh Nguyen <mxnguyen@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581706}
[modify] https://crrev.com/2e8384697adee0656a913038d97400acf2f2ddfb/extensions/browser/updater/update_service.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 17

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

commit 7390024923140a7671b1841b3565c927504b83f3
Author: Sorin Jianu <sorin@chromium.org>
Date: Fri Aug 17 01:11:53 2018

Make CrxComponent an optional member in the CrxUpdateItem.

Since the instance of CrxComponent is provided by the clients of the
update_client::UpdateClient, it is possible that instance is missing in
some cases, such as when a CRX is uninstalled. That leads to corner
cases such as calls to UpdateClient::GetCrxUpdateState returning an
error, because an instance of CrxUpdateItem could not be returned.

This is a mechanical change to replace the std::unique_ptr wrapper of
the CrxComponent with a base::Optional wrapper, so that CrxUpdateItem
could have correct value semantics.

BUG= 869663 

Change-Id: I6aa805c2784af5ed959c8777f724322763404cb0
Reviewed-on: https://chromium-review.googlesource.com/1173018
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Minh Nguyen <mxnguyen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583913}
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/chrome/browser/ui/webui/components_ui.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/component_updater/component_installer_unittest.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/component_updater/component_updater_service.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/component_updater/component_updater_service_internal.h
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/update_client/action_runner.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/update_client/component.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/update_client/component.h
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/update_client/crx_update_item.h
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/update_client/ping_manager_unittest.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/update_client/protocol_builder.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/update_client/update_checker_unittest.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/update_client/update_client.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/update_client/update_client.h
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/update_client/update_client_unittest.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/components/update_client/update_engine.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/extensions/browser/updater/update_data_provider.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/extensions/browser/updater/update_data_provider.h
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/extensions/browser/updater/update_data_provider_unittest.cc
[modify] https://crrev.com/7390024923140a7671b1841b3565c927504b83f3/extensions/browser/updater/update_service_unittest.cc

Status: Fixed (was: Unconfirmed)
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 21

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

commit 6e0e7cd9bf10a61f443ba63115e86e9204d061ae
Author: Minh X. Nguyen <mxnguyen@chromium.org>
Date: Tue Aug 21 00:17:28 2018

Extensions: Add Extensions.UnifiedExtensionUpdaterUpdateServiceErrors histogram.

Now that the update client can return update service error details, this
information can be used to understand more about what kind of service
errors the update service may encouter when doing update checks.

Bug:  869663 
Change-Id: I10c693b05ebbe6c1f804606c4749acb0a178e7bd
Reviewed-on: https://chromium-review.googlesource.com/1180076
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Robert Kaplow (slow) <rkaplow@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Commit-Queue: Minh Nguyen <mxnguyen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584585}
[modify] https://crrev.com/6e0e7cd9bf10a61f443ba63115e86e9204d061ae/extensions/browser/updater/update_service.cc
[modify] https://crrev.com/6e0e7cd9bf10a61f443ba63115e86e9204d061ae/extensions/browser/updater/update_service.h
[modify] https://crrev.com/6e0e7cd9bf10a61f443ba63115e86e9204d061ae/tools/metrics/histograms/histograms.xml

Sign in to add a comment