[Sync] Act defensively when parent lookups fail on commit.

[Sync] Act defensively when parent lookups fail on commit.

When committing a bookmark (a hierarchical type), we follow parent
links going upwards making sure that we also include all parents that
are so far client side only. We will ultimately reverse this list so
that we ensure parents always reach the server before children.

What happened in the crash (see bug) this CL is trying to fix is that
one of the parent look ups failed. Someone's parent id was a client id,
and  the directory didn't have a copy of it. In a simple world, this
wouldn't happen, the bookmarks model should have sent the parent
bookmark along (or even before) the child bookmark.

Our working assumption is there's some race somewhere involving
potentially multiple clients doing weird things like renaming, moving,
and deleting folders, and re-associating. Somehow a constraint is
broken, and on commit we have a child without a parent. Investigation
into the root cause of this did not turn up anything easily, so instead
we're changing the code to simply be more defensive and not crash in
this circumstance. It would not be safe to send a child without a
parent to the server, so our reaction is to skip committing the
orphaned bookmarks. There are already several other cases that take the
approach of not committing hierarchical things that are in a somewhat
bad state, so this approach is somewhat consistent.

The long term goal is to have all Bookmarks (the only hierarchical
type) logic converted over to a USS implementation, and all of this
code will be obsolete in the not terribly distant future anyways.

BUG=711381
Review-Url: https://codereview.chromium.org/2826503002
Cr-Commit-Position: refs/heads/master@{#465033}
Committed: https://chromium.googlesource.com/chromium/src/+/85ee7d9d45955be82e64b198a5b8901a16ea371a

[skym, 2017-04-17 18:05:46]

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

[commit-bot: I haz the power, 2017-04-17 18:06:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-04-17 18:15:47]

skym@chromium.org changed reviewers:
+ pavely@chromium.org

[skym, 2017-04-17 18:15:48]

PTAL

[commit-bot: I haz the power, 2017-04-17 18:57:24]

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

[commit-bot: I haz the power, 2017-04-17 18:57:25]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-04-17 20:40:14]

lgtm
Could you add a CL description explaining that:
- The issue is that committing bookmark requires that its parent is available.
When it is not, the code CHECKs.
- We don't know exactly why this happens and want client to skip such entries
instead of crashing.

[skym, 2017-04-17 21:47:46]

Description was changed from

==========
[Sync] Act defensively when parent lookups fail on commit.

BUG=711381
==========

to

==========
[Sync] Act defensively when parent lookups fail on commit.

When committing a bookmark (a hierarchical type), we follow parent
links going upwards making sure that we also include all parents that
are so far client side only. We will ultimately reverse this list so
that we ensure parents always reach the server before children.

What happened in the crash (see bug) this CL is trying to fix is that
one of the parent look ups failed. Someone's parent id was a client id,
and  the directory didn't have a copy of it. In a simple world, this
wouldn't happen, the bookmarks model should have sent the parent
bookmark along (or even before) the child bookmark.

Our working assumption is there's some race somewhere involving
potentially multiple clients doing weird things like renaming, moving,
and deleting folders, and re-associating. Somehow a constraint is
broken, and on commit we have a child without a parent. Investigation
into the root cause of this did not turn up anything easily, so instead
we're changing the code to simply be more defensive and not crash in
this circumstance.

The long term goal is to have all Bookmarks (the only hierarchical
type) logic converted over to a USS implementation, and all of this
code will be obsolete in the not terribly distant future anyways.

BUG=711381
==========

[skym, 2017-04-17 21:49:59]

Description was changed from

==========
[Sync] Act defensively when parent lookups fail on commit.

When committing a bookmark (a hierarchical type), we follow parent
links going upwards making sure that we also include all parents that
are so far client side only. We will ultimately reverse this list so
that we ensure parents always reach the server before children.

What happened in the crash (see bug) this CL is trying to fix is that
one of the parent look ups failed. Someone's parent id was a client id,
and  the directory didn't have a copy of it. In a simple world, this
wouldn't happen, the bookmarks model should have sent the parent
bookmark along (or even before) the child bookmark.

