Fix extensions sync in cases where items change type

Fix extensions sync in cases where items change type

If an extension changes into an app or vice versa, we can be left with
an old sync data item even if the installed item is uninstalled. Then on
the next sync, we'll try re-installing based on the id in this old
record, so it becomes impossible to remove. The chrome webstore does not
normally allow items to change type, but it has happened in a few cases,
possibly either via their publishing API or one-off manual
inteventions. A notable case is Google Hangouts, which started out as a
packaged app and became an extension at some point.

This CL changes the sync logic to always delete the sync data item if
there is an installed item of a differing type. Previously, we only did
this in the special case of themes due to a bug where themes could end
up being recorded as extensions.

Also, while working on tests for this CL I discovered some unit and
browser tests where we were directly calling the ProcessSyncChanges
method on ExtensionSyncService without first calling
MergeDataAndStartSyncing, which is a violation of the expectations of
the SyncableService interface, so I've fixed those and added a DCHECK to
help prevent future occurences.

BUG=590692
Committed: https://crrev.com/e48ab754b00bbb2dee227adebb8936b3b13be77c
Cr-Commit-Position: refs/heads/master@{#380813}

[asargent_no_longer_on_chrome, 2016-03-09 23:19:57]

Description was changed from

==========
Fix sync for cases where items change type

If an extension changes into an app or vice versa, we can be left with
an old sync data item even if the installed item is uninstalled. Then on
the next sync, we'll try re-installing based on the id in this old
record, so it becomes impossible to remove. The chrome webstore does not
normally allow items to change type, but it has happened in a few cases,
possibly either via their publishing API or one-off manual
inteventions. A notable case is Google Hangouts, which started out as a
packaged app and became an extension at some point.

This CL changes the sync logic to always delete the sync data item if
there is an installed item of a differing type. Previously, we only did
this in the special case of themes due to a bug where themes could end
up being recorded as extensions.

Also, while working on tests for this CL I discovered some unit and
browser tests where we were directly calling the ProcessSyncChanges
method on ExtensionSyncService without first calling
MergeDataAndStartSyncing, which is a violation of the expectations of
the SyncableService interface, so I've fixed those and added a DCHECK to
help prevent future occurences.

BUG=517910
==========

to

==========
Fix extensions sync in cases where items change type

If an extension changes into an app or vice versa, we can be left with
an old sync data item even if the installed item is uninstalled. Then on
the next sync, we'll try re-installing based on the id in this old
record, so it becomes impossible to remove. The chrome webstore does not
normally allow items to change type, but it has happened in a few cases,
possibly either via their publishing API or one-off manual
inteventions. A notable case is Google Hangouts, which started out as a
packaged app and became an extension at some point.

This CL changes the sync logic to always delete the sync data item if
there is an installed item of a differing type. Previously, we only did
this in the special case of themes due to a bug where themes could end
up being recorded as extensions.

Also, while working on tests for this CL I discovered some unit and
browser tests where we were directly calling the ProcessSyncChanges
method on ExtensionSyncService without first calling
MergeDataAndStartSyncing, which is a violation of the expectations of
the SyncableService interface, so I've fixed those and added a DCHECK to
help prevent future occurences.

BUG=517910
==========

[asargent_no_longer_on_chrome, 2016-03-09 23:28:07]

asargent@chromium.org changed reviewers:
+ treib@chromium.org

[asargent_no_longer_on_chrome, 2016-03-09 23:28:07]

Sending to both treib and rdevlin.cronin since this sync stuff can be tricky and
can benefit from more scrutiny

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_service.cc:331:
bundle->PushSyncDeletion(id, extension_sync_data.GetSyncData());
treib/devlin: I noticed that we originally added this code just for themes back
in codereview.chromium.org/1492453002 - was there any reason we didn't make that
more general, apart from conservatism? Can you think of scenarios where this
will cause problems?

[asargent_no_longer_on_chrome, 2016-03-09 23:29:26]

Oops, hit submit too early. 

treib: please review

+cc devlin (for real this time) as FYI and in case you want to help sanity check

[Marc Treib, 2016-03-10 10:36:34]

Nice! LGTM, but please wait for Devlin to double-check. History proves that I'm
not very good at spotting possible sync problems...

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_sync_unittest.cc:108:
syncer::SyncData sync_data = change.sync_data();
const syncer::SyncData& ?

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_sync_unittest.cc:2051: // Make sure
we saw an app item deleted.
Hm, it's a bit unfortunate that we have to wait for a sync restart to get the
deletion - would be nicer if we could do it immediately. But then we'd have to
wait for the thing to be installed so we can re-check the type...
IMO not worth the complexity for this rare edge case. So all good here :)

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_service.cc:331:
bundle->PushSyncDeletion(id, extension_sync_data.GetSyncData());
On 2016/03/09 23:28:07, Antony Sargent wrote:
> treib/devlin: I noticed that we originally added this code just for themes
back
> in codereview.chromium.org/1492453002 - was there any reason we didn't make
that
> more general, apart from conservatism? Can you think of scenarios where this
> will cause problems?

