components/sync_sessions/sessions_sync_manager.cc - Issue 1877083002: [Sync] Moved tab_node_id tracking to session object and improved foreign session garbage collection.

Unified Diff: components/sync_sessions/sessions_sync_manager.cc

Issue 1877083002: [Sync] Moved tab_node_id tracking to session object and improved foreign session garbage collection. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 4 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« no previous file with comments | « components/sync_sessions/sessions_sync_manager.h ('k') | components/sync_sessions/synced_session.h » ('j') | components/sync_sessions/synced_session_tracker.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: components/sync_sessions/sessions_sync_manager.cc

diff --git a/components/sync_sessions/sessions_sync_manager.cc b/components/sync_sessions/sessions_sync_manager.cc

index 0b9eb7e56112aa0ea852985ad699a5ec55a7435e..ced58f56ff6191845dd2a200d4aa6119597ee12c 100644

--- a/components/sync_sessions/sessions_sync_manager.cc

+++ b/components/sync_sessions/sessions_sync_manager.cc

@@ -43,7 +43,7 @@ const char kNTPOpenTabSyncURL[] = "chrome://newtab/#open_tabs";

// Default number of days without activity after which a session is considered

// stale and becomes a candidate for garbage collection.

-const size_t kDefaultStaleSessionThresholdDays = 14; // 2 weeks.

+const int kDefaultStaleSessionThresholdDays = 14; // 2 weeks.

// Comparator function for use with std::sort that will sort tabs by

// descending timestamp (i.e., most recent first).

@@ -515,6 +515,18 @@ syncer::SyncError SessionsSyncManager::ProcessSyncChanges(

// deletions, the header node will be updated and foreign tab will

// get deleted.

DisassociateForeignSession(session.session_tag());

+ } else if (session.has_tab() &&

+ current_machine_tag() != session.session_tag()) {

Nicolas Zea 2016/04/12 20:38:03 the current_machine_tag condition isn't necessary

skym 2016/04/12 22:12:14 Done.

+ // The challenge here is that we don't know if this tab deletion is

+ // being processes before or after the parent was updated to no longer

Nicolas Zea 2016/04/12 20:38:03 being processes -> being processed

skym 2016/04/12 22:12:14 Done.

+ // reference us, or even more extreme, the parent has been deleted as

Nicolas Zea 2016/04/12 20:38:03 "reference us, or even" -> "reference the tab. Or,

skym 2016/04/12 22:12:13 Done.

+ // well. Tell the tracker to do what it can. The header's update will

+ // mostly get us into the correct state, the only thing this deletion

+ // needs to accomplish is make sure we never tell sync to delete this

+ // tab later during garbage collection.

+ session_tracker_.TryDeleteTab(session.session_tag(),

+ session.tab().tab_id(),

+ session.tab_node_id());

}

break;

case syncer::SyncChange::ACTION_ADD:

@@ -523,9 +535,6 @@ syncer::SyncError SessionsSyncManager::ProcessSyncChanges(

// We should only ever receive a change to our own machine's session

// info if encryption was turned on. In that case, the data is still

// the same, so we can ignore.

- // TODO(skym): Is it really safe to return here? Why not continue?

- // Couldn't there be multiple SessionSpecifics in the SyncChangeList

- // that contain different session tags?

LOG(WARNING) << "Dropping modification to local session.";

return syncer::SyncError();

}

