[Sync] Maintain a global_id mapping to update UserEvents that references navigations

[Sync] Maintain a global_id mapping to update UserEvents that references navigations.

UserEvents are created and reference sessions data by global_id. It is
important that they travel over the wire in this format, as this is how
the sessions data is keyed into Footprints, and the sync server cannot
feasibly perform a persistent read before writing new UserEvents.
Because global_id is forced to be part of the wire interface and
specifics, it is also conveniently part of the interface to the
UserEventService.

However, while this global_id is seemingly sacred to sync, it comes
from simply the timestmap of the navigation entry. And if a page is
reloaded, this value can change. A problem arises when a UserEvent
references a navigation by a global_id that was updated and the old
value is never going to reach the sync server.

To fix we maintain two sets of mappings, one that allows us to map old
global_ids to new global_id, and one that allows us to efficiently look
up uncommitted UserEvents by global_id, so that if we get either a new
global_id or a new UserEvent we can quickly fix the global_id.

BUG=747621
Review-Url: https://codereview.chromium.org/2958303002
Cr-Commit-Position: refs/heads/master@{#488826}
Committed: https://chromium.googlesource.com/chromium/src/+/a58a1f014663671847efd294313b99fe71e1a6bf

[skym, 2017-06-28 17:11:50]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 17:12:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-06-28 17:20:19]

Description was changed from

==========
[Sync] Maintain a global_id mapping to update UserEvents that reference
session'to sessions.

BUG=701032
==========

to

==========
[Sync] Maintain a global_id mapping to update UserEvents that reference
session'to sessions.

UserEvents are created and reference sessions data by global_id. It is
important that they travel over the write in this format, as this is
how the sessions data is keyed into Footprints, and the sync server
cannot feasibly perform a persistent read before writing new
UserEvents. Because global_id is forced to be part of the wire
interface and specifics, it is also conveniently part of the interface
to  the UserEventService.

However, this while this global_id is seemingly sacred to sync, it
comes from simply the timestmap of the navigation entry. And if a page
is reloaded, this value can change. A problem arises when a UserEvent
references a navigation by a global_id that was updated and the old
value is never going to reach the sync server.

To fix we maintain two sets of mappings, one that allows us to map old
global_ids to new global_id, and one that allows us to efficiently look
up uncommitted UserEvents by global_id, so that if we get either a new
global_id or a new UserEvent we can quickly fix the global_id.

BUG=701032
==========

[skym, 2017-06-28 17:20:19]

skym@chromium.org changed reviewers:
+ holte@chromium.org, pnoland@chromium.org

[skym, 2017-06-28 17:20:28]

PTAL

[commit-bot: I haz the power, 2017-06-28 17:24:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-28 17:25:00]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[skym, 2017-06-28 17:32:07]

Forgot to assign reviewers to code:

holte@ - histograms.xml
pnoland@ - everything else

[skym, 2017-06-28 17:32:32]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 17:32:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 17:45:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-28 17:45:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Steven Holte, 2017-06-28 18:59:45]

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
File components/sync_sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1343:
UMA_HISTOGRAM_COUNTS("Sync.GlobalIdConflict", 1);
UMA_HISTOGRAM_COUNTS is an alias for UMA_HISTOGRAM_COUNTS_1M (Using 50 buckets
for values 1 -1M).  If you are always recording a value of 1, you should
probably instead just use an enum histogram with a single bucket.

[skym, 2017-06-28 22:44:25]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[Patrick Noland, 2017-06-28 22:44:38]