It was meant as a temporary hack at that point, and I was trying to keep its
scope as small as possible. Nice to see my temporary hack find a permanent home
:D  (for some value of "nice")
I don't see any problems this could cause.
One nit: Please keep a comment here on why the ApplySyncData call is necessary.

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_service.cc:600: // from the call to
GrantPermissionsAndEnableExtension.
Ooh, nice catch!

[Devlin, 2016-03-10 18:08:28]

rdevlin.cronin@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2016-03-10 18:08:29]

lgtm

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_sync_unittest.cc:156: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_sync_unittest.cc:175: type,
syncer::SyncDataList(),
nit: indentation

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_sync_unittest.cc:2015:
InstallCRX(data_dir().AppendASCII("sync_datatypes").AppendASCII("v2.crx"),
Is v3.crx used?

[asargent_no_longer_on_chrome, 2016-03-11 23:40:59]

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_sync_unittest.cc:108:
syncer::SyncData sync_data = change.sync_data();
On 2016/03/10 10:36:34, Marc Treib wrote:
> const syncer::SyncData& ?

The sync_data() method actually returns a SyncData by value, and SyncData's are
deliberately cheap to copy (internally they use a shared immutable/ref-counted
SyncEntity object).

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_sync_unittest.cc:156: };
On 2016/03/10 18:08:29, Devlin wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_sync_unittest.cc:175: type,
syncer::SyncDataList(),
On 2016/03/10 18:08:29, Devlin wrote:
> nit: indentation