Our working assumption is there's some race somewhere involving
potentially multiple clients doing weird things like renaming, moving,
and deleting folders, and re-associating. Somehow a constraint is
broken, and on commit we have a child without a parent. Investigation
into the root cause of this did not turn up anything easily, so instead
we're changing the code to simply be more defensive and not crash in
this circumstance.

The long term goal is to have all Bookmarks (the only hierarchical
type) logic converted over to a USS implementation, and all of this
code will be obsolete in the not terribly distant future anyways.

BUG=711381
==========

to

==========
[Sync] Act defensively when parent lookups fail on commit.

When committing a bookmark (a hierarchical type), we follow parent
links going upwards making sure that we also include all parents that
are so far client side only. We will ultimately reverse this list so
that we ensure parents always reach the server before children.

What happened in the crash (see bug) this CL is trying to fix is that
one of the parent look ups failed. Someone's parent id was a client id,
and  the directory didn't have a copy of it. In a simple world, this
wouldn't happen, the bookmarks model should have sent the parent
bookmark along (or even before) the child bookmark.

Our working assumption is there's some race somewhere involving
potentially multiple clients doing weird things like renaming, moving,
and deleting folders, and re-associating. Somehow a constraint is
broken, and on commit we have a child without a parent. Investigation
into the root cause of this did not turn up anything easily, so instead
we're changing the code to simply be more defensive and not crash in
this circumstance. It would not be safe to send a child without a
parent to the server, so our reaction is to skip committing the
orphaned bookmarks. There are already several other cases that take the
approach of not committing hierarchical things that are in a somewhat
bad state, so this approach is somewhat consistent.


The long term goal is to have all Bookmarks (the only hierarchical
type) logic converted over to a USS implementation, and all of this
code will be obsolete in the not terribly distant future anyways.

BUG=711381
==========

[skym, 2017-04-17 21:50:11]

Description was changed from

==========
[Sync] Act defensively when parent lookups fail on commit.

When committing a bookmark (a hierarchical type), we follow parent
links going upwards making sure that we also include all parents that
are so far client side only. We will ultimately reverse this list so
that we ensure parents always reach the server before children.

What happened in the crash (see bug) this CL is trying to fix is that
one of the parent look ups failed. Someone's parent id was a client id,
and  the directory didn't have a copy of it. In a simple world, this
wouldn't happen, the bookmarks model should have sent the parent
bookmark along (or even before) the child bookmark.

Our working assumption is there's some race somewhere involving
potentially multiple clients doing weird things like renaming, moving,
and deleting folders, and re-associating. Somehow a constraint is
broken, and on commit we have a child without a parent. Investigation
into the root cause of this did not turn up anything easily, so instead
we're changing the code to simply be more defensive and not crash in
this circumstance. It would not be safe to send a child without a
parent to the server, so our reaction is to skip committing the
orphaned bookmarks. There are already several other cases that take the
approach of not committing hierarchical things that are in a somewhat
bad state, so this approach is somewhat consistent.


The long term goal is to have all Bookmarks (the only hierarchical
type) logic converted over to a USS implementation, and all of this
code will be obsolete in the not terribly distant future anyways.

BUG=711381
==========

to

==========
[Sync] Act defensively when parent lookups fail on commit.

When committing a bookmark (a hierarchical type), we follow parent
links going upwards making sure that we also include all parents that
are so far client side only. We will ultimately reverse this list so
that we ensure parents always reach the server before children.

What happened in the crash (see bug) this CL is trying to fix is that
one of the parent look ups failed. Someone's parent id was a client id,
and  the directory didn't have a copy of it. In a simple world, this
wouldn't happen, the bookmarks model should have sent the parent
bookmark along (or even before) the child bookmark.

