[Sync] Bridges with async storage cannot correctly handle changes during initialization. |
||||||
Issue descriptionI thought that once issue 709094 was fixed we wouldn't have this problem anymore. But trying to fix issue 761420 I'm realizing this is not the case. If you've already merged on a previous run on sync, you're starting up again, you're in the process of loading metadata, but that call hasn't come back from storage yet, and you're told to handle a local change, what do you do? The processor doesn't have metadata yet, it cannot correctly handle it. If you just send it to the store only, it's going to get lost and never synced, since no merge is ever going to happen. It's almost as if the proc needs to take that piece of data through SharedModelTypeProcessor::Put(), and hold onto it. Then use it during SharedModelTypeProcessor::ModelReadyToSync since it can now be committed. Note that the proc, and ideally the service too, needs to be able to discern between the scenarios where sync is disabled vs metadata hasn't reached the proc yet. Because in the former, we want services to be able to no-op instead of constructing objects to send to the proc.
,
Mar 6 2018
,
Mar 6 2018
CC+ Martin and Sylvain We hit this bug when trying to record the consent on iOS. The code reviewer pushed back as he believed the code is still flaky and we're just hiding the bug (I think he is correct) - CL where this happened https://chromium-review.googlesource.com/c/chromium/src/+/951484 My understanding from Martin is that this will be fixed in M67. I am thus marking this bug as assigned and raising its priority to P1. Markus: Feel free to reassign if you are not the right owner for this bug.
,
Mar 6 2018
,
Mar 29 2018
Markus: Ping! Do you plan to work on this?
,
Mar 29 2018
I'm currently building a workaround for consent auditor: https://crrev.com/c/980975
,
Apr 9 2018
Markus, friendly ping! Does the workaround from #6 solve the bug or have impact on priority?
,
Apr 9 2018
The workaround only handles userconsent events in user_event_sync_bridge.cc at the moment. It could be extended to handle other events as well but we didn't see a need for it as other features don't have the same reliability requirements. I guess the priority/milestone were only important for consents, so it can be reduced or the issue closed if it is not considered important.
,
Apr 27 2018
Triage ping: Any updates on this?
,
May 14 2018
Since the most important aspect of the issue tracked here (consent auditing) is fixed I'm closing and bug. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by gangwu@chromium.org
, Jan 17 2018