Done. (Not sure why git cl upload didn't warn me, usually it does. There are a
few other minor changes that git cl format introduced)

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_sync_unittest.cc:2015:
InstallCRX(data_dir().AppendASCII("sync_datatypes").AppendASCII("v2.crx"),
On 2016/03/10 18:08:29, Devlin wrote:
> Is v3.crx used?

Good catch - early on I had been thinking of adding an additional test of going
from an extension to an app (v3.crx has a higher version number but is back to
being an app), but then at some point decided it probably wasn't worth it and
forgot to remove v3.crx. I've removed it now.

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/1778923003/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_sync_service.cc:331:
bundle->PushSyncDeletion(id, extension_sync_data.GetSyncData());
> One nit: Please keep a comment here on why the ApplySyncData call is
necessary.

added comment back in

[asargent_no_longer_on_chrome, 2016-03-11 23:41:14]

The CQ bit was checked by asargent@chromium.org

[asargent_no_longer_on_chrome, 2016-03-11 23:41:15]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1778923003/#ps20001
(title: "review feedback")

[asargent_no_longer_on_chrome, 2016-03-11 23:41:22]

The CQ bit was unchecked by asargent@chromium.org

[asargent_no_longer_on_chrome, 2016-03-11 23:42:07]

Description was changed from

==========
Fix extensions sync in cases where items change type

If an extension changes into an app or vice versa, we can be left with
an old sync data item even if the installed item is uninstalled. Then on
the next sync, we'll try re-installing based on the id in this old
record, so it becomes impossible to remove. The chrome webstore does not
normally allow items to change type, but it has happened in a few cases,
possibly either via their publishing API or one-off manual
inteventions. A notable case is Google Hangouts, which started out as a
packaged app and became an extension at some point.

This CL changes the sync logic to always delete the sync data item if
there is an installed item of a differing type. Previously, we only did
this in the special case of themes due to a bug where themes could end
up being recorded as extensions.

Also, while working on tests for this CL I discovered some unit and
browser tests where we were directly calling the ProcessSyncChanges
method on ExtensionSyncService without first calling
MergeDataAndStartSyncing, which is a violation of the expectations of
the SyncableService interface, so I've fixed those and added a DCHECK to
help prevent future occurences.

BUG=517910
==========

to

==========
Fix extensions sync in cases where items change type

If an extension changes into an app or vice versa, we can be left with
an old sync data item even if the installed item is uninstalled. Then on
the next sync, we'll try re-installing based on the id in this old
record, so it becomes impossible to remove. The chrome webstore does not
normally allow items to change type, but it has happened in a few cases,
possibly either via their publishing API or one-off manual
inteventions. A notable case is Google Hangouts, which started out as a
packaged app and became an extension at some point.

This CL changes the sync logic to always delete the sync data item if
there is an installed item of a differing type. Previously, we only did
this in the special case of themes due to a bug where themes could end
up being recorded as extensions.

Also, while working on tests for this CL I discovered some unit and
browser tests where we were directly calling the ProcessSyncChanges
method on ExtensionSyncService without first calling
MergeDataAndStartSyncing, which is a violation of the expectations of
the SyncableService interface, so I've fixed those and added a DCHECK to
help prevent future occurences.

BUG=590692
==========

[asargent_no_longer_on_chrome, 2016-03-11 23:42:39]

The CQ bit was checked by asargent@chromium.org

[commit-bot: I haz the power, 2016-03-11 23:43:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1778923003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1778923003/20001

[commit-bot: I haz the power, 2016-03-12 00:59:28]

Description was changed from

==========
Fix extensions sync in cases where items change type

If an extension changes into an app or vice versa, we can be left with
an old sync data item even if the installed item is uninstalled. Then on
the next sync, we'll try re-installing based on the id in this old
record, so it becomes impossible to remove. The chrome webstore does not
normally allow items to change type, but it has happened in a few cases,
possibly either via their publishing API or one-off manual
inteventions. A notable case is Google Hangouts, which started out as a
packaged app and became an extension at some point.

This CL changes the sync logic to always delete the sync data item if
there is an installed item of a differing type. Previously, we only did
this in the special case of themes due to a bug where themes could end
up being recorded as extensions.

Also, while working on tests for this CL I discovered some unit and
browser tests where we were directly calling the ProcessSyncChanges
method on ExtensionSyncService without first calling
MergeDataAndStartSyncing, which is a violation of the expectations of
the SyncableService interface, so I've fixed those and added a DCHECK to
help prevent future occurences.

BUG=590692
==========

to

==========
Fix extensions sync in cases where items change type

If an extension changes into an app or vice versa, we can be left with
an old sync data item even if the installed item is uninstalled. Then on
the next sync, we'll try re-installing based on the id in this old
record, so it becomes impossible to remove. The chrome webstore does not
normally allow items to change type, but it has happened in a few cases,
possibly either via their publishing API or one-off manual
inteventions. A notable case is Google Hangouts, which started out as a
packaged app and became an extension at some point.

This CL changes the sync logic to always delete the sync data item if
there is an installed item of a differing type. Previously, we only did
this in the special case of themes due to a bug where themes could end
up being recorded as extensions.

Also, while working on tests for this CL I discovered some unit and
browser tests where we were directly calling the ProcessSyncChanges
method on ExtensionSyncService without first calling
MergeDataAndStartSyncing, which is a violation of the expectations of
the SyncableService interface, so I've fixed those and added a DCHECK to
help prevent future occurences.

BUG=590692
==========

[commit-bot: I haz the power, 2016-03-12 00:59:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-12 01:00:42]

Description was changed from

==========
Fix extensions sync in cases where items change type

If an extension changes into an app or vice versa, we can be left with
an old sync data item even if the installed item is uninstalled. Then on
the next sync, we'll try re-installing based on the id in this old
record, so it becomes impossible to remove. The chrome webstore does not
normally allow items to change type, but it has happened in a few cases,
possibly either via their publishing API or one-off manual
inteventions. A notable case is Google Hangouts, which started out as a
packaged app and became an extension at some point.

This CL changes the sync logic to always delete the sync data item if
there is an installed item of a differing type. Previously, we only did
this in the special case of themes due to a bug where themes could end
up being recorded as extensions.

Also, while working on tests for this CL I discovered some unit and
browser tests where we were directly calling the ProcessSyncChanges
method on ExtensionSyncService without first calling
MergeDataAndStartSyncing, which is a violation of the expectations of
the SyncableService interface, so I've fixed those and added a DCHECK to
help prevent future occurences.

BUG=590692
==========

to

==========
Fix extensions sync in cases where items change type

If an extension changes into an app or vice versa, we can be left with
an old sync data item even if the installed item is uninstalled. Then on
the next sync, we'll try re-installing based on the id in this old
record, so it becomes impossible to remove. The chrome webstore does not
normally allow items to change type, but it has happened in a few cases,
possibly either via their publishing API or one-off manual
inteventions. A notable case is Google Hangouts, which started out as a
packaged app and became an extension at some point.

This CL changes the sync logic to always delete the sync data item if
there is an installed item of a differing type. Previously, we only did
this in the special case of themes due to a bug where themes could end
up being recorded as extensions.

Also, while working on tests for this CL I discovered some unit and
browser tests where we were directly calling the ProcessSyncChanges
method on ExtensionSyncService without first calling
MergeDataAndStartSyncing, which is a violation of the expectations of
the SyncableService interface, so I've fixed those and added a DCHECK to
help prevent future occurences.

BUG=590692

Committed: https://crrev.com/e48ab754b00bbb2dee227adebb8936b3b13be77c
Cr-Commit-Position: refs/heads/master@{#380813}
==========

[commit-bot: I haz the power, 2016-03-12 01:00:43]

Patchset 2 (id:??) landed as
https://crrev.com/e48ab754b00bbb2dee227adebb8936b3b13be77c
Cr-Commit-Position: refs/heads/master@{#380813}