[Sync] Moved tab_node_id tracking to session object and improved foreign session garbage collection.

[Sync] Moved tab_node_id tracking to session object and improved foreign session
garbage collection.

Orphaned tabs and empty header nodes were previously completely missed by
garbage collection, leading to a bloat of sync data. Additionally, orphan tabs
that were added after the header during merge were never cleaned up by the
tracker, using memory. These issues should all be resolved, but in order to
correctly perform garbage collection of cleaned up session data, the location
of tab_node_ids were moved from the SessionTabWrapper to the SyncedSession so
that they can be tracked through cleanup calls. Should expect a minor memory
improvement for users syncing session data.

BUG=599548
Committed: https://crrev.com/199c7fdea8b1652b9ff0d17cdd2989476b63cf5f
Cr-Commit-Position: refs/heads/master@{#387391}

[skym, 2016-04-11 23:22:09]

Description was changed from

==========
[Sync] Stopped removing orphaned remote tabs locally so garbage collection could
correctly delete all of them.

BUG=599548
==========

to

==========
[Sync] Stopped removing orphaned remote tabs locally so garbage collection could
correctly delete all of them.

This should allow local clients to correctly track and delete stale foreign
sessions. Orphaned tabs and empty header nodes were previously completely
missed. Some of the orphaned tabs, depending on if they were in the merged sync
data before or after the header, were getting cleaned up and forgotten about,
and the approach to cleanup remote sessions had to stop. This means the memory
overhead of the sessions data type may increase for some users, as we no longer
drop some orphaned tabs. We will however correctly cleanup stale data, which
will provide a memory win some users, especially in cases where most tabs
appeared after the header during merge.

BUG=599548
==========

[skym, 2016-04-11 23:22:09]

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

[skym, 2016-04-11 23:34:10]

Patchset #1 (id:1) has been deleted

[skym, 2016-04-11 23:35:37]

PTAL

[skym, 2016-04-12 14:33:19]

Description was changed from

==========
[Sync] Stopped removing orphaned remote tabs locally so garbage collection could
correctly delete all of them.

This should allow local clients to correctly track and delete stale foreign
sessions. Orphaned tabs and empty header nodes were previously completely
missed. Some of the orphaned tabs, depending on if they were in the merged sync
data before or after the header, were getting cleaned up and forgotten about,
and the approach to cleanup remote sessions had to stop. This means the memory
overhead of the sessions data type may increase for some users, as we no longer
drop some orphaned tabs. We will however correctly cleanup stale data, which
will provide a memory win some users, especially in cases where most tabs
appeared after the header during merge.

BUG=599548
==========

to

==========
[Sync] Stopped removing orphaned remote tabs locally so garbage collection could
correctly delete all of them.

This should allow local clients to correctly track and delete stale foreign
sessions. Orphaned tabs and empty header nodes were previously completely
missed. Some of the orphaned tabs, depending on if they were in the merged sync
data before or after the header, were getting cleaned up and forgotten about,
and the approach to cleanup remote sessions had to stop. This means the memory
overhead of the sessions data type may increase for some users, as we no longer
drop some orphaned tabs. We will however correctly cleanup stale data, which
will provide a memory win some users, especially in cases where most tabs
appeared after the header during merge.

BUG=599548
==========

[skym, 2016-04-12 14:37:23]

I realized this morning that my changes don't quite work. While we respond to
foreign tab deletions by removing the tab node id from the tab referenced by the
current tab id, that doesn't mean another tab with a different tab id isn't
holding onto the same tab node id. If we then tried to garbage collect 14 days
later we would delete the already deleted tab node id and the processor would
blow up on us.

I think a better solution is to push the tracking of tab node ids into the
session object, and actually run cleanup like we used to.

[skym, 2016-04-12 16:48:52]

Description was changed from

==========
[Sync] Stopped removing orphaned remote tabs locally so garbage collection could
correctly delete all of them.

This should allow local clients to correctly track and delete stale foreign
sessions. Orphaned tabs and empty header nodes were previously completely
missed. Some of the orphaned tabs, depending on if they were in the merged sync
data before or after the header, were getting cleaned up and forgotten about,
and the approach to cleanup remote sessions had to stop. This means the memory
overhead of the sessions data type may increase for some users, as we no longer
drop some orphaned tabs. We will however correctly cleanup stale data, which
will provide a memory win some users, especially in cases where most tabs
appeared after the header during merge.

BUG=599548
==========

to

==========
[Sync] Moved tab_node_id tracking to session object and improved foreign session
garbage collection.

Orphaned tabs and empty header nodes were previously completely missed by
garbage collection, leading to a bloat of sync data. Additionally, orphan tabs
that were added after the header during merge were never cleaned up by the
tracker, using memory. These issues should all be resolved, but in order to
correctly perform garbage collection of cleaned up session data, the location
of tab_node_ids were moved from the SessionTabWrapper to the SyncedSession so
that they can be tracked through cleanup calls. Should expect a minor memory
improvement for users syncing session data.

BUG=599548
==========

[skym, 2016-04-12 16:50:58]

Fixed the issue with reused tab_node_ids potentially being double deleted. By
moving tab_node_ids from SessionTabWrapper to SyncedSession, we're now able to
resume aggressively cleaning up foreign sessions, which avoids the memory
regression from previous changes. PTAL!

[Nicolas Zea, 2016-04-12 20:38:04]

I started some of these comments before the rewrite, so a few may be stale

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

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:519: current_machine_tag() !=
session.session_tag()) {
the current_machine_tag condition isn't necessary here right? (given the
original if statement above)

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:521: // being processes before
or after the parent was updated to no longer
being processes -> being processed

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:522: // reference us, or even
more extreme, the parent has been deleted as
"reference us, or even" -> "reference the tab. Or, even"

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:672: // to garbage collection
this session. We never do granular garbage
to garbage collection -> to garbage collect (or to do garbage collection)

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1050: if (!changes.empty()) {
nit: rest of file has (I think) been leaving curly braces off for single line if
bodies

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

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:31: // To be presentable it
means you must have windows with tabs with syncable
"To be presentable it means you must have windows with tabs with syncable
content" -> "Presentable means |foreign_session| must have syncable content"

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:132: if (tab_map_iter ==
synced_tab_map_.end()) {
nit: single line if body

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
File components/sync_sessions/synced_session_tracker.h (right):

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:49: bool presentable) const;
readability nit: prefer defining an enum and using that here rather than passing
a bool.
enum SessionLookup {
  RAW,  /// ....
  PRESENTABLE,  // Have one window with at least one tab with syncable content,
}

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:96: // find the given tab, but
isn't the primary identifier in the case, since
"in the case" -> "in this case"

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:97: // there may be multiple
tabs shadowing eachother for the given |tab_id|. In
eachother -> each other

Also, it's not clear ot me what you mean when you say multiple tabs may be
shadowing each other for the given |tab_id|?

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:99: // this method will simply
remove the given |tab_node_id|. If the tab is
will remove |tab_node_id| from where?

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:223: // clients lifetime.
I find this paragraph pretty tough to parse. Could you provide a more concrete
example?

https://codereview.chromium.org/1877083002/diff/40001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/session_sync_test_helper.h (right):

https://codereview.chromium.org/1877083002/diff/40001/chrome/browser/sync/glu...
chrome/browser/sync/glue/session_sync_test_helper.h:45: void
BuildTabSpecifics(const std::string& tag,
This entire file should really have comments, but at least could we add some
distinguishing this method from the one above?

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
File components/sync_sessions/synced_session.h (right):

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
components/sync_sessions/synced_session.h:63: // All of the known to be valid
tab node ids associated with this session.
Why mention "known to be valid"?

Also "stored in the header" makes it sound like this data somehow gets taken and
pushed into the header node. Similarly, "assembled for all tabs" makes it sound
like the tabs are themselves using this data (rather than this data coming from
the tab nodes). Consider rephrasing?

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
File components/sync_sessions/synced_session_tracker.cc (right):

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:125:
tab_node_ids->erase(TabNodePool::kInvalidTabNodeID);
When would this happen?

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:157: // if the parent session
isn't going to delete in it's destructor.
"isn't going to delete" delete what?

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
File components/sync_sessions/synced_session_tracker.h (left):

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:94: // See
ResetSessionTracking(...).
Should this remain?

[skym, 2016-04-12 22:05:39]

Patchset #4 (id:80001) has been deleted

[skym, 2016-04-12 22:12:14]

Updates for zea.

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

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:519: current_machine_tag() !=
session.session_tag()) {
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> the current_machine_tag condition isn't necessary here right? (given the
> original if statement above)

Done.

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:521: // being processes before
or after the parent was updated to no longer
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> being processes -> being processed

Done.

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:522: // reference us, or even
more extreme, the parent has been deleted as
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> "reference us, or even" -> "reference the tab. Or, even"

Done.

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:672: // to garbage collection
this session. We never do granular garbage
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> to garbage collection -> to garbage collect (or to do garbage collection)

Comment is different now.

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/sessions_sync_manager.cc:1050: if (!changes.empty()) {
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> nit: rest of file has (I think) been leaving curly braces off for single line
if
> bodies

Done.

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

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:31: // To be presentable it
means you must have windows with tabs with syncable
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> "To be presentable it means you must have windows with tabs with syncable
> content" -> "Presentable means |foreign_session| must have syncable content"

Done.

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:132: if (tab_map_iter ==
synced_tab_map_.end()) {
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> nit: single line if body

Method has changed significantly.

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
File components/sync_sessions/synced_session_tracker.h (right):

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:49: bool presentable) const;
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> readability nit: prefer defining an enum and using that here rather than
passing
> a bool.
> enum SessionLookup {
>   RAW,  /// ....
>   PRESENTABLE,  // Have one window with at least one tab with syncable
content,
> }
>   

Done.

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:96: // find the given tab, but
isn't the primary identifier in the case, since
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> "in the case" -> "in this case"

Comment is different now.

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:97: // there may be multiple
tabs shadowing eachother for the given |tab_id|. In
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> eachother -> each other
> 
> Also, it's not clear ot me what you mean when you say multiple tabs may be
> shadowing each other for the given |tab_id|?

Comment is different now.

I've mostly stopped using the term shadowing, but we still have the same thing
happening. tab_ids are not stable across restarts, and it's possible we may have
multiple sync tab objects, each with a different tab_node_id value, that all
claim to have the same tab_id. SessionsSyncManager uses the data from the one
with the most recent modification time. And so all of losers here, the tabs with
the older modification times, are "shadowing" the winner.

The reason that we care, is that if we decide to garbage collect/delete this
foreign session, we want to issue deletes for all of the tab node ids, even ones
that the SessionsSyncManager ignored before they had old modification times and
the same tab id as other tabs.

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:99: // this method will simply
remove the given |tab_node_id|. If the tab is
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> will remove |tab_node_id| from where?

In this patch set it was from the SessionTabWrapper but now I've moved it to the
SyncedSession. Updated the current comment to make this clear.

https://codereview.chromium.org/1877083002/diff/20001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:223: // clients lifetime.
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> I find this paragraph pretty tough to parse. Could you provide a more concrete
> example?

Moved this to SyncedSession. Added a comment pointing towards
SyncedSessionTracker::GetTabImpl for concrete example

https://codereview.chromium.org/1877083002/diff/40001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/session_sync_test_helper.h (right):

https://codereview.chromium.org/1877083002/diff/40001/chrome/browser/sync/glu...
chrome/browser/sync/glue/session_sync_test_helper.h:45: void
BuildTabSpecifics(const std::string& tag,
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> This entire file should really have comments, but at least could we add some
> distinguishing this method from the one above?

Done.

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
File components/sync_sessions/synced_session.h (right):

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
components/sync_sessions/synced_session.h:63: // All of the known to be valid
tab node ids associated with this session.
On 2016/04/12 20:38:03, Nicolas Zea wrote:
> Why mention "known to be valid"?
> 
> Also "stored in the header" makes it sound like this data somehow gets taken
and
> pushed into the header node. Similarly, "assembled for all tabs" makes it
sound
> like the tabs are themselves using this data (rather than this data coming
from
> the tab nodes). Consider rephrasing?

Yeah, I have no idea what I was thinking when I typed "known to be valid",
because I'm filtering out invalid tabs in LookupTabNodeIds(...).

Done.

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
File components/sync_sessions/synced_session_tracker.cc (right):

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:125:
tab_node_ids->erase(TabNodePool::kInvalidTabNodeID);
On 2016/04/12 20:38:04, Nicolas Zea wrote:
> When would this happen?

So I'm not sure if this is needed, partly because I don't really understand when
exactly the invalid ids are used. I don't think it should ever happen,
especially since we typically only ask for node ids for foreign sessions. The
call that my understanding is really shaky on is
https://code.google.com/p/chromium/codesearch#chromium/src/components/sync_se...
. I have not spent the time to understand what's going on there.

However, this check existed before my changes, it was part of the 'if' statement
on line 121 before my changes, so I carried it over to be safe. If we did ever
return the invalid tab node id, this data type would encounter an unrecoverable
error in the processor, and a bad time would be had by all.

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.cc:157: // if the parent session
isn't going to delete in it's destructor.
On 2016/04/12 20:38:04, Nicolas Zea wrote:
> "isn't going to delete" delete what?

Orphaned SessionTab and SessionWindow objects. Update comment.

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
File components/sync_sessions/synced_session_tracker.h (left):

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
components/sync_sessions/synced_session_tracker.h:94: // See
ResetSessionTracking(...).
On 2016/04/12 20:38:04, Nicolas Zea wrote:
> Should this remain?

Yes. Done.

[Nicolas Zea, 2016-04-14 18:17:25]

LGTM, nice job figuring this out!

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
File components/sync_sessions/synced_session.h (right):

https://codereview.chromium.org/1877083002/diff/60001/components/sync_session...
components/sync_sessions/synced_session.h:63: // All of the known to be valid
tab node ids associated with this session.
On 2016/04/12 22:12:14, skym wrote:
> On 2016/04/12 20:38:03, Nicolas Zea wrote:
> > Why mention "known to be valid"?
> > 
> > Also "stored in the header" makes it sound like this data somehow gets taken
> and
> > pushed into the header node. Similarly, "assembled for all tabs" makes it
> sound
> > like the tabs are themselves using this data (rather than this data coming
> from
> > the tab nodes). Consider rephrasing?
> 
> Yeah, I have no idea what I was thinking when I typed "known to be valid",
> because I'm filtering out invalid tabs in LookupTabNodeIds(...).
> 
> Done.

Much clearer, thanks!

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

https://codereview.chromium.org/1877083002/diff/100001/components/sync_sessio...
components/sync_sessions/synced_session_tracker.cc:157: // that is currently
parents.
that is currently parents -> that it is currently the parent of

[skym, 2016-04-14 18:43:33]

Fixing comment for zea.

[skym, 2016-04-14 18:44:34]

The CQ bit was checked by skym@chromium.org

[skym, 2016-04-14 18:44:35]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1877083002/#ps120001
(title: "Fixing comment for zea.")

[commit-bot: I haz the power, 2016-04-14 18:44:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877083002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877083002/120001

[commit-bot: I haz the power, 2016-04-14 19:26:54]

Description was changed from

==========
[Sync] Moved tab_node_id tracking to session object and improved foreign session
garbage collection.

Orphaned tabs and empty header nodes were previously completely missed by
garbage collection, leading to a bloat of sync data. Additionally, orphan tabs
that were added after the header during merge were never cleaned up by the
tracker, using memory. These issues should all be resolved, but in order to
correctly perform garbage collection of cleaned up session data, the location
of tab_node_ids were moved from the SessionTabWrapper to the SyncedSession so
that they can be tracked through cleanup calls. Should expect a minor memory
improvement for users syncing session data.

BUG=599548
==========

to

==========
[Sync] Moved tab_node_id tracking to session object and improved foreign session
garbage collection.

Orphaned tabs and empty header nodes were previously completely missed by
garbage collection, leading to a bloat of sync data. Additionally, orphan tabs
that were added after the header during merge were never cleaned up by the
tracker, using memory. These issues should all be resolved, but in order to
correctly perform garbage collection of cleaned up session data, the location
of tab_node_ids were moved from the SessionTabWrapper to the SyncedSession so
that they can be tracked through cleanup calls. Should expect a minor memory
improvement for users syncing session data.

BUG=599548
==========

[commit-bot: I haz the power, 2016-04-14 19:26:56]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-04-14 19:28:21]

Description was changed from

==========
[Sync] Moved tab_node_id tracking to session object and improved foreign session
garbage collection.

Orphaned tabs and empty header nodes were previously completely missed by
garbage collection, leading to a bloat of sync data. Additionally, orphan tabs
that were added after the header during merge were never cleaned up by the
tracker, using memory. These issues should all be resolved, but in order to
correctly perform garbage collection of cleaned up session data, the location
of tab_node_ids were moved from the SessionTabWrapper to the SyncedSession so
that they can be tracked through cleanup calls. Should expect a minor memory
improvement for users syncing session data.

BUG=599548
==========

to

==========
[Sync] Moved tab_node_id tracking to session object and improved foreign session
garbage collection.

Orphaned tabs and empty header nodes were previously completely missed by
garbage collection, leading to a bloat of sync data. Additionally, orphan tabs
that were added after the header during merge were never cleaned up by the
tracker, using memory. These issues should all be resolved, but in order to
correctly perform garbage collection of cleaned up session data, the location
of tab_node_ids were moved from the SessionTabWrapper to the SyncedSession so
that they can be tracked through cleanup calls. Should expect a minor memory
improvement for users syncing session data.

BUG=599548

Committed: https://crrev.com/199c7fdea8b1652b9ff0d17cdd2989476b63cf5f
Cr-Commit-Position: refs/heads/master@{#387391}
==========

[commit-bot: I haz the power, 2016-04-14 19:28:22]

Patchset 5 (id:??) landed as
https://crrev.com/199c7fdea8b1652b9ff0d17cdd2989476b63cf5f
Cr-Commit-Position: refs/heads/master@{#387391}