[Sync] Fix Session's handling of windows.

[Sync] Fix Session's handling of windows.

We previously made some false assumptions about the order of window id's, and
freed memory for windows inappropriately. This would lead to memory corruption,
usually seen in the form of accesses to bad TabNavigation objects in the
session model associator. This patch completely rewrites how
we handle synced windows. Because there's no real order to windows, we now
store them in a map. In addition, because tabs can be moved between windows
and we aren't explicitly told which windows were closed or which tabs were
closed, we now have explicit tracking of the usage of windows and tabs, coupled
with a garbage collection phase that frees those tabs/windows that weren't
used during the tracking (see SyncedSessionTracker::ResetSessionTracking and
CleanupSession methods).

Due to the change in how we store windows, the integration tests and the
sessions ui had to be modified, but the real changes are within the session
model associator and the synced session tracker. Also, undisabled a test
that should be working.

BUG=94124, 81104
TEST=unit_tests; Opening windows with valid tabs, waiting for sync, closing
those windows.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103878

[Nicolas Zea, 2011-09-22 23:19:34]

also Yaron as FYI

[akalin, 2011-09-26 18:34:43]

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:645: size_t i, num_windows;
int num_windows = header.window_size();
...
for (int i = 0; i < num_windows; ++i) {
 ...

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/synced_session.h (right):

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session.h:42: SyncedWindowMap windows;
Add a comment saying that the SessionWindow*s are owned

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session.h:43: };
DISALLOW_COPY_AND_ASSIGN()?  Seems like things will break if you allow this to
be copied (double-frees of SessionWindow*, etc)

Alternatively, make SyncedWindowMap have linked_ptr<SessionWindow>

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/synced_session_tracker.h (right):

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session_tracker.h:123: typedef
std::pair<SessionTab*, bool> SessionTabPair;
prefer an explicit struct, otherwise it gets confusing (e.g., it->second.second)

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session_tracker.h:127: typedef
std::pair<SessionWindow*, bool> SessionWindowPair;
explicit struct

[Nicolas Zea, 2011-09-26 19:47:36]

Comments addressed, PTAL

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:645: size_t i, num_windows;
On 2011/09/26 18:34:44, akalin wrote:
> int num_windows = header.window_size();
> ...
> for (int i = 0; i < num_windows; ++i) {
>  ...

Done.

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/synced_session.h (right):

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session.h:42: SyncedWindowMap windows;
On 2011/09/26 18:34:44, akalin wrote:
> Add a comment saying that the SessionWindow*s are owned

Done.

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session.h:43: };
On 2011/09/26 18:34:44, akalin wrote:
> DISALLOW_COPY_AND_ASSIGN()?  Seems like things will break if you allow this to
> be copied (double-frees of SessionWindow*, etc)
> 
> Alternatively, make SyncedWindowMap have linked_ptr<SessionWindow>

Done.

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/synced_session_tracker.h (right):

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session_tracker.h:123: typedef
std::pair<SessionTab*, bool> SessionTabPair;
On 2011/09/26 18:34:44, akalin wrote:
> prefer an explicit struct, otherwise it gets confusing (e.g.,
it->second.second)

Done.

http://codereview.chromium.org/7966020/diff/5001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session_tracker.h:127: typedef
std::pair<SessionWindow*, bool> SessionWindowPair;
On 2011/09/26 18:34:44, akalin wrote:
> explicit struct

Done.

[akalin, 2011-09-26 21:57:23]

Few more comments, still looking over it

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session.h (right):

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session.h:45: // Map of windows that make up
this session. Windowws are owned by the session
Windowws -> Windows

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session_tracker.cc (right):

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:178: SyncedSession*
synced_session = NULL;
move this closer to where it's used?

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:266: std::string title = "";
wrap this in if (VLOG_IS_ON(1)) ?

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/ui/webui/ses...
File chrome/browser/ui/webui/sessions_ui.cc (right):

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/ui/webui/ses...
chrome/browser/ui/webui/sessions_ui.cc:197:
browser_sync::SyncedSession::SyncedWindowMap::const_iterator iter =
prefer for loop (even though the indentation gets awkward)

[Nicolas Zea, 2011-09-26 22:55:26]

Comments addressed, PTAL

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session.h (right):

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session.h:45: // Map of windows that make up
this session. Windowws are owned by the session
On 2011/09/26 21:57:24, akalin wrote:
> Windowws -> Windows

