Rewrite TemplateURLService's SyncableService implmentation to avoid sending ACTION_DELETEs to Sync.

chrome/browser/search_engines/template_url_service.cc

Index: chrome/browser/search_engines/template_url_service.cc
diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc
index 928525a0fd9e8949461ebc80f6eee449068fe468..46c31d1211907e39b1f759fa7fdc0daf1ed207e8 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -191,6 +191,14 @@ std::string OldBaseURLSearchTermsData::GoogleBaseURLValue() const {
   return old_base_url;
 }
 
+// Returns true if |turl|'s GUID is not found inside |sync_data|. We use this
+// in MergeDataAndStartSyncing to differentiate between TemplateURLs from Sync
+// and TemplateURLs that were initially local, assuming |sync_data| is the
+// |initial_sync_data| parameter.
+bool IsFromSync(const TemplateURL* turl, SyncDataMap& sync_data) {
+  return (sync_data.find(turl->sync_guid()) != sync_data.end());
+}
+
 }  // namespace
 
 
@@ -997,17 +1005,19 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
         LOG(ERROR) << "Trying to add an existing TemplateURL.";
         continue;
       }
-      std::string guid = turl->sync_guid();
-      if (!existing_keyword_turl || ResolveSyncKeywordConflict(turl.get(),
-          existing_keyword_turl, &new_changes)) {
-        // Force the local ID to kInvalidTemplateURLID so we can add it.
-        TemplateURLData data(turl->data());
-        data.id = kInvalidTemplateURLID;
-        Add(new TemplateURL(profile_, data));
-
-        // Possibly set the newly added |turl| as the default search provider.
-        SetDefaultSearchProviderIfNewlySynced(guid);
+      const std::string guid = turl->sync_guid();
+      if (existing_keyword_turl) {
+        // Resolve any conflicts so we can safely add the new entry.
+        ResolveSyncKeywordConflict2(turl.get(), existing_keyword_turl,
+                                    &new_changes);
       }
+      // Force the local ID to kInvalidTemplateURLID so we can add it.
+      TemplateURLData data(turl->data());
+      data.id = kInvalidTemplateURLID;
+      Add(new TemplateURL(profile_, data));
+
+      // Possibly set the newly added |turl| as the default search provider.
+      SetDefaultSearchProviderIfNewlySynced(guid);
     } else if (iter->change_type() == syncer::SyncChange::ACTION_UPDATE) {
       if (!existing_turl) {
         NOTREACHED() << "Unexpected sync change state.";
@@ -1017,16 +1027,11 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
         LOG(ERROR) << "Trying to update a non-existent TemplateURL.";
         continue;
       }
-      // Possibly resolve a keyword conflict if they have the same keywords but
-      // are not the same entry.
-      if (existing_keyword_turl && (existing_keyword_turl != existing_turl) &&
-          !ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl,
-                                      &new_changes)) {
-        // Note that because we're processing changes, this Remove() call won't
-        // generate an ACTION_DELETE; but ResolveSyncKeywordConflict() did
-        // already, so we should be OK.
-        Remove(existing_turl);
-        continue;
+      if (existing_keyword_turl && (existing_keyword_turl != existing_turl)) {
+        // Resolve any conflicts with other entries so we can safely update the
+        // keyword.
+        ResolveSyncKeywordConflict2(turl.get(), existing_keyword_turl,
+                                    &new_changes);
       }
       UIThreadSearchTermsData search_terms_data(existing_turl->profile());
       if (UpdateNoNotify(existing_turl, *turl, search_terms_data))
@@ -1114,6 +1119,7 @@ syncer::SyncError TemplateURLService::MergeDataAndStartSyncing(
     }
 
     if (local_turl) {
+      DCHECK(IsFromSync(local_turl, sync_data_map));
       // This local search engine is already synced. If the timestamp differs
       // from Sync, we need to update locally or to the cloud. Note that if the
       // timestamps are equal, we touch neither.
@@ -1144,27 +1150,11 @@ syncer::SyncError TemplateURLService::MergeDataAndStartSyncing(
         // Merge duplicates and remove the processed local TURL from the map.
         std::string old_guid = dupe_turl->sync_guid();
         MergeSyncAndLocalURLDuplicates(sync_turl.release(), dupe_turl,
-                                       &new_changes);
+                                       sync_data_map, &new_changes);
         local_data_map.erase(old_guid);
       } else {
-        std::string guid = sync_turl->sync_guid();
-        // Keyword conflict is possible in this case. Resolve it first before
-        // adding the new TemplateURL. Note that we don't remove the local TURL
-        // from local_data_map in this case as it may still need to be pushed to
-        // the cloud.  We also explicitly don't resolve conflicts against
-        // extension keywords; see comments in ProcessSyncChanges().
-        TemplateURL* existing_keyword_turl =
-            FindNonExtensionTemplateURLForKeyword(sync_turl->keyword());
-        if (!existing_keyword_turl || ResolveSyncKeywordConflict(
-            sync_turl.get(), existing_keyword_turl, &new_changes)) {
-          // Force the local ID to kInvalidTemplateURLID so we can add it.
-          TemplateURLData data(sync_turl->data());
-          data.id = kInvalidTemplateURLID;
-          Add(new TemplateURL(profile_, data));
-
-          // Possibly set the newly added |turl| as the default search provider.
-          SetDefaultSearchProviderIfNewlySynced(guid);
-        }
+        MergeInSyncTemplateURL(sync_turl.get(), sync_data_map, &new_changes,

[Nicolas Zea, 2012/07/23 20:20:52]

given that this method supports finding a conflicting local turl and taking it
into account, do you need the dupe detection at this level? Aren't they both
looking for a duplicate by keyword?

[SteveT, 2012/07/23 21:07:00]

So the above method (MergeSyncAndLocalURLDuplicates) finds true-dupes (that is,
both the Keyword and URL are the same).

I recall that we did discuss that maybe we don't care if the URL matches or not,
and we should just take whichever entry is "better"?

What do you think? Should we attempt to be extra smart about matching URLs, or
treat them no differently than any other keyword conflict?

[Nicolas Zea, 2012/07/23 22:14:44]

How is the behavior between the two different? It seems like they both wind up
looking at modification time anyways, while MergeInSyncTemplateURL also looks at
created by policy and default? MergeInSync looks like it's basically doing the
same work.

[SteveT, 2012/07/24 14:46:30]

Actually yeah, MergeInSyncTemplateURL is sort of a superset of
MergeSyncAndLocalURLDuplicates. In fact, it's a little bit smarter in that it
considers policy and DSP, like you said.

I had thought that MergeSyncAndLocalURLDuplicates handled a case where the URLs
are the same that might be missed by MergeIn, but it seems like MergeIn
implicitly takes care of that.

MergeIn also gracefully handles the case where |local_turl| is actually from
Sync (where we have to uniquify). MergeSyncAndLocalURLDuplicates doesn't handle
this case, but like I asserted in an earlier email, this is currently not
possible due to the data we have in the server.


So I will simplify this section to just called MergeInSyncTemplateURL.

+                               &local_data_map);
       }
     }
   }