@@ -558,7 +567,7 @@ syncer::SyncChange SessionsSyncManager::TombstoneTab(

bool SessionsSyncManager::GetAllForeignSessions(

std::vector<const sync_driver::SyncedSession*>* sessions) {

- if (!session_tracker_.LookupAllForeignSessions(sessions))

+ if (!session_tracker_.LookupAllForeignSessions(sessions, true))

return false;

std::sort(sessions->begin(), sessions->end(), SessionsRecencyComparator);

return true;

@@ -571,8 +580,6 @@ bool SessionsSyncManager::InitFromSyncModel(

bool found_current_header = false;

for (syncer::SyncDataList::const_iterator it = sync_data.begin();

it != sync_data.end(); ++it) {

- // TODO(skym): Why don't we ever look at data.change_type()? Why is this

- // code path so much different from ProcessSyncChanges?

const syncer::SyncData& data = *it;

DCHECK(data.GetSpecifics().has_session());

const sync_pb::SessionSpecifics& specifics = data.GetSpecifics().session();

@@ -619,6 +626,7 @@ void SessionsSyncManager::UpdateTrackerWithForeignSession(

sync_driver::SyncedSession* foreign_session =

session_tracker_.GetSession(foreign_session_tag);

if (specifics.has_header()) {

// Read in the header data for this foreign session. Header data is

// essentially a collection of windows, each of which has an ordered id list

@@ -635,9 +643,8 @@ void SessionsSyncManager::UpdateTrackerWithForeignSession(

PopulateSessionHeaderFromSpecifics(header, modification_time,

foreign_session);

- // Reset the tab/window tracking for this session (must do this before

- // we start calling PutWindowInSession and PutTabInWindow so that all

- // unused tabs/windows get cleared by the CleanupSession(...) call).

+ // Reset the tab/window tracking for this session before adding them all

+ // back. This allows us to avoid a two way diff.

session_tracker_.ResetSessionTracking(foreign_session_tag);

// Process all the windows and their tab information.

@@ -653,8 +660,6 @@ void SessionsSyncManager::UpdateTrackerWithForeignSession(

modification_time,

foreign_session->windows[window_id]);

}

- // Delete any closed windows and unused tabs as necessary.

- session_tracker_.CleanupSession(foreign_session_tag);

} else if (specifics.has_tab()) {

const sync_pb::SessionTab& tab_s = specifics.tab();

SessionID::id_type tab_id = tab_s.tab_id();

@@ -663,6 +668,11 @@ void SessionsSyncManager::UpdateTrackerWithForeignSession(

if (session_tracker_.LookupSessionTab(foreign_session_tag, tab_id,

&existing_tab) &&

existing_tab->timestamp > modification_time) {

+ // Remember that there is more sync data behind this tab_id if we decide

+ // to garbage collection this session. We never do granular garbage

Nicolas Zea 2016/04/12 20:38:03 to garbage collection -> to garbage collect (or to

skym 2016/04/12 22:12:14 Comment is different now.

+ // collection because we don't want to fight with the owning device.

+ session_tracker_.GetTab(foreign_session_tag, tab_id,

+ specifics.tab_node_id());

DVLOG(1) << "Ignoring " << foreign_session_tag << "'s session tab "

<< tab_id << " with earlier modification time";

return;

@@ -740,7 +750,8 @@ void SessionsSyncManager::PopulateSessionHeaderFromSpecifics(

break;

}

- session_header->modified_time = mtime;

+ session_header->modified_time =

+ std::max(mtime, session_header->modified_time);

}

// static

@@ -811,16 +822,13 @@ void SessionsSyncManager::DeleteForeignSessionInternal(

std::set<int> tab_node_ids_to_delete;

session_tracker_.LookupTabNodeIds(tag, &tab_node_ids_to_delete);

- if (!DisassociateForeignSession(tag)) {

- // We don't have any data for this session, our work here is done!

- return;

+ if (DisassociateForeignSession(tag)) {

+ // Only tell sync to delete the header if there was one.

+ change_output->push_back(

+ syncer::SyncChange(FROM_HERE, SyncChange::ACTION_DELETE,

+ SyncData::CreateLocalDelete(tag, syncer::SESSIONS)));

}

- // Prepare deletes for the meta-node as well as individual tab nodes.

- change_output->push_back(

- syncer::SyncChange(FROM_HERE, SyncChange::ACTION_DELETE,

- SyncData::CreateLocalDelete(tag, syncer::SESSIONS)));

for (std::set<int>::const_iterator it = tab_node_ids_to_delete.begin();

it != tab_node_ids_to_delete.end(); ++it) {

change_output->push_back(syncer::SyncChange(

@@ -834,11 +842,7 @@ void SessionsSyncManager::DeleteForeignSessionInternal(

bool SessionsSyncManager::DisassociateForeignSession(

const std::string& foreign_session_tag) {

- if (foreign_session_tag == current_machine_tag()) {

- DVLOG(1) << "Local session deleted! Doing nothing until a navigation is "

- << "triggered.";

- return false;

- }

+ DCHECK_NE(foreign_session_tag, current_machine_tag());

DVLOG(1) << "Disassociating session " << foreign_session_tag;

return session_tracker_.DeleteSession(foreign_session_tag);

}

@@ -1030,30 +1034,22 @@ SessionsSyncManager::synced_window_delegates_getter() const {

void SessionsSyncManager::DoGarbageCollection() {

std::vector<const sync_driver::SyncedSession*> sessions;

- if (!GetAllForeignSessions(&sessions))

- return; // No foreign sessions.

- // Iterate through all the sessions and delete any with age older than

- // |stale_session_threshold_days_|.

+ session_tracker_.LookupAllForeignSessions(&sessions, false);

syncer::SyncChangeList changes;

- for (std::vector<const sync_driver::SyncedSession*>::const_iterator iter =

- sessions.begin();

- iter != sessions.end(); ++iter) {

- const sync_driver::SyncedSession* session = *iter;

+ for (const auto* session : sessions) {

int session_age_in_days =

(base::Time::Now() - session->modified_time).InDays();

- std::string session_tag = session->session_tag;

- if (session_age_in_days > 0 && // If false, local clock is not trustworty.

- static_cast<size_t>(session_age_in_days) >

- stale_session_threshold_days_) {

+ if (session_age_in_days > stale_session_threshold_days_) {

+ std::string session_tag = session->session_tag;

DVLOG(1) << "Found stale session " << session_tag << " with age "

<< session_age_in_days << ", deleting.";

DeleteForeignSessionInternal(session_tag, &changes);

}

- if (!changes.empty())

+ if (!changes.empty()) {

Nicolas Zea 2016/04/12 20:38:03 nit: rest of file has (I think) been leaving curly

skym 2016/04/12 22:12:14 Done.

sync_processor_->ProcessSyncChanges(FROM_HERE, changes);

+ }

}

}; // namespace browser_sync