Done.

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session_tracker.cc (right):

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:178: SyncedSession*
synced_session = NULL;
On 2011/09/26 21:57:24, akalin wrote:
> move this closer to where it's used?
It's set in the if here, then used in the Window loop below.

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:266: std::string title = "";
On 2011/09/26 21:57:24, akalin wrote:
> wrap this in if (VLOG_IS_ON(1)) ?

Done.

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/ui/webui/ses...
File chrome/browser/ui/webui/sessions_ui.cc (right):

http://codereview.chromium.org/7966020/diff/11001/chrome/browser/ui/webui/ses...
chrome/browser/ui/webui/sessions_ui.cc:197:
browser_sync::SyncedSession::SyncedWindowMap::const_iterator iter =
On 2011/09/26 21:57:24, akalin wrote:
> prefer for loop (even though the indentation gets awkward)

Done.

[akalin, 2011-09-27 19:44:23]

http://codereview.chromium.org/7966020/diff/9004/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/synced_session_tracker.cc (right):

http://codereview.chromium.org/7966020/diff/9004/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session_tracker.cc:151: delete
window_wrapper.window_ptr;
the more I look over this CL and the more I think about it, the more I think
that this manual memory management should be replaced with linked_ptrs.

The one problem I see is that a SessionWindow owns its contained tabs, which is
why you have to clear it before you release it above.  Does that mean we have to
track tab ownership from this class?  Since we own any tabs we create, but then
they may be taken over by windows?

Also, we also have SyncedSessions, which owns its contained windows -- how does
that interact with this stuff, too?