@@ -2309,6 +2299,131 @@ string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl,
   return keyword_candidate;
 }
 
+// Find the "better" TemplateURL based on a set of heuristics:
+//  * newer last_modified timestamp.
+//  * ...
+// Returns true if |turl1| is better, false otherwise. In the case of ties,
+// prefer |turl1| (return true).
+bool CompareTemplateURLs(const TemplateURL* turl1, const TemplateURL* turl2) {
+  return false;
+}
+
+void TemplateURLService::MergeInSyncTemplateURL(
+    TemplateURL* sync_turl,
+    SyncDataMap& sync_data,
+    syncer::SyncChangeList* change_list,
+    SyncDataMap* initial_data) {
+  DCHECK(sync_turl);
+  DCHECK(!GetTemplateURLForGUID(sync_turl->sync_guid()));
+  DCHECK(IsFromSync(sync_turl, sync_data));
+
+  TemplateURL* local_turl =
+      FindNonExtensionTemplateURLForKeyword(sync_turl->keyword());
+  bool should_add_sync_turl = local_turl == NULL;
+
+  // If there was no TemplateURL in the local model that conflicts with
+  // |sync_turl|, we can skip the following preparation steps and just
+  // |sync_turl| directly.
+  if (!should_add_sync_turl) {
+    DCHECK(local_turl);
+    const bool local_is_better =
+        (local_turl->last_modified() > sync_turl->last_modified()) ||
+        local_turl->created_by_policy() ||
+        (local_turl == GetDefaultSearchProvider());
+    if (IsFromSync(local_turl, sync_data)) {
+      // |local_turl| was merged in earlier from Sync, so we're not allowed to
+      // remove it. In this case, we want to uniquify the worse one and send
+      // an update for the changed keyword to sync. We can reuse the logic from
+      // ResolveSyncKeywordConflict2 for this.
+      ResolveSyncKeywordConflict2(sync_turl, local_turl, change_list);
+      should_add_sync_turl = true;
+    } else {
+      // |local_turl| is not yet known to Sync. If it is better, then we want to
+      // transfer its values up to sync. Otherwise, we remove it and allow the
+      // entry from Sync to overtake it in the model.
+      if (local_is_better) {
+        ResetTemplateURLGUID(local_turl, sync_turl->sync_guid());
+        syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
+        change_list->push_back(syncer::SyncChange(
+              FROM_HERE,
+              syncer::SyncChange::ACTION_UPDATE,
+              sync_data));
+        // Note that in this case we do not set |should_add_sync_turl|, since
+        // we've effectively "merged" it in by updating the local entry with
+        // its sync_guid.
+        // TODO(stevet/reviewer): If local_turl was the DSP, should we set the
+        // Synced GUID pref with the new GUID? What about in

[Nicolas Zea, 2012/07/23 20:20:52]

Hmm, interesting. Preferences are associated before search engines, so at this
point the pref GUID should match what's been synced. So, unless somewhere after
pref association but before this is called we're attempting to overwrite the
pref guid, it shouldn't be possible for the unsynced local turl to be the pref
guid.

If it's not possible for something to attempt to write the guid at startup, then
I'd say we shouldn't be messing with the pref guid here, although some sort of
histogram might be useful to double check.

[SteveT, 2012/07/23 21:07:00]

Oh, so we have an ordering guarantee now that Preferences will be associated
(and pref observers notified) before we get to this point?

So local_turl at this point _could_ be the DSP, but it should change to point to
a Synced entry sometime within MergeDataAndStartSyncing.

If that is the case, then yeah, let's not mess with the SyncedDSPGUID pref.

Removed the TODO.

[Nicolas Zea, 2012/07/23 22:14:44]

Yeah...it's not a guarantee, given that preferences might hit an error or
something.

That said, the case we're concerned about here is an unsynced search engine
being
the default, and losing the dupe-resolution with a synced search engine. If this
happens, then yes, I think we should set the pref GUID to the new one. If we
didn't,
then other clients are going to be stuck in a state of pending guid, since the
unsynced
local SE would never have been added.

I suspect we can only get into this state if SE/preferences hit some error/crash
in
a weird way...but honestly, in sync, if it _CAN_ happen...it will. And several
thousand
times at that.

[SteveT, 2012/07/24 14:46:30]

So the problem here isn't the local_turl losing conflict res (in fact, it can't,
since one requirement is to not be the DSP). The problem here is if an unsynced
local_turl wins... in which case we exchange it's guid for sync_turl's guid
(which is effectively deleting it).

In this case, if local_turl is the DSP, we should probably set the SyncedDSPGUID
pref with the new guid from sync. I wrote some code to do this.

The one issue with this is when the new pref has already arrived before search
engines sync, but we haven't yet received the expected default entry in our
MergeDataAndStartSyncing loop.

In this case, we will override the pref with the local default's GUID, which
sort of breaks how Preferences sync works, right? (The Prefs sync model always
prefers the value from Sync over the local value, but for this particular case
and pref, we're breaking that rule) 

Does this seem OK, or do we need to avoid doing this? We _could_ hold off on
setting the SyncedDSPGUID until the end of the MergeDataAndStartSyncing loop
(wait for our expected synced default to come in). If it doesn't, then we could
set the local DSP as the new default.

[Nicolas Zea, 2012/07/24 19:31:47]

Ah, I see. So we're in one of two states:
1) We locally changed the DSP, and there is no pending synced default change.
2) We locally changed the DSP while there was a pending synced default change.

For 1, I think it's safe to say that we should update the pref guid.
For 2, it's basically a valid race right? And we in fact do have a way to detect
the
the race, since we can check for pending DSP. So if we have a pending DSP,
don't overwrite real DSP pref, but update the local DSP engine to use the
new sync GUID.

WDYT?

[SteveT, 2012/07/24 21:51:18]

Yes, so in (1), pending_synced_default_change_ is not set, which means at the
beginning of MergeDataAndStartSyncing we did not expect one of the incoming
TURLs to be the synced DSP -OR- we already received the incoming DSP (earlier in
MergeData, or during Prefs' MergeData). Either way, it seems safe to set the
SyncedDSPGUID, since it's either a no-op (writing the same GUID into the pref),
or we're telling Sync that the DSP should be this new value.

One tricky edge case here stems from the fact that we are merging based on
keywords. Back when we had MergeSyncAndLocalDuplicates, doing this would be fine
since we know the URLs were the same. In this case, you are hoping that because
they had the same keywords, they have similar URLs pointing to the same search
host. So for example, if this edge case was triggered and the user had
local_turl keyword "q" and URL "google.com?q={searchTerm} and sync_turl had
keyword "q" with URL "yahoo.com?q={searchTerm}... the logic will assign this
yahoo search engine to be the default.

This is a weird edge case, but maybe it's OK because we want to go with the
notion of sync data winning.


In (2), the pending_synced_default_change_ flag is true, which means we are
still waiting on a synced TURL to come in from this batch of sync data. If this
currently sync_turl happens to be that entry... then the call to
SetDefaultSearchProviderIfNewlySynced will take care of that after we replace
local_turl's sync_guid with sync_turl's sync_guid.

But yeah, it sounds like this is the case where we don't want to override the
pref... since it'll lead to problems similar to what your initial fix to
SetDefaultSearchProviderNoNotify addressed (to not overwrite the pref when we're
waiting for the default to come). It's the same race issue.


Now what if this batch of sync data doesn't come in with an entry that has
sync_guid == the SyncedDSPGUID? (perhaps it was deleted due to the DSP swap
bug... Raz still needs to get us numbers on this)  In this case, would we want
to attempt to repair the sync data by setting the current local DSP to be the
synced default? (see my last my comment about waiting til the end of
MergeDataAndStartSyncing to do this)

[Nicolas Zea, 2012/07/24 22:13:37]

I don't think we should be attempting to repair anything. It's not clear to me
we have enough information to detect that the dsp is not on it's way. The
system can handle a pending DSP guid that never arrives. In this case, the
local DSP will remain the same (although the guid changed) right? I think
that's the right behavior if you have a DSP change that never arrives.

+        // MergeSyncAndLocalURLDuplicates, where we do something similar?
+      } else {
+        // We guarantee that this isn't the local search provider. Otherwise,
+        // local would have won.
+        DCHECK(local_turl != GetDefaultSearchProvider());
+        const std::string guid = local_turl->sync_guid();
+        Remove(local_turl);
+        // Also remove the entry from the initial data so that we don't push it
+        // up to Sync.
+        initial_data->erase(guid);

[Nicolas Zea, 2012/07/23 20:20:52]

does this need to be done for the case above as well?

[SteveT, 2012/07/23 21:07:00]

Hm, yes, you're right. In either case, since local_turl is not known to sync, it
will be completely overridden by the conflicting sync_turl. I'll pull the
erase() out.

+        should_add_sync_turl = true;
+      }
+    }
+  }
+
+  if (should_add_sync_turl) {
+    const std::string guid = sync_turl->sync_guid();
+    // Force the local ID to kInvalidTemplateURLID so we can add it.
+    TemplateURLData data(sync_turl->data());
+    data.id = kInvalidTemplateURLID;
+    Add(new TemplateURL(profile_, data));
+
+    // Possibly set the newly added |turl| as the default search provider.
+    // TODO(stevet/reviewer): Note that if this call notices a Default change,
+    // it will NOT set the pref since we are currently proccessing sync changes.
+    // See SetDefaultSearchProviderNoNotify.
+    // We might have to detect that case ourselves and explicitly set the pref?

[Nicolas Zea, 2012/07/23 20:20:52]

I'm not sure I follow; why would we want to update the pref at this point? This
method should never update I thought, since it's updating the DSP based on the
pref, right?

[SteveT, 2012/07/23 21:07:00]

Ah right, I was thinking about just SetDefaultSearchProviderNoNotify as opposed
to the extra logic layered in top by SetDefaultSearchProviderIfNewlySynced.

Also, from your comment above, it sounds like the canonical synced DSP GUID pref
should be synced before we even enter MergeDataAndStartSyncing, so calling just
SetDefaultSearchProviderIfNewlySynced here is the right thing to do.

Removed the TODO.

+    SetDefaultSearchProviderIfNewlySynced(guid);
+  }
+}
+
+void TemplateURLService::ResolveSyncKeywordConflict2(
+    TemplateURL* sync_turl,
+    TemplateURL* local_turl,

[Nicolas Zea, 2012/07/23 20:20:52]

|sync_turl| and |local_turl| don't really make sense anymore since they're both
known to sync?

[SteveT, 2012/07/23 21:07:00]

This is true, but local_turl is already located on the local model when this
method is asked to resolve conflicts... Either way, I'll rename |local_turl| to
|conflicting_turl| to be a bit more descriptive... Suggestions welcome.

[Nicolas Zea, 2012/07/23 22:14:44]

maybe unapplied_sync_turl and applied_sync_turl?

[SteveT, 2012/07/24 14:46:30]

Sounds good. Done.

+    syncer::SyncChangeList* change_list) {
+  DCHECK(loaded_);
+  DCHECK(sync_turl);
+  DCHECK(local_turl);
+  DCHECK(change_list);
+  DCHECK(local_turl->keyword() == sync_turl->keyword());
+  DCHECK(!local_turl->IsExtensionKeyword());
+
+  // Both |sync_turl| and |local_turl| are known to Sync, so don't delete
+  // either of them. Instead, determine which is "better" and uniquify the other
+  // one, sending an update to the server for the updated entry.
+  const bool local_is_better =
+      (local_turl->last_modified() > sync_turl->last_modified()) ||
+      local_turl->created_by_policy() ||
+      (local_turl == GetDefaultSearchProvider());
+  TemplateURL* loser = local_is_better ? local_turl : sync_turl;
+  string16 new_keyword = UniquifyKeyword(*loser, false);
+  DCHECK(!GetTemplateURLForKeyword(new_keyword));
+  syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*loser);
+  change_list->push_back(syncer::SyncChange(FROM_HERE,
+      syncer::SyncChange::ACTION_UPDATE,
+      sync_data));
+  if (local_is_better) {
+    // Just set the keyword of |sync_turl|. The caller is responsible for adding
+    // or updating sync_turl in the local model.
+    sync_turl->data_.SetKeyword(new_keyword);
+  } else {
+    // Update |local_turl| in the local model with the new keyword.
+    TemplateURLData data(local_turl->data());
+    data.SetKeyword(new_keyword);
+    TemplateURL new_turl(local_turl->profile(), data);
+    UIThreadSearchTermsData search_terms_data(local_turl->profile());
+    if (UpdateNoNotify(local_turl, new_turl, search_terms_data))
+      NotifyObservers();
+  }
+}
+
 bool TemplateURLService::ResolveSyncKeywordConflict(
     TemplateURL* sync_turl,
     TemplateURL* local_turl,
@@ -2316,9 +2431,9 @@ bool TemplateURLService::ResolveSyncKeywordConflict(
   DCHECK(loaded_);
   DCHECK(sync_turl);
   DCHECK(local_turl);
+  DCHECK(change_list);
   DCHECK(sync_turl->sync_guid() != local_turl->sync_guid());
   DCHECK(!local_turl->IsExtensionKeyword());
-  DCHECK(change_list);
 
   const bool local_is_better =
       (local_turl->last_modified() > sync_turl->last_modified()) ||
@@ -2389,6 +2504,7 @@ TemplateURL* TemplateURLService::FindDuplicateOfSyncTemplateURL(
 void TemplateURLService::MergeSyncAndLocalURLDuplicates(
     TemplateURL* sync_turl,
     TemplateURL* local_turl,
+    SyncDataMap& sync_data,
     syncer::SyncChangeList* change_list) {
   DCHECK(loaded_);
   DCHECK(sync_turl);
@@ -2407,10 +2523,12 @@ void TemplateURLService::MergeSyncAndLocalURLDuplicates(
 
     // See comments in ResolveSyncKeywordConflict() regarding generating an
     // ACTION_DELETE manually since Remove() won't do it.
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_DELETE,
-                                              sync_data));
+    if (IsFromSync(local_turl, sync_data)) {

[Nicolas Zea, 2012/07/23 20:20:52]

I don't think we want to delete the local turl if it's synced. Did you meant for
this to be here?

[SteveT, 2012/07/23 21:07:00]

Hm yeah, this is actually wrong.

What happens under our new no-deletes model in this case, when we have two
_synced_ entries that have the same keyword and URL? I guess we want to uniquify
one of them and keep both around.

Maybe as I suggested above, we can stop treating this as a special case and just
roll the logic into ResolveSyncKeywordConflicts2?

[Nicolas Zea, 2012/07/23 22:14:44]

Yeah, I think that's the right approach.

[SteveT, 2012/07/24 14:46:30]

Oops, when I said "roll the logic into ResolveSyncKeywordConflcits2" I actually
meant "just use MergeInSyncTemplateURL.

The special case I was referring to was the fact that we try to do something
special for when the URL is the same (combine them into one rather than
uniquifying).

+      syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
+      change_list->push_back(syncer::SyncChange(FROM_HERE,
+            syncer::SyncChange::ACTION_DELETE,
+            sync_data));
+    }
     Remove(local_turl);
 
     // Force the local ID to kInvalidTemplateURLID so we can add it.