Reland v4 of Session refactor

Reland v4 of Session refactor

Previous review: https://codereview.chromium.org/2651583006/

Changes from previous review:
- Deletes sync tab nodes with invalid tab ids
- Adds support for gracefully ignoring tabs with the same tab id. It's
unclear how this might happen, but we now log an error and silently
ignore the tab, instead of crashing.
- Adds some more checks to tests

TEST=SessionsSyncManagerTest.DuplicateTabIdFromNative, SessionsSyncManagerTest.MergeDeletesCorruptTabNodeId
BUG=639009
Review-Url: https://codereview.chromium.org/2683263002
Cr-Commit-Position: refs/heads/master@{#449677}
Committed: https://chromium.googlesource.com/chromium/src/+/9e83363531256aad94ed2bec676bbca8d715fbe1

[Nicolas Zea, 2017-02-09 19:49:40]

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

[commit-bot: I haz the power, 2017-02-09 19:50:30]

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-02-09 20:53:52]

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

[commit-bot: I haz the power, 2017-02-09 20:53:53]

Dry run: This issue passed the CQ dry run.

[Nicolas Zea, 2017-02-09 21:18:46]

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

[Nicolas Zea, 2017-02-09 21:18:46]

+Sky and Patrick.

The first patchset is just a reland of the previous patch. The second patchset
contains the changes to gracefully handle issues with duplicate tab ids (see
crash b9000ad580000000 for an example of the crashes the previous patch
triggered).

[Nicolas Zea, 2017-02-09 23:16:04]

Actually, hold off on this for the moment. I think I may have identified another
situation where the CHECK that's being triggered can occur.

[Nicolas Zea, 2017-02-10 01:10:01]

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

[commit-bot: I haz the power, 2017-02-10 01:10:24]

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

[Nicolas Zea, 2017-02-10 01:10:52]

Description was changed from

==========
Reland v4 of Session refactor

Previous review: https://codereview.chromium.org/2651583006/

Changes from previous review:
Adds support for gracefully ignoring tabs with the same tab id. It's
unclear how this might happen, but we now log an error and silently
ignore the tab, instead of crashing.

TEST=SessionsSyncMagerTest.DuplicateTabIds
BUG=639009
==========

to

==========
Reland v4 of Session refactor

Previous review: https://codereview.chromium.org/2651583006/

Changes from previous review:
- Deletes sync tab nodes with invalid tab ids
- Adds support for gracefully ignoring tabs with the same tab id. It's
unclear how this might happen, but we now log an error and silently
ignore the tab, instead of crashing.
- Adds some more checks to tests

TEST=SessionsSyncMagerTest.DuplicateTabIds
BUG=639009
==========

[Nicolas Zea, 2017-02-10 01:11:17]

Okay, PTAL. I added logic to gracefully handle and delete tab nodes that may
have been created with invalid tab ids.

[commit-bot: I haz the power, 2017-02-10 02:06:42]

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

[commit-bot: I haz the power, 2017-02-10 02:06:43]

Dry run: This issue passed the CQ dry run.

[skym, 2017-02-10 16:49:32]

lgtm

[Patrick Noland, 2017-02-10 17:40:26]

What were the scenarios where tab nodes with invalid ids were created? Does
deleting them cause the loss of "real" data?

[Nicolas Zea, 2017-02-10 18:41:26]

Description was changed from

==========
Reland v4 of Session refactor

Previous review: https://codereview.chromium.org/2651583006/

Changes from previous review:
- Deletes sync tab nodes with invalid tab ids
- Adds support for gracefully ignoring tabs with the same tab id. It's
unclear how this might happen, but we now log an error and silently
ignore the tab, instead of crashing.
- Adds some more checks to tests

TEST=SessionsSyncMagerTest.DuplicateTabIds
BUG=639009
==========

to

==========
Reland v4 of Session refactor

Previous review: https://codereview.chromium.org/2651583006/

Changes from previous review:
- Deletes sync tab nodes with invalid tab ids
- Adds support for gracefully ignoring tabs with the same tab id. It's
unclear how this might happen, but we now log an error and silently
ignore the tab, instead of crashing.
- Adds some more checks to tests

TEST=SessionsSyncManagerTest.DuplicateTabIdFromNative,
SessionsSyncManagerTest.MergeDeletesCorruptTabNodeId
BUG=639009
==========

[Nicolas Zea, 2017-02-10 18:44:58]

In general, as long as you're only deleting your local data at startup, there's
no concern about deleting "real" data. The current truth is what's open, which
you're going to be rewriting anyways. The exception to that is when custom tabs
are open, in which case we're going to try to reassociate the non-custom tabs
with their previous sync node. But if the sync node didn't have a valid tab id,
it was corrupted and we shouldn't be reassociating with it anyway.

This is more a defensive measure than anything though. I haven't confirmed that
there were cases where we wrote invalid tab ids, but it's the only other case I
can think of that might lead to the logic for tracking tabs getting confused and
leading to the crashes seen.

[Nicolas Zea, 2017-02-10 18:45:18]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2017-02-10 18:46:00]

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

[Patrick Noland, 2017-02-10 18:46:16]

lgtm

[commit-bot: I haz the power, 2017-02-10 18:59:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486752318379230,
"parent_rev": "18beb504d09781e7da1f7b7ca03c943f495484d4", "commit_rev":
"9e83363531256aad94ed2bec676bbca8d715fbe1"}

[commit-bot: I haz the power, 2017-02-10 18:59:55]

Description was changed from

==========
Reland v4 of Session refactor

Previous review: https://codereview.chromium.org/2651583006/

Changes from previous review:
- Deletes sync tab nodes with invalid tab ids
- Adds support for gracefully ignoring tabs with the same tab id. It's
unclear how this might happen, but we now log an error and silently
ignore the tab, instead of crashing.
- Adds some more checks to tests

TEST=SessionsSyncManagerTest.DuplicateTabIdFromNative,
SessionsSyncManagerTest.MergeDeletesCorruptTabNodeId
BUG=639009
==========

to

==========
Reland v4 of Session refactor

Previous review: https://codereview.chromium.org/2651583006/

Changes from previous review:
- Deletes sync tab nodes with invalid tab ids
- Adds support for gracefully ignoring tabs with the same tab id. It's
unclear how this might happen, but we now log an error and silently
ignore the tab, instead of crashing.
- Adds some more checks to tests

TEST=SessionsSyncManagerTest.DuplicateTabIdFromNative,
SessionsSyncManagerTest.MergeDeletesCorruptTabNodeId
BUG=639009

Review-Url: https://codereview.chromium.org/2683263002
Cr-Commit-Position: refs/heads/master@{#449677}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e83363531256aad94ed2bec676b...
==========

[commit-bot: I haz the power, 2017-02-10 18:59:56]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9e83363531256aad94ed2bec676b...

[Nicolas Zea, 2017-02-14 02:13:47]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2694963002/ by zea@chromium.org.

The reason for reverting is: Still crashing. Likely due to sync nodes being
reused across windows not being handled gracefully..