[Sync] Handle reassociation of tabs where the old tab is already mapped.

[Sync] Handle reassociation of tabs where the old tab is already mapped.

If ReassociateLocalTab is called with a tab node that is already mapped to a
tab, it's possible to wind up with the synced session tracker holding two
tab objects referring to the same tab id. This can lead to memory corruption.

This CL makes the tracker defensive against that scenario, and adds some more
data verification in the unit tests

BUG=714524, 639009
Review-Url: https://codereview.chromium.org/2856913007
Cr-Commit-Position: refs/heads/master@{#469553}
Committed: https://chromium.googlesource.com/chromium/src/+/783900c29c6242e65191a8951e12ce45c739571e

[Nicolas Zea, 2017-05-04 00:51:40]

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

[commit-bot: I haz the power, 2017-05-04 00:52:17]

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-05-04 02:33:32]

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

[commit-bot: I haz the power, 2017-05-04 02:33:33]

Dry run: This issue passed the CQ dry run.

[Nicolas Zea, 2017-05-04 18:15:28]

Description was changed from

==========
[Sync] Handle reassociation of tabs where the old tab is already mapped better.

If ReassociateLocalTab is called with a tab node that is already mapped to a
tab, it's possible to wind up with the synced session tracker holding two
tab objects referring to the same tab id. This can lead to memory corruption.

This CL makes the tracker defensive against that scenario, and adds some more
data verification in the unit tests

BUG=714524, 639009
==========

to

==========
[Sync] Handle reassociation of tabs where the old tab is already mapped.

If ReassociateLocalTab is called with a tab node that is already mapped to a
tab, it's possible to wind up with the synced session tracker holding two
tab objects referring to the same tab id. This can lead to memory corruption.

This CL makes the tracker defensive against that scenario, and adds some more
data verification in the unit tests

BUG=714524, 639009
==========

[Nicolas Zea, 2017-05-04 19:24:19]

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

[Nicolas Zea, 2017-05-04 19:24:20]

PTAL

[skym, 2017-05-04 20:07:21]

lgtm

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
File components/sync_sessions/synced_session_tracker.cc (right):

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker.cc:412: tab_ptr =
old_tab_iter->second;
Is it possible this assigns a nullptr? Cause the new code sure makes it seem
like this is possible. If it shouldn't happen, I like the old way better, of 

else {
  tab_ptr = GetTab(dklsgfjalds);
}

Could also throw a DCHECK up here. But if we are worried old_tab_iter->second
holds a nullptr, feel free to disregard.

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker.cc:426: for (auto&
window_iter_pair : GetSession(local_session_tag_)->windows) {
Sure would have been nice if things had parent pointers.

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker.cc:427: auto tab_iter =
std::find_if(
Did you consider remove_if?

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker.cc:428:
window_iter_pair.second->wrapped_window.tabs.begin(),
Did you consider assigning window_iter_pair.second->wrapped_window.tabs to a
temp variable?

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
File components/sync_sessions/synced_session_tracker.h (right):

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker.h:178: // overwritten.
Did you mean to newline so early here?

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
File components/sync_sessions/synced_session_tracker_unittest.cc (right):

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker_unittest.cc:53: auto sess_iter =
tracker_.synced_tab_map_.find(session_tag);
sess_iter is a confusing name. Maybe tab_map_iter?

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker_unittest.cc:54: if (sess_iter ==
tracker_.synced_tab_map_.end())
I think a ternary on the same line as declaration would be more clear here, even
though it'd span two lines. Or alternatively, don't re-set it to 0.

[Nicolas Zea, 2017-05-04 23:36:09]

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

[Nicolas Zea, 2017-05-04 23:36:15]

Comments addressed.

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
File components/sync_sessions/synced_session_tracker.cc (right):

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker.cc:412: tab_ptr =
old_tab_iter->second;
On 2017/05/04 20:07:20, skym wrote:
> Is it possible this assigns a nullptr? Cause the new code sure makes it seem
> like this is possible. If it shouldn't happen, I like the old way better, of 
> 
> else {
>   tab_ptr = GetTab(dklsgfjalds);
> }
> 
> Could also throw a DCHECK up here. But if we are worried old_tab_iter->second
> holds a nullptr, feel free to disregard.

Added DCHECK

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker.cc:426: for (auto&
window_iter_pair : GetSession(local_session_tag_)->windows) {
On 2017/05/04 20:07:20, skym wrote:
> Sure would have been nice if things had parent pointers.

Yeah, this really motivates moving away from the SessionTab/SessionWindow
objects and onto our own entirely new structures.

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker.cc:427: auto tab_iter =
std::find_if(
On 2017/05/04 20:07:20, skym wrote:
> Did you consider remove_if?

I think that would prevent me from breaking out?

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker.cc:428:
window_iter_pair.second->wrapped_window.tabs.begin(),
On 2017/05/04 20:07:20, skym wrote:
> Did you consider assigning window_iter_pair.second->wrapped_window.tabs to a
> temp variable?

Done.

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
File components/sync_sessions/synced_session_tracker.h (right):

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker.h:178: // overwritten.
On 2017/05/04 20:07:20, skym wrote:
> Did you mean to newline so early here?

Done.

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
File components/sync_sessions/synced_session_tracker_unittest.cc (right):

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker_unittest.cc:53: auto sess_iter =
tracker_.synced_tab_map_.find(session_tag);
On 2017/05/04 20:07:20, skym wrote:
> sess_iter is a confusing name. Maybe tab_map_iter?

Done.

https://codereview.chromium.org/2856913007/diff/1/components/sync_sessions/sy...
components/sync_sessions/synced_session_tracker_unittest.cc:54: if (sess_iter ==
tracker_.synced_tab_map_.end())
On 2017/05/04 20:07:20, skym wrote:
> I think a ternary on the same line as declaration would be more clear here,
even
> though it'd span two lines. Or alternatively, don't re-set it to 0.

Done.

[Nicolas Zea, 2017-05-04 23:36:35]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2017-05-04 23:36:35]

The CQ bit was unchecked by zea@chromium.org

[Nicolas Zea, 2017-05-04 23:36:35]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2856913007/#ps20001
(title: "Address comments")

[commit-bot: I haz the power, 2017-05-04 23:37:06]

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-05-05 00:46:51]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493940995234760,
"parent_rev": "a367e903ff405e49e4a22b0512b8b989be08838a", "commit_rev":
"783900c29c6242e65191a8951e12ce45c739571e"}

[commit-bot: I haz the power, 2017-05-05 00:47:06]

Description was changed from

==========
[Sync] Handle reassociation of tabs where the old tab is already mapped.

If ReassociateLocalTab is called with a tab node that is already mapped to a
tab, it's possible to wind up with the synced session tracker holding two
tab objects referring to the same tab id. This can lead to memory corruption.

This CL makes the tracker defensive against that scenario, and adds some more
data verification in the unit tests

BUG=714524, 639009
==========

to

==========
[Sync] Handle reassociation of tabs where the old tab is already mapped.

If ReassociateLocalTab is called with a tab node that is already mapped to a
tab, it's possible to wind up with the synced session tracker holding two
tab objects referring to the same tab id. This can lead to memory corruption.

This CL makes the tracker defensive against that scenario, and adds some more
data verification in the unit tests

BUG=714524, 639009

Review-Url: https://codereview.chromium.org/2856913007
Cr-Commit-Position: refs/heads/master@{#469553}
Committed:
https://chromium.googlesource.com/chromium/src/+/783900c29c6242e65191a8951e12...
==========

[commit-bot: I haz the power, 2017-05-05 00:47:07]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/783900c29c6242e65191a8951e12...