In the CL description, I think you meant to write "It is
important that they travel over the wire in this format" and "However, while
this global_id is seemingly sacred to sync, it
comes from simply the timestamp of the navigation entry"

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
File components/sync/user_events/global_id_mapper.h (right):

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
components/sync/user_events/global_id_mapper.h:17: class GlobalIdMapper {
Please add a comment explaining what this is class is for and why someone would
want to use it.

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
components/sync/user_events/global_id_mapper.h:25: virtual int64_t
GetLatestGlobalId(int64_t global_id) = 0;
I think I understand what this is doing, but the comment and name are kind of
confusing. First of all I don't think you meant to put | | around the second
global_id. Second, referring to a navigation is confusing since no other code or
comment here refers to navigation at all.  Third, the comment says "current
global id" when the function is named "latest" global id; using consistent
terminology would be helpful.

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
File components/sync/user_events/user_event_sync_bridge.cc (right):

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:112: while (iter !=
in_flight_nav_linked_events_.end()) {
I think you can use base::EraseIf here

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
File components/sync/user_events/user_event_sync_bridge_unittest.cc (right):

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge_unittest.cc:184:
bridge()->GetAllData(VerifyCallback({{storage_key, specifics3}}));
It's kind of hard to follow the different timestamps/ids through this test. Can
you make them variables with names(and rename the specifics they belong to) so
that it's apparent what you're expecting? e.g.

uint64_t first_id = 2u;
uint64_t second_id = 4u;
const UserEventSpecifics specifics_with_second_id(CreateSpecifics(1u, second_id,
3u));

mapper()->ChangeId(first_id, second_id) 
...
bridge()->GetAllData(VerifyCallback({{storage_key, specifics_with_second_id}}))

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
File components/sync_sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1306: if (g2u_iter !=
global_to_unique_.end()) {
This might look better as if (auto g2u_iter = ... != end()), but decide for
yourself how you feel about that idiom

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1348:
unique_to_current_global_.insert(u2g_iter,
if you use base::flat_map (see
https://cs.chromium.org/chromium/src/base/containers/README.md?type=cs) you can
use insert_or_assign, and your max size is small enough that it's a wash at
worst performance-wise

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1373: if
(global_to_unique_.size() > kNavigationTrackingCleanupThreshold) {
Can you use the range-based erase here?

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1383: while (u2g_iter !=
unique_to_current_global_.end()) {
I think you can use base::EraseIf here

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
File components/sync_sessions/sessions_sync_manager_unittest.cc (right):

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager_unittest.cc:2701: for (int i = 0;
i < 105; i++) {
Can you reference the constant instead of 105 here?

[commit-bot: I haz the power, 2017-06-28 22:44:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 00:25:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 00:25:48]

Dry run: This issue passed the CQ dry run.

[skym, 2017-06-30 21:04:57]

Description was changed from

==========
[Sync] Maintain a global_id mapping to update UserEvents that reference
session'to sessions.

UserEvents are created and reference sessions data by global_id. It is
important that they travel over the write in this format, as this is
how the sessions data is keyed into Footprints, and the sync server
cannot feasibly perform a persistent read before writing new
UserEvents. Because global_id is forced to be part of the wire
interface and specifics, it is also conveniently part of the interface
to  the UserEventService.

However, this while this global_id is seemingly sacred to sync, it
comes from simply the timestmap of the navigation entry. And if a page
is reloaded, this value can change. A problem arises when a UserEvent
references a navigation by a global_id that was updated and the old
value is never going to reach the sync server.

To fix we maintain two sets of mappings, one that allows us to map old
global_ids to new global_id, and one that allows us to efficiently look
up uncommitted UserEvents by global_id, so that if we get either a new
global_id or a new UserEvent we can quickly fix the global_id.

BUG=701032
==========

to

==========
[Sync] Maintain a global_id mapping to update UserEvents that reference
session'to sessions.

UserEvents are created and reference sessions data by global_id. It is
important that they travel over the wire in this format, as this is how
the sessions data is keyed into Footprints, and the sync server cannot
feasibly perform a persistent read before writing new UserEvents.
Because global_id is forced to be part of the wire interface and
specifics, it is also conveniently part of the interface to the
UserEventService.

However, while this global_id is seemingly sacred to sync, it comes
from simply the timestmap of the navigation entry. And if a page is
reloaded, this value can change. A problem arises when a UserEvent
references a navigation by a global_id that was updated and the old
value is never going to reach the sync server.

To fix we maintain two sets of mappings, one that allows us to map old
global_ids to new global_id, and one that allows us to efficiently look
up uncommitted UserEvents by global_id, so that if we get either a new
global_id or a new UserEvent we can quickly fix the global_id.

BUG=701032
==========

[skym, 2017-07-05 19:13:56]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-05 19:14:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-07-05 19:14:29]

Updates for comments.

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
File components/sync/user_events/global_id_mapper.h (right):

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
components/sync/user_events/global_id_mapper.h:17: class GlobalIdMapper {
On 2017/06/28 22:44:37, Patrick Noland wrote:
> Please add a comment explaining what this is class is for and why someone
would
> want to use it. 

Done.

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
components/sync/user_events/global_id_mapper.h:25: virtual int64_t
GetLatestGlobalId(int64_t global_id) = 0;
On 2017/06/28 22:44:37, Patrick Noland wrote:
> I think I understand what this is doing, but the comment and name are kind of
> confusing. First of all I don't think you meant to put | | around the second
> global_id. Second, referring to a navigation is confusing since no other code
or
> comment here refers to navigation at all.  Third, the comment says "current
> global id" when the function is named "latest" global id; using consistent
> terminology would be helpful. 

Done the first and third requests, the class comment now includes words about
navigations, so keeping that here as well.

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
File components/sync/user_events/user_event_sync_bridge.cc (right):

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:112: while (iter !=
in_flight_nav_linked_events_.end()) {
On 2017/06/28 22:44:37, Patrick Noland wrote:
> I think you can use base::EraseIf here

Done. Not sure if it's any simpler now, but it is more fancy!

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
File components/sync/user_events/user_event_sync_bridge_unittest.cc (right):

https://codereview.chromium.org/2958303002/diff/20001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge_unittest.cc:184:
bridge()->GetAllData(VerifyCallback({{storage_key, specifics3}}));
On 2017/06/28 22:44:37, Patrick Noland wrote:
> It's kind of hard to follow the different timestamps/ids through this test.
Can
> you make them variables with names(and rename the specifics they belong to) so
> that it's apparent what you're expecting? e.g.
> 
> uint64_t first_id = 2u;
> uint64_t second_id = 4u;
> const UserEventSpecifics specifics_with_second_id(CreateSpecifics(1u,
second_id,
> 3u));
> 
> mapper()->ChangeId(first_id, second_id) 
> ...
> bridge()->GetAllData(VerifyCallback({{storage_key,
specifics_with_second_id}}))

Done.

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
File components/sync_sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1306: if (g2u_iter !=
global_to_unique_.end()) {
On 2017/06/28 22:44:37, Patrick Noland wrote:
> This might look better as if (auto g2u_iter = ... != end()), but decide for
> yourself how you feel about that idiom

Acknowledged.

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1343:
UMA_HISTOGRAM_COUNTS("Sync.GlobalIdConflict", 1);
On 2017/06/28 18:59:45, Steven Holte wrote:
> UMA_HISTOGRAM_COUNTS is an alias for UMA_HISTOGRAM_COUNTS_1M (Using 50 buckets
> for values 1 -1M).  If you are always recording a value of 1, you should
> probably instead just use an enum histogram with a single bucket.

Done, switched to UMA_HISTOGRAM_ENUMERATION. Wasn't sure if I should emit 0 or 1
and what the units should be, so switched to a real enum and emitting all cases
here instead.

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1348:
unique_to_current_global_.insert(u2g_iter,
On 2017/06/28 22:44:37, Patrick Noland wrote:
> if you use base::flat_map (see
> https://cs.chromium.org/chromium/src/base/containers/README.md?type=cs) you
can
> use insert_or_assign, and your max size is small enough that it's a wash at
> worst performance-wise

I think it'd be a bit confusing to have these two maps have different types.
global_to_unique_ is likely going to sit at ~max capacity for most users, I'm
not sure about switching.

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1373: if
(global_to_unique_.size() > kNavigationTrackingCleanupThreshold) {
On 2017/06/28 22:44:37, Patrick Noland wrote:
> Can you use the range-based erase here? 

Done.

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1383: while (u2g_iter !=
unique_to_current_global_.end()) {
On 2017/06/28 22:44:37, Patrick Noland wrote:
> I think you can use base::EraseIf here

Done.

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
File components/sync_sessions/sessions_sync_manager_unittest.cc (right):

https://codereview.chromium.org/2958303002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager_unittest.cc:2701: for (int i = 0;
i < 105; i++) {
On 2017/06/28 22:44:37, Patrick Noland wrote:
> Can you reference the constant instead of 105 here?

I'd prefer to not move to the constant's declaration out of the .cc file just so
that a test can reference it.

[commit-bot: I haz the power, 2017-07-05 21:15:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-05 21:15:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not
started yet; builder either lacks capacity or does not exist (misspelled?))
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[Patrick Noland, 2017-07-05 23:59:32]

lgtm

https://codereview.chromium.org/2958303002/diff/80001/components/sync/user_ev...
File components/sync/user_events/user_event_sync_bridge.cc (right):

https://codereview.chromium.org/2958303002/diff/80001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:113: const
std::pair<int64_t, sync_pb::UserEventSpecifics> kv) {
Can you use auto here?

https://codereview.chromium.org/2958303002/diff/80001/components/sync_session...
File components/sync_sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/2958303002/diff/80001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1396: [this](const
std::pair<int, int64_t> kv) {
Can you use auto here?

[skym, 2017-07-06 17:28:55]

https://codereview.chromium.org/2958303002/diff/80001/components/sync/user_ev...
File components/sync/user_events/user_event_sync_bridge.cc (right):

https://codereview.chromium.org/2958303002/diff/80001/components/sync/user_ev...
components/sync/user_events/user_event_sync_bridge.cc:113: const
std::pair<int64_t, sync_pb::UserEventSpecifics> kv) {
On 2017/07/05 23:59:32, Patrick Noland wrote:
> Can you use auto here?

In C++14 you can https://en.wikipedia.org/wiki/C%2B%2B14#Generic_lambdas

https://codereview.chromium.org/2958303002/diff/80001/components/sync_session...
File components/sync_sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/2958303002/diff/80001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1396: [this](const
std::pair<int, int64_t> kv) {
On 2017/07/05 23:59:32, Patrick Noland wrote:
> Can you use auto here?

Not in C++11 :(

[skym, 2017-07-21 22:53:52]

Friendly ping holte@, I thought this was committed a while ago. Looks like I
completely forgot about this.

[skym, 2017-07-21 23:07:29]

Description was changed from

==========
[Sync] Maintain a global_id mapping to update UserEvents that reference
session'to sessions.

UserEvents are created and reference sessions data by global_id. It is
important that they travel over the wire in this format, as this is how
the sessions data is keyed into Footprints, and the sync server cannot
feasibly perform a persistent read before writing new UserEvents.
Because global_id is forced to be part of the wire interface and
specifics, it is also conveniently part of the interface to the
UserEventService.

However, while this global_id is seemingly sacred to sync, it comes
from simply the timestmap of the navigation entry. And if a page is
reloaded, this value can change. A problem arises when a UserEvent
references a navigation by a global_id that was updated and the old
value is never going to reach the sync server.

To fix we maintain two sets of mappings, one that allows us to map old
global_ids to new global_id, and one that allows us to efficiently look
up uncommitted UserEvents by global_id, so that if we get either a new
global_id or a new UserEvent we can quickly fix the global_id.

BUG=701032
==========

to

==========
[Sync] Maintain a global_id mapping to update UserEvents that references
navigations.

UserEvents are created and reference sessions data by global_id. It is
important that they travel over the wire in this format, as this is how
the sessions data is keyed into Footprints, and the sync server cannot
feasibly perform a persistent read before writing new UserEvents.
Because global_id is forced to be part of the wire interface and
specifics, it is also conveniently part of the interface to the
UserEventService.

However, while this global_id is seemingly sacred to sync, it comes
from simply the timestmap of the navigation entry. And if a page is
reloaded, this value can change. A problem arises when a UserEvent
references a navigation by a global_id that was updated and the old
value is never going to reach the sync server.

To fix we maintain two sets of mappings, one that allows us to map old
global_ids to new global_id, and one that allows us to efficiently look
up uncommitted UserEvents by global_id, so that if we get either a new
global_id or a new UserEvent we can quickly fix the global_id.

BUG=701032
==========

[skym, 2017-07-21 23:08:03]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-21 23:08:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Steven Holte, 2017-07-21 23:13:42]

histograms lgtm

[skym, 2017-07-22 00:06:27]

Description was changed from

==========
[Sync] Maintain a global_id mapping to update UserEvents that references
navigations.

UserEvents are created and reference sessions data by global_id. It is
important that they travel over the wire in this format, as this is how
the sessions data is keyed into Footprints, and the sync server cannot
feasibly perform a persistent read before writing new UserEvents.
Because global_id is forced to be part of the wire interface and
specifics, it is also conveniently part of the interface to the
UserEventService.

However, while this global_id is seemingly sacred to sync, it comes
from simply the timestmap of the navigation entry. And if a page is
reloaded, this value can change. A problem arises when a UserEvent
references a navigation by a global_id that was updated and the old
value is never going to reach the sync server.

To fix we maintain two sets of mappings, one that allows us to map old
global_ids to new global_id, and one that allows us to efficiently look
up uncommitted UserEvents by global_id, so that if we get either a new
global_id or a new UserEvent we can quickly fix the global_id.

BUG=701032
==========

to

==========
[Sync] Maintain a global_id mapping to update UserEvents that references
navigations.

UserEvents are created and reference sessions data by global_id. It is
important that they travel over the wire in this format, as this is how
the sessions data is keyed into Footprints, and the sync server cannot
feasibly perform a persistent read before writing new UserEvents.
Because global_id is forced to be part of the wire interface and
specifics, it is also conveniently part of the interface to the
UserEventService.

However, while this global_id is seemingly sacred to sync, it comes
from simply the timestmap of the navigation entry. And if a page is
reloaded, this value can change. A problem arises when a UserEvent
references a navigation by a global_id that was updated and the old
value is never going to reach the sync server.

To fix we maintain two sets of mappings, one that allows us to map old
global_ids to new global_id, and one that allows us to efficiently look
up uncommitted UserEvents by global_id, so that if we get either a new
global_id or a new UserEvent we can quickly fix the global_id.

BUG=747621
==========

[commit-bot: I haz the power, 2017-07-22 00:28:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-22 00:28:21]

Dry run: This issue passed the CQ dry run.

[skym, 2017-07-22 00:28:52]

The CQ bit was checked by skym@chromium.org

[skym, 2017-07-22 00:28:53]

The patchset sent to the CQ was uploaded after l-g-t-m from pnoland@chromium.org
Link to the patchset: https://codereview.chromium.org/2958303002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-07-22 00:28:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-22 01:32:48]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1500683332897970,
"parent_rev": "9f2dfb6d3b7e631f084520f823a18186fe2f3ba3", "commit_rev":
"800715d5937534637df6ac6cc1c9ea9424b6bae0"}

[commit-bot: I haz the power, 2017-07-22 01:33:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1500683332897970,
"parent_rev": "6fb7ab1ac3ae602cfada4a87d018def01f24d1ca", "commit_rev":
"a58a1f014663671847efd294313b99fe71e1a6bf"}

[commit-bot: I haz the power, 2017-07-22 01:33:18]

Description was changed from

==========
[Sync] Maintain a global_id mapping to update UserEvents that references
navigations.

UserEvents are created and reference sessions data by global_id. It is
important that they travel over the wire in this format, as this is how
the sessions data is keyed into Footprints, and the sync server cannot
feasibly perform a persistent read before writing new UserEvents.
Because global_id is forced to be part of the wire interface and
specifics, it is also conveniently part of the interface to the
UserEventService.

However, while this global_id is seemingly sacred to sync, it comes
from simply the timestmap of the navigation entry. And if a page is
reloaded, this value can change. A problem arises when a UserEvent
references a navigation by a global_id that was updated and the old
value is never going to reach the sync server.

To fix we maintain two sets of mappings, one that allows us to map old
global_ids to new global_id, and one that allows us to efficiently look
up uncommitted UserEvents by global_id, so that if we get either a new
global_id or a new UserEvent we can quickly fix the global_id.

BUG=747621
==========

to

==========
[Sync] Maintain a global_id mapping to update UserEvents that references
navigations.

UserEvents are created and reference sessions data by global_id. It is
important that they travel over the wire in this format, as this is how
the sessions data is keyed into Footprints, and the sync server cannot
feasibly perform a persistent read before writing new UserEvents.
Because global_id is forced to be part of the wire interface and
specifics, it is also conveniently part of the interface to the
UserEventService.

However, while this global_id is seemingly sacred to sync, it comes
from simply the timestmap of the navigation entry. And if a page is
reloaded, this value can change. A problem arises when a UserEvent
references a navigation by a global_id that was updated and the old
value is never going to reach the sync server.

To fix we maintain two sets of mappings, one that allows us to map old
global_ids to new global_id, and one that allows us to efficiently look
up uncommitted UserEvents by global_id, so that if we get either a new
global_id or a new UserEvent we can quickly fix the global_id.

BUG=747621

Review-Url: https://codereview.chromium.org/2958303002
Cr-Commit-Position: refs/heads/master@{#488826}
Committed:
https://chromium.googlesource.com/chromium/src/+/a58a1f014663671847efd294313b...
==========

[commit-bot: I haz the power, 2017-07-22 01:33:19]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a58a1f014663671847efd294313b...

[vabr (Chromium), 2017-07-22 08:22:21]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2017-07-22 08:22:22]

This CL causes crash on iOS: https://crbug.com/747658

Please fix or revert at your earliest convenience.

Thanks!
Vaclav

https://codereview.chromium.org/2958303002/diff/100001/components/sync_sessio...
File components/sync_sessions/sessions_sync_manager.cc (right):

https://codereview.chromium.org/2958303002/diff/100001/components/sync_sessio...
components/sync_sessions/sessions_sync_manager.cc:596:
modified_tab->GetSerializedNavigationAtIndex(
This might be on the path of the crash in https://crbug.com/747658.

[skym, 2017-07-24 18:35:37]

Patchset #7 (id:120001) has been deleted