Maybe I'm just unfamiliar with this code but I'm confused by the ownership. :(

http://codereview.chromium.org/7966020/diff/9004/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session_tracker.cc:178: SyncedSession*
synced_session = NULL;
To elaborate on my previous comment, I was thinking something like:

void ...CleanupSession() {
  SyncedWindowMap::iterator ...;
  if (window_iter != end()) {
    SyncedSession* synced_session = NULL;
    {
      it = synced_session_map_.find(session_tag);
      if (it != ..end()) {
        synced_session = *it;
      }
    }
    for (IDToSessionWindowMap::iterator iter = ...) {
      ...
    }
  }

as I don't see synced_session used in the 2nd if statement below

http://codereview.chromium.org/7966020/diff/9004/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session_tracker.cc:299: // because they should
already be referenced through synced_session_map_.
Confused by this comment -- we delete the synced sessions above, which should
delete the windows, right?  But we're deleting more windows here, too?

[Nicolas Zea, 2011-09-27 20:12:13]

http://codereview.chromium.org/7966020/diff/9004/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/synced_session_tracker.cc (right):

http://codereview.chromium.org/7966020/diff/9004/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session_tracker.cc:151: delete
window_wrapper.window_ptr;
Yes, we do we have to track the tab ownership. The thing that makes this
complicated is that tab's are created in one of two cases. First, if the tab
node arrives before the meta node (that contains the windows). We then create an
unmapped tab node. In the second case, where we get a request for a tab we
haven't received yet while processing the window information in a meta node, we
create an empty SessionTab object, which will get filled when the tab node does
arrive.

In other words, tab ownership is indeed closely tracked, and can be owned by one
of two objects (when we're not within a tracking phase). When the tab is in a
temporary state because we haven't received the meta node with the window's that
hold them, they are owned by unmapped_tabs. While the tab has a window, it's
owned by the window itself (similarly the window is owned by the SyncedSession).

Now, that said, during the tracking phase we're in the middle of moving
ownership around (since we need to support tabs being moved between windows). As
a result, we always keep pointers to all the tabs and windows through the
synced_window_map_ and synced_tab_map_. Those are just containers and don't own
the tabs/windows. They're just for keeping track of if something is in use and
aid in quick lookups.

http://codereview.chromium.org/7966020/diff/9004/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session_tracker.cc:178: SyncedSession*
synced_session = NULL;
On 2011/09/27 19:44:24, akalin wrote:
> To elaborate on my previous comment, I was thinking something like:
> 
> void ...CleanupSession() {
>   SyncedWindowMap::iterator ...;
>   if (window_iter != end()) {
>     SyncedSession* synced_session = NULL;
>     {
>       it = synced_session_map_.find(session_tag);
>       if (it != ..end()) {
>         synced_session = *it;
>       }
>     }
>     for (IDToSessionWindowMap::iterator iter = ...) {
>       ...
>     }
>   }
> 
> as I don't see synced_session used in the 2nd if statement below

You're right, it's not used in the if statement for the SessionTabs. But I
wanted to delete the SessionWindows even if there is no SyncedSession (if there
is we additionally delete it from the session itself though).  That said, I was
thinking I should have that functionality just for robustness, but I suppose it
doesn't make sense to be doing session tracking if you haven't created the
session yet. I've changed to code to reflect this.

http://codereview.chromium.org/7966020/diff/9004/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session_tracker.cc:299: // because they should
already be referenced through synced_session_map_.
On 2011/09/27 19:44:24, akalin wrote:
> Confused by this comment -- we delete the synced sessions above, which should
> delete the windows, right?  But we're deleting more windows here, too?

This just deletes the maps, not the windows themselves (synced_window_map_ has a
session tag for a key and a map for a value).

[akalin, 2011-09-28 23:08:29]

> This just deletes the maps, not the windows themselves (synced_window_map_ has
a
> session tag for a key and a map for a value).

Ah, I see.  Is there any reason why the value is a map pointer?  It seems to me
an unneeded optimization, as we don't copy values in and out of the map very
often.

I'd like to have the values be straight map values.  Tab/window ownership is
already subtle enough, no need to muddle the waters with more pointers. :(

[Nicolas Zea, 2011-09-29 00:51:29]

Fair enough, done. PTAL

[akalin, 2011-09-29 21:59:54]

Okay, I think I finally grok this.  I think my main problem was that the
ownership was getting set *outside* of the SessionTracker (in the model
associator) so that confused me.

Have some comments to (hopefully) make the ownership tracking easier by making
it reside in SessionTracker completely.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session_tracker.cc (right):

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:26: // Fill vector of
sessions from our synced session map.
clear sessions first? (or assert empty)

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:58:
SyncedSession::SyncedWindowMap::iterator window_iter =
nit: better to rewrite as for loop

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:71: if
(synced_tab_map_.find(tag) == synced_tab_map_.end()) {
store iterators, use that instead of []

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:118:
IDToSessionWindowMap::iterator window_map_iter;
move this var in for loop (can rename to it)

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:128:
IDToSessionTabMap::iterator tab_map_iter;
here, too

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:187:
IDToSessionWindowMap::iterator iter;
move in for loop

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session_tracker.h (right):

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:12: #include <utility>
no need for this anymore

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:38: // Returns true if we had
foreign sessions to fill it with, false otherwise.
Comment that ownership of the SyncedSessions stays with the SST.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:39: bool
LookupAllForeignSessions(std::vector<const SyncedSession*>* sessions);
make const?

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:47: bool
LookupSessionWindows(const std::string& session_tag,
Comment that ownership of the SessionWindows stays with the SST.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:47: bool
LookupSessionWindows(const std::string& session_tag,
make const?

also, do the returned SessionWindow pointers have to be const?

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:52: // - Sets tab to point to
the SessionTab, and returns true.
Comment that ownership of the SessionTab stays with the SST.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:57: const SessionTab** tab);
make const?

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:60: // |session_tag|.  If none
exists, creates one.
Given the changes below, is this function needed anymore?  Can it be changed to
simply:

void PutSession(...);

That creates the session if it's not already created?

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:82: SessionWindow*
GetWindow(const std::string& session_tag,
Change this to something like:

PutWindowInSession(session_tag, window_id);

and do the
foreign_session->windows[window_id] = window_ptr;
dance inside

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:88: SessionTab*
GetTabForWindow(const std::string& session_tag,
Similarly, change this to something like:

PutTabInWindow(session_tag, window_id, tab_id);

and do the:
window->tabs[tab_id] = tab_ptr;
dance inside

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:94: SessionTab* GetTab(const
std::string& session_tag,
Comment that ownership of the SessionTab stays with its owning window/SST.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:99: void clear();
nit: Clear() is Google style

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:101: inline bool empty() {
nit: -inline, make const (if it's in the class definition, it's already inline)

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:106: inline size_t
num_synced_sessions() {
make const, -inline

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:111: inline size_t
num_synced_tabs(const std::string& session_tag) {
make const

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:112: if
(synced_tab_map_.find(session_tag) != synced_tab_map_.end()) {
probably need to store the iterator in a var and deref it to keep constness

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:122: // above).
comment that SessionTabWrapper doesn't take ownership; the tab is owned either
by its contained window or by unmapped_tabs_

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:134: struct
SessionWindowWrapper {
comment that SessionWindowWrapper doesn't take ownership; the window is owned by
its containing session

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:135: SessionWindowWrapper() :
window_ptr(NULL), used(false) {}
used -> owner?

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:170: // tabs, and won't be
deleted if we delete synced_session_map_.
say that these are owned by the SST itself

[Nicolas Zea, 2011-09-30 22:52:54]

All comments addressed, PTAL

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session_tracker.cc (right):

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:26: // Fill vector of
sessions from our synced session map.
On 2011/09/29 21:59:55, akalin wrote:
> clear sessions first? (or assert empty)

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:58:
SyncedSession::SyncedWindowMap::iterator window_iter =
On 2011/09/29 21:59:55, akalin wrote:
> nit: better to rewrite as for loop

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:71: if
(synced_tab_map_.find(tag) == synced_tab_map_.end()) {
On 2011/09/29 21:59:55, akalin wrote:
> store iterators, use that instead of []

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:118:
IDToSessionWindowMap::iterator window_map_iter;
On 2011/09/29 21:59:55, akalin wrote:
> move this var in for loop (can rename to it)

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:128:
IDToSessionTabMap::iterator tab_map_iter;
On 2011/09/29 21:59:55, akalin wrote:
> here, too

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.cc:187:
IDToSessionWindowMap::iterator iter;
On 2011/09/29 21:59:55, akalin wrote:
> move in for loop

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session_tracker.h (right):

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:12: #include <utility>
On 2011/09/29 21:59:55, akalin wrote:
> no need for this anymore

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:38: // Returns true if we had
foreign sessions to fill it with, false otherwise.
On 2011/09/29 21:59:55, akalin wrote:
> Comment that ownership of the SyncedSessions stays with the SST.

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:39: bool
LookupAllForeignSessions(std::vector<const SyncedSession*>* sessions);
On 2011/09/29 21:59:55, akalin wrote:
> make const?

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:47: bool
LookupSessionWindows(const std::string& session_tag,
On 2011/09/29 21:59:55, akalin wrote:
> Comment that ownership of the SessionWindows stays with the SST.

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:47: bool
LookupSessionWindows(const std::string& session_tag,
On 2011/09/29 21:59:55, akalin wrote:
> make const?
> 
> also, do the returned SessionWindow pointers have to be const?

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:52: // - Sets tab to point to
the SessionTab, and returns true.
On 2011/09/29 21:59:55, akalin wrote:
> Comment that ownership of the SessionTab stays with the SST.

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:57: const SessionTab** tab);
On 2011/09/29 21:59:55, akalin wrote:
> make const?

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:60: // |session_tag|.  If none
exists, creates one.
On 2011/09/29 21:59:55, akalin wrote:
> Given the changes below, is this function needed anymore?  Can it be changed
to
> simply:
> 
> void PutSession(...);
> 
> That creates the session if it's not already created?

We still need to get the session itself to set some of it's values (name and
device type).

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:82: SessionWindow*
GetWindow(const std::string& session_tag,
On 2011/09/29 21:59:55, akalin wrote:
> Change this to something like:
> 
> PutWindowInSession(session_tag, window_id);
> 
> and do the
> foreign_session->windows[window_id] = window_ptr;
> dance inside

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:88: SessionTab*
GetTabForWindow(const std::string& session_tag,
On 2011/09/29 21:59:55, akalin wrote:
> Similarly, change this to something like:
> 
> PutTabInWindow(session_tag, window_id, tab_id);
> 
> and do the:
> window->tabs[tab_id] = tab_ptr;
> dance inside

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:94: SessionTab* GetTab(const
std::string& session_tag,
On 2011/09/29 21:59:55, akalin wrote:
> Comment that ownership of the SessionTab stays with its owning window/SST.

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:99: void clear();
On 2011/09/29 21:59:55, akalin wrote:
> nit: Clear() is Google style

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:101: inline bool empty() {
On 2011/09/29 21:59:55, akalin wrote:
> nit: -inline, make const (if it's in the class definition, it's already
inline)

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:106: inline size_t
num_synced_sessions() {
On 2011/09/29 21:59:55, akalin wrote:
> make const, -inline

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:111: inline size_t
num_synced_tabs(const std::string& session_tag) {
On 2011/09/29 21:59:55, akalin wrote:
> make const

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:112: if
(synced_tab_map_.find(session_tag) != synced_tab_map_.end()) {
On 2011/09/29 21:59:55, akalin wrote:
> probably need to store the iterator in a var and deref it to keep constness

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:122: // above).
On 2011/09/29 21:59:55, akalin wrote:
> comment that SessionTabWrapper doesn't take ownership; the tab is owned either
> by its contained window or by unmapped_tabs_

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:134: struct
SessionWindowWrapper {
On 2011/09/29 21:59:55, akalin wrote:
> comment that SessionWindowWrapper doesn't take ownership; the window is owned
by
> its containing session

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:135: SessionWindowWrapper() :
window_ptr(NULL), used(false) {}
On 2011/09/29 21:59:55, akalin wrote:
> used -> owner?

Done.

http://codereview.chromium.org/7966020/diff/23001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:170: // tabs, and won't be
deleted if we delete synced_session_map_.
On 2011/09/29 21:59:55, akalin wrote:
> say that these are owned by the SST itself

Done.

[akalin, 2011-10-01 01:54:14]

More comments.  How urgent is this CL?  If this needs to go in soon, we can talk
about checking this in as-is or with minimal changes and handling these comments
in a follow-up.

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:133: SyncedSession*
current_session =
i think with the changes suggested below there's no need to get the session
explicitly

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:193:
synced_session_tracker_.PutWindowInSession(local_tag, window_id);
see comment below about making PutWindowInSession also fill in info from
specifics

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:638: // we start calling
PutWindowInSessino and PutTabInWindow so that all
Sessino -> Session

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:651:
PopulateSessionWindowFromSpecifics(foreign_session_tag,
I think the PopulateSessionWindowFromSpecifics logic more properly belongs in
the SyncedSessionTracker, merged with PutWindowInSession.  So it'd look like:

synced_session_tracker_.PutWindowInSession(foreign_session_tag, window_id,
window_s, modification_time);

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:663: SessionTab* tab =
I think there's a similar need to rename GetTab to, say, PutWindowslessTab() and
have it take the tab specifics

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:916: bool
SessionModelAssociator::GetLocalSession(
This is needed for test only, right?  Append "ForTest, change to return a const
pointer directly, also see comment re. GetSession in SyncedSessionTracker

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:925: bool
SessionModelAssociator::GetAllForeignSessions(
append ForTest, return vector directly

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:931: bool
SessionModelAssociator::GetForeignSession(
append ForTest, return vector directly

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session_tracker.h (right):

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:39: bool
LookupAllForeignSessions(std::vector<const SyncedSession*>* sessions)
There's no need to differentiate between this returning false and this returning
true but giving an empty vector, right?  If so, I think it's cleaner to make it
return a vector directly.

From a cursory search, is this used outside of test?  If not, I'd like to suffix
it with ForTest.

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:49: bool
LookupSessionWindows(const std::string& session_tag,
Ditto

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:53: // Ownership of the
SessionTab's remains within the SyncedSessionTracker.
no "'s" since there's only one

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:58: bool
LookupSessionTab(const std::string& session_tag,
what about returning the tab directly, and NULL if it doesn't exist?

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:65: SyncedSession*
GetSession(const std::string& session_tag);
WIth my comments in the model associator file, I think this can be changed to:

const SyncedSession* GetSessionForTest(...) const;

and maybe a private helper function: "SyncedSession GetSession(...)" used by
PutWindowInSession.

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:87: // |session_tag|, and
markes the window as being owned. If none existed for
Specify that the session is created as needed also.  Also see comments in model
associator about making this function also set info from specifics.

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:96: void PutTabInWindow(const
std::string& session_tag,
Would it make sense for PutTabInWindow to auto-create the window/session, too?

[akalin, 2011-10-01 01:57:15]

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session_tracker.h (right):

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:39: bool
LookupAllForeignSessions(std::vector<const SyncedSession*>* sessions)
On 2011/10/01 01:54:14, akalin wrote:
> There's no need to differentiate between this returning false and this
returning
> true but giving an empty vector, right?  If so, I think it's cleaner to make
it
> return a vector directly.
> 
> From a cursory search, is this used outside of test?  If not, I'd like to
suffix
> it with ForTest.

Ah, this is used in the foreign_session_handler, of course.  Nevermind my
comments about appending ForTest.

[Nicolas Zea, 2011-10-03 18:36:50]

I see what you mean, but I think that's now getting into a larger refactor of
the Sessions datatype itself. I agree that needs to be done, but I think I'd
rather do that as part of the effort to transition sessions to the new API. For
now, since this is a feature-blocking crasher, I'd like to land this closer to
the current state.

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:638: // we start calling
PutWindowInSessino and PutTabInWindow so that all
On 2011/10/01 01:54:14, akalin wrote:
> Sessino -> Session

Done.

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:663: SessionTab* tab =
On 2011/10/01 01:54:14, akalin wrote:
> I think there's a similar need to rename GetTab to, say, PutWindowslessTab()
and
> have it take the tab specifics

But it's not necessarily windowless. It's possible the meta node already
processed it before the tab node arrived, in which case this will get and fill
the previously created SessionTab.

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator.cc:916: bool
SessionModelAssociator::GetLocalSession(
On 2011/10/01 01:54:14, akalin wrote:
> This is needed for test only, right?  Append "ForTest, change to return a
const
> pointer directly, also see comment re. GetSession in SyncedSessionTracker

This and the methods below are used in (or are meant to be available to) the
foreign session handler and the session UI.

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session_tracker.h (right):

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:53: // Ownership of the
SessionTab's remains within the SyncedSessionTracker.
On 2011/10/01 01:54:14, akalin wrote:
> no "'s" since there's only one

Done.

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:58: bool
LookupSessionTab(const std::string& session_tag,
On 2011/10/01 01:54:14, akalin wrote:
> what about returning the tab directly, and NULL if it doesn't exist?

I kind of prefer being consistent with the other "Lookup*" methods above and
returning a bool the way they do.

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:87: // |session_tag|, and
markes the window as being owned. If none existed for
On 2011/10/01 01:54:14, akalin wrote:
> Specify that the session is created as needed also.  Also see comments in
model
> associator about making this function also set info from specifics.

Done.

http://codereview.chromium.org/7966020/diff/33021/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session_tracker.h:96: void PutTabInWindow(const
std::string& session_tag,
On 2011/10/01 01:54:14, akalin wrote:
> Would it make sense for PutTabInWindow to auto-create the window/session, too?

The window/session should be created before this is called. I don't really see
any reason to call this without them having been made, at least at the moment.

[Nicolas Zea, 2011-10-03 18:39:43]

Also filed crbug.com/98892 to cover the move to the new API and refactoring the
population code into the SyncedSessionTracker

[akalin, 2011-10-03 18:42:15]

On 2011/10/03 18:36:50, nzea wrote:
> I see what you mean, but I think that's now getting into a larger refactor of
> the Sessions datatype itself. I agree that needs to be done, but I think I'd
> rather do that as part of the effort to transition sessions to the new API.
For
> now, since this is a feature-blocking crasher, I'd like to land this closer to
> the current state.

Okay, I'll take another look with an eye towards landing this soon.

[akalin, 2011-10-03 18:50:21]

LGTM after nits

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session.cc (right):

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session.cc:49: if (num_populated == 0)
return (num_populated == 0);

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sessions_helper.cc (right):

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sessions_helper.cc:48: std::swap(*windows,
windows_);
set should probably delete the pointers in windows_ before swapping, right?

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sessions_helper.h (right):

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sessions_helper.h:34: const
SessionWindowMap* const_get() const;
maybe:

const SessionWindowMap& Get() const;
SessionWindowMap* GetMutable();
void Reset(...);

[Nicolas Zea, 2011-10-03 19:36:15]

Done. Committing once bots go green.

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/synced_session.cc (right):

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/synced_session.cc:49: if (num_populated == 0)
On 2011/10/03 18:50:21, akalin wrote:
> return (num_populated == 0);

Done.

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sessions_helper.cc (right):

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sessions_helper.cc:48: std::swap(*windows,
windows_);
On 2011/10/03 18:50:21, akalin wrote:
> set should probably delete the pointers in windows_ before swapping, right?

Done.

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sessions_helper.h (right):

http://codereview.chromium.org/7966020/diff/37001/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sessions_helper.h:34: const
SessionWindowMap* const_get() const;
On 2011/10/03 18:50:21, akalin wrote:
> maybe:
> 
> const SessionWindowMap& Get() const;
> SessionWindowMap* GetMutable();
> void Reset(...);

Done.

[commit-bot: I haz the power, 2011-10-03 21:08:21]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/7966020/41002

[commit-bot: I haz the power, 2011-10-04 13:43:14]

Change committed as 103878