Marking with RB label as this is a regression issue.

Looping in M57 TPM as well. 

Not reproduce on Chrome 57.0.2970.0 - Ubuntu 14.04

I suspect some sort of sync hiccup here, but stevenjb@ can you take a look?

I can reproduce this with both old and new settings, so something regressed. I will take a look.

ltian@ / ianwen@

Could this be caused by https://codereview.chromium.org/2487633003 ?
Or https://codereview.chromium.org/2347973002 ?

Given that this affects old settings also, it is not a MD Settings specific issue.

I suspect this bug is caused by my change because it seems is about sync or setting default provider. My change is just about whether a search engine should be displayed in the upper list or bottom list of default engine setting page. 

I did a bisect, the failure occurs in 9016 (Chrome 57.0.2929.0) but not 9015 (Chrome 57.0.2926.0). The chrome changes in that range are:

https://chromium.googlesource.com/chromium/src/+log/57.0.2926.0..57.0.2929.0?pretty=fuller&n=10000

That does not include either of the two suspects in comment #5. I can try bisecting that range later this week.

In testing I discovered that 'Reset settings' is changing the preference locally, but not syncing it, so other devices still have 'Bing' as the search provider. Unfortunately just searching for 'sync' in that changelist is not helpful.

bisect complete, this is the culprit: https://codereview.chromium.org/2516963002

Aaand it looks like the offending change was already reverted for  issue 679569  and  issue 679470 : https://codereview.chromium.org/2623833005

[Auto-generated comment by a script] We noticed that this issue is targeted for M-57; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-57 label, otherwise remove Merge-TBD label. Thanks.

I lie. That revert touched template_url.cc but was a different change entirely (by the same author).

I will confirm whether or not the problem is still in ToT.

I was able to confirm that the bug still exists in ToT, and that the following two reverts are required to fix it (the second patch modifies the first patch):

$ git revert e128f9d8f6ebe7064709a42805e3d3233f521e31
$ git revert 58606e51c44b74f89cf5fbbb370a78731acd942a

It's not clear to me this is a bug.

Basically, if you reset your local settings, then sync down a setting, the synced setting replaces the reset local setting.  That seems like Working As Intended.  If anything, the change in question fixed this (which seems consistent with it being about fixing broken search engine matching code).

If we want "reset settings" to also somehow reset some synced settings, then people who understand more about "reset settings" and sync than me need to be the ones to decide that, and implement it.

Marking Needs-Feedback in the sense of trying to determine why this is perceived to be a bug, so I can figure out if what I'm saying above is wrong.

This is an unexpected change in behavior, and is inconsistent with how other settings are synced and reset.

e.g. if we set 'Show Home button' in preferences, that preference syncs. If we then click 'Reset settings', the 'Show home button' preference is reset to false and the new value is synced to other browsers / devices.

The bug here specifically appears to be that 'reset settings' is not syncing the change to the default.search_provider.synced_guid value.

OK, so to restate what you're saying (I haven't verified any of this), the issue is that the default search provider is a "syncable" setting, and that normally "reset local settings" updates the synced versions of all syncable settings, but it doesn't update this one.

One question is whether this is truly a regression, in that I'm not sure we updated this synced setting on reset before -- I think it's plausible we didn't update it on reset, but we also didn't update it when signing back in, so the behavior looked correct locally (but would have been incorrect remotely).  However, I don't know for sure.

The answer to that second question would dictate whether the changes in question here are really responsible for this.

I didn't mean to remove a-v-y from the thread here.  a-v-y, can you look into this, at least to the point of determining whether this is a regression from your changes?

You may be correct in that the previous behavior may have been masking a sync issue. We can verify that by testing whether a version without the change successfully syncs the reset value. I can try to investigate that later today, otherwise it would be Wed.

I just checked, and on a device with the patch reverted, we do correctly sync the preference on reset. i.e. setting the search engine to a non default value will sync the state to other devices, then resetting the value will also sync the reset (default) value correctly.

OK, so this is a true regression then.  Interesting.

a-v-y, if I don't hear from you within the next couple days saying you're looking into this, then I plan to revert the changes in question.

I will look at this problem in a couple of days.

I was able to reproduce and debug for a while this issue.
IMHO, while my fix is correct it uncovered another bug in TemplateUrlService.

After my change the condition on 
https://cs.chromium.org/chromium/src/components/search_engines/template_url_service.cc?q=ApplyDefaultSearchChangeNoMetrics&sq=package:chromium&l=1924
returns false for google engine and it prevents setting of kSyncedDefaultSearchProviderGUID pref.

As a result default search changes from RepairPrepopulatedSearchEngines call are not propagated to sync subsystem.
So the previous default search selected by user is returned after next sync attempt.
I'll continue looking at this issue and see what I can do.

I spend some time today debugging templateUrlService.
I see no fast fix for this problem without risk of breaking search functionality even more. 
I propose that you revert my changes and I will try to reland them later with knowledge of templateUrlService being dependent on TemplateURL::MatchesData bug.

OK, I'll revert these.

Reverted on trunk.  stevenjb, want to double-check that the problem is indeed fixed?

Requesting merge to M57 for r446165 and r446914.

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

(Update: the second patch only ever landed on 57, so only the first patch's revert needs to be merged.)

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Verified the issue on Chrome57.0.2987.30/CrOS 9202.17.0 - Big/Lars

This was merged in https://codereview.chromium.org/2672653002/ .