Our working assumption is there's some race somewhere involving
potentially multiple clients doing weird things like renaming, moving,
and deleting folders, and re-associating. Somehow a constraint is
broken, and on commit we have a child without a parent. Investigation
into the root cause of this did not turn up anything easily, so instead
we're changing the code to simply be more defensive and not crash in
this circumstance. It would not be safe to send a child without a
parent to the server, so our reaction is to skip committing the
orphaned bookmarks. There are already several other cases that take the
approach of not committing hierarchical things that are in a somewhat
bad state, so this approach is somewhat consistent.

The long term goal is to have all Bookmarks (the only hierarchical
type) logic converted over to a USS implementation, and all of this
code will be obsolete in the not terribly distant future anyways.

BUG=711381
==========

[skym, 2017-04-17 21:50:16]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2017-04-17 21:50:33]

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-04-17 21:54:26]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1492465816571240, "parent_rev":
"e111e380e8f4178c4a1907c4fa6b4408a22800bc", "commit_rev":
"85ee7d9d45955be82e64b198a5b8901a16ea371a"}

[commit-bot: I haz the power, 2017-04-17 21:54:38]

Description was changed from

==========
[Sync] Act defensively when parent lookups fail on commit.

When committing a bookmark (a hierarchical type), we follow parent
links going upwards making sure that we also include all parents that
are so far client side only. We will ultimately reverse this list so
that we ensure parents always reach the server before children.

What happened in the crash (see bug) this CL is trying to fix is that
one of the parent look ups failed. Someone's parent id was a client id,
and  the directory didn't have a copy of it. In a simple world, this
wouldn't happen, the bookmarks model should have sent the parent
bookmark along (or even before) the child bookmark.

Our working assumption is there's some race somewhere involving
potentially multiple clients doing weird things like renaming, moving,
and deleting folders, and re-associating. Somehow a constraint is
broken, and on commit we have a child without a parent. Investigation
into the root cause of this did not turn up anything easily, so instead
we're changing the code to simply be more defensive and not crash in
this circumstance. It would not be safe to send a child without a
parent to the server, so our reaction is to skip committing the
orphaned bookmarks. There are already several other cases that take the
approach of not committing hierarchical things that are in a somewhat
bad state, so this approach is somewhat consistent.

The long term goal is to have all Bookmarks (the only hierarchical
type) logic converted over to a USS implementation, and all of this
code will be obsolete in the not terribly distant future anyways.

BUG=711381
==========

to

==========
[Sync] Act defensively when parent lookups fail on commit.

When committing a bookmark (a hierarchical type), we follow parent
links going upwards making sure that we also include all parents that
are so far client side only. We will ultimately reverse this list so
that we ensure parents always reach the server before children.

What happened in the crash (see bug) this CL is trying to fix is that
one of the parent look ups failed. Someone's parent id was a client id,
and  the directory didn't have a copy of it. In a simple world, this
wouldn't happen, the bookmarks model should have sent the parent
bookmark along (or even before) the child bookmark.

Our working assumption is there's some race somewhere involving
potentially multiple clients doing weird things like renaming, moving,
and deleting folders, and re-associating. Somehow a constraint is
broken, and on commit we have a child without a parent. Investigation
into the root cause of this did not turn up anything easily, so instead
we're changing the code to simply be more defensive and not crash in
this circumstance. It would not be safe to send a child without a
parent to the server, so our reaction is to skip committing the
orphaned bookmarks. There are already several other cases that take the
approach of not committing hierarchical things that are in a somewhat
bad state, so this approach is somewhat consistent.

The long term goal is to have all Bookmarks (the only hierarchical
type) logic converted over to a USS implementation, and all of this
code will be obsolete in the not terribly distant future anyways.

BUG=711381

Review-Url: https://codereview.chromium.org/2826503002
Cr-Commit-Position: refs/heads/master@{#465033}
Committed:
https://chromium.googlesource.com/chromium/src/+/85ee7d9d45955be82e64b198a5b8...
==========

[commit-bot: I haz the power, 2017-04-17 21:54:40]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/85ee7d9d45955be82e64b198a5b8...