IIUC, there is a race here. The following interleaving is possible:

[A component update begins]
RemoveComponent is called
ImageLoader removes component
[component update completes, ImageLoader installs new version of the component]
UnregisterComponent is called

In this case, we're left with an extra copy of the component owned by ImageLoader. Two ideas around this are:
(1) We could introduce a callback in UnregisterComponent and you could UnregisterComponent then call loader->RemoveComponent. In this case, we might have an race with shutdown situations, though.
(2) We could introduce a flag in the installer's OnCustomInstall that doesn't give ImageLoader a copy if RemoveComponent has been called. (Figuring out the threading requirements of the flag may be important.)


Let's say we use (1), I'm not sure what the race you mean by race with shutdown situations. But doing it in this way does not solve another existing race (happens rarely though):

[A component update begins]
[component update completes]
RemoveComponent is called
UnregisterComponent is called
ImageLoader removes component
[ImageLoader installs new version of the component]

The root cause for this is that we are not executing sequentially and transactionally here. Two proposals: (1.1) use SequencedTaskRunner to schedule ImageLoader operations. Then in ImageLoaderClient, make sure that operations are atomic and execution sequence is consistent with calling sequence. (1.2) on top of (1.1), introduce a mechanism to disallow UpdateComponent&RemoveComponent from interleaving each other. Doing it correctly should guarantee consistency. 

Regarding (2), it is a handy fix and should solve this properly. I'm a bit hesitant about adding the flag indicating 'Remove' is in progress since update_client already has an 'update in progress' state.

 

Background

IIUC, there is a race here. The following interleaving is possible:

[A component update begins]
RemoveComponent is called
ImageLoader removes component
[component update completes, ImageLoader installs new version of the component]
UnregisterComponent is called

In this case, we're left with an extra copy of the component owned by ImageLoader. Two ideas around this are:
(1) We could introduce a callback in UnregisterComponent and you could UnregisterComponent then call loader->RemoveComponent. In this case, we might have an race with shutdown situations, though.
(2) We could introduce a flag in the installer's OnCustomInstall that doesn't give ImageLoader a copy if RemoveComponent has been called. (Figuring out the threading requirements of the flag may be important.)


Let's say we use (1), I'm not sure what the race you mean by race with shutdown situations. But doing it in this way does not solve another existing race (happens rarely though):

[A component update begins]
[component update completes]
RemoveComponent is called
UnregisterComponent is called
ImageLoader removes component
[ImageLoader installs new version of the component]

The root cause for this is that we are not executing sequentially and transactionally here. Two proposals: (1.1) use SequencedTaskRunner to schedule ImageLoader operations. Then in ImageLoaderClient, make sure that operations are atomic and execution sequence is consistent with calling sequence. (1.2) on top of (1.1), introduce a mechanism to disallow UpdateComponent&RemoveComponent from interleaving each other. Doing it correctly should guarantee consistency. 

Regarding (2), it is a handy fix and should solve this properly. I'm a bit hesitant about adding the flag indicating 'Remove' is in progress since update_client already has an 'update in progress' state.

Plan

Currently OnCustomInstall which sends imageloader dbus operations is invoked in background thread: 

constexpr base::TaskTraits kTaskTraits = {
    base::MayBlock(), base::TaskPriority::BACKGROUND,
    base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN};

Inside CustomInstall, we call imageloader dbus API on UI thread. We should switch it to SequencedTaskRunner. 

Then make sure dbus operations is atomic and sequenced based on invocation time.
 

###Background###

IIUC, there is a race here. The following interleaving is possible:

[A component update begins]
RemoveComponent is called
ImageLoader removes component
[component update completes, ImageLoader installs new version of the component]
UnregisterComponent is called

In this case, we're left with an extra copy of the component owned by ImageLoader. Two ideas around this are:
(1) We could introduce a callback in UnregisterComponent and you could UnregisterComponent then call loader->RemoveComponent. In this case, we might have an race with shutdown situations, though.
(2) We could introduce a flag in the installer's OnCustomInstall that doesn't give ImageLoader a copy if RemoveComponent has been called. (Figuring out the threading requirements of the flag may be important.)


Let's say we use (1), I'm not sure what the race you mean by race with shutdown situations. But doing it in this way does not solve another existing race (happens rarely though):

[A component update begins]
[component update completes]
RemoveComponent is called
UnregisterComponent is called
ImageLoader removes component
[ImageLoader installs new version of the component]

The root cause for this is that we are not executing sequentially and transactionally here. Two proposals: (1.1) use SequencedTaskRunner to schedule ImageLoader operations. Then in ImageLoaderClient, make sure that operations are atomic and execution sequence is consistent with calling sequence. (1.2) on top of (1.1), introduce a mechanism to disallow UpdateComponent&RemoveComponent from interleaving each other. Doing it correctly should guarantee consistency. 

Regarding (2), it is a handy fix and should solve this properly. I'm a bit hesitant about adding the flag indicating 'Remove' is in progress since update_client already has an 'update in progress' state.

###Plan###

Currently OnCustomInstall which sends imageloader dbus operations is invoked in background thread: 

constexpr base::TaskTraits kTaskTraits = {
    base::MayBlock(), base::TaskPriority::BACKGROUND,
    base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN};

Inside CustomInstall, we call imageloader dbus API on UI thread. We should switch it to SequencedTaskRunner. 

Then make sure dbus operations is atomic and sequenced based on invocation time.
 

###Background###

IIUC, there is a race here. The following interleaving is possible:

[A component update begins]
RemoveComponent is called
ImageLoader removes component
[component update completes, ImageLoader installs new version of the component]
UnregisterComponent is called

In this case, we're left with an extra copy of the component owned by ImageLoader. Two ideas around this are:
(1) We could introduce a callback in UnregisterComponent and you could UnregisterComponent then call loader->RemoveComponent. In this case, we might have an race with shutdown situations, though.
(2) We could introduce a flag in the installer's OnCustomInstall that doesn't give ImageLoader a copy if RemoveComponent has been called. (Figuring out the threading requirements of the flag may be important.)


Let's say we use (1), I'm not sure what the race you mean by race with shutdown situations. But doing it in this way does not solve another existing race (happens rarely though):

[A component update begins]
[component update completes]
RemoveComponent is called
UnregisterComponent is called
ImageLoader removes component
[ImageLoader installs new version of the component]

The root cause for this is that we are not executing sequentially and transactionally here. Two proposals: (1.1) use SequencedTaskRunner to schedule ImageLoader operations. Then in ImageLoaderClient, make sure that operations are atomic and execution sequence is consistent with calling sequence. (1.2) on top of (1.1), introduce a mechanism to disallow UpdateComponent&RemoveComponent from interleaving each other. Doing it correctly should guarantee consistency. 

Regarding (2), it is a handy fix and should solve this properly. I'm a bit hesitant about adding the flag indicating 'Remove' is in progress since update_client already has an 'update in progress' state.

###Plan###

1. Currently OnCustomInstall which sends imageloader dbus operations is invoked in background thread: 

constexpr base::TaskTraits kTaskTraits = {
    base::MayBlock(), base::TaskPriority::BACKGROUND,
    base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN};

Inside CustomInstall, we call imageloader dbus API on UI thread which guarantees sequence and thread safety.

2. add Uninstalling state in component updater which prevent component updater from starting other operations (update, install, uninstall, etc.).

3. cancel on-demand install when either uninstall or update is in progress.

4. during uninstall, remove (unmount) imageloader copy first before remove copy (main copy) in chrome. two benefits: (1) cancel uninstall if imageloader fails to remove (due to in use, fs failure, etc.), avoid dangling copy in imageloader. (2) only certain components are removable. if we remove main copy first, then we need to check twice in both chrome and imageloader.