4 files changed, 221 insertions, 58 deletions
diff --git a/phpBB/phpbb/session.php b/phpBB/phpbb/session.php
index 7c76c08b73..6851bc8188 100644
--- a/phpBB/phpbb/session.php
+++ b/phpBB/phpbb/session.php
@@ -954,72 +954,96 @@ class session
 	{
 		global $db, $config, $phpbb_container, $phpbb_dispatcher;
 
-		$batch_size = 10;
-
 		if (!$this->time_now)
 		{
 			$this->time_now = time();
 		}
 
-		// Firstly, delete guest sessions
-		$sql = 'DELETE FROM ' . SESSIONS_TABLE . '
-			WHERE session_user_id = ' . ANONYMOUS . '
-				AND session_time < ' . (int) ($this->time_now - $config['session_length']);
-		$db->sql_query($sql);
+		/**
+		 * Get expired sessions for registered users, only most recent for each user
+		 * Inner SELECT gets most recent expired sessions for unique session_user_id
+		 * Outer SELECT gets data for them
+		 */
+		$sql_select = 'SELECT s1.session_page, s1.session_user_id, s1.session_time AS recent_time
+			FROM ' . SESSIONS_TABLE . ' AS s1
+			INNER JOIN (
+				SELECT session_user_id, MAX(session_time) AS recent_time
+				FROM ' . SESSIONS_TABLE . '
+				WHERE session_time < ' . ($this->time_now - (int) $config['session_length']) . '
+					AND session_user_id <> ' . ANONYMOUS . '
+				GROUP BY session_user_id
+			) AS s2
+			ON s1.session_user_id = s2.session_user_id
+				AND s1.session_time = s2.recent_time';
+
+		switch ($db->get_sql_layer())
+		{
+			case 'sqlite3':
+				if (phpbb_version_compare($db->sql_server_info(true), '3.8.3', '>='))
+				{
+					// For SQLite versions 3.8.3+ which support Common Table Expressions (CTE)
+					$sql = "WITH s3 (session_page, session_user_id, session_time) AS ($sql_select)
+						UPDATE " . USERS_TABLE . '
+						SET (user_lastpage, user_lastvisit) = (SELECT session_page, session_time FROM s3 WHERE session_user_id = user_id)
+						WHERE EXISTS (SELECT session_user_id FROM s3 WHERE session_user_id = user_id)';
+					$db->sql_query($sql);
 
-		// Get expired sessions, only most recent for each user
-		$sql = 'SELECT session_user_id, session_page, MAX(session_time) AS recent_time
-			FROM ' . SESSIONS_TABLE . '
-			WHERE session_time < ' . ($this->time_now - $config['session_length']) . '
-			GROUP BY session_user_id, session_page';
-		$result = $db->sql_query_limit($sql, $batch_size);
+					break;
+				}
 
-		$del_user_id = array();
-		$del_sessions = 0;
+			// No break, for SQLite versions prior to 3.8.3 and Oracle
+			case 'oracle':
+				$result = $db->sql_query($sql_select);
+				while ($row = $db->sql_fetchrow($result))
+				{
+					$sql = 'UPDATE ' . USERS_TABLE . '
+						SET user_lastvisit = ' . (int) $row['recent_time'] . ", user_lastpage = '" . $db->sql_escape($row['session_page']) . "'
+						WHERE user_id = " . (int) $row['session_user_id'];
+					$db->sql_query($sql);
+				}
+				$db->sql_freeresult($result);
+			break;
 
-		while ($row = $db->sql_fetchrow($result))
-		{
-			$sql = 'UPDATE ' . USERS_TABLE . '
-				SET user_lastvisit = ' . (int) $row['recent_time'] . ", user_lastpage = '" . $db->sql_escape($row['session_page']) . "'
-				WHERE user_id = " . (int) $row['session_user_id'];
-			$db->sql_query($sql);
+			case 'mysqli':
+				$sql = 'UPDATE ' . USERS_TABLE . " u,
+					($sql_select) s3
+					SET u.user_lastvisit = s3.recent_time, u.user_lastpage = s3.session_page
+					WHERE u.user_id = s3.session_user_id";
+				$db->sql_query($sql);
+			break;
 
-			$del_user_id[] = (int) $row['session_user_id'];
-			$del_sessions++;
+			default:
+				$sql = 'UPDATE ' . USERS_TABLE . "
+					SET user_lastvisit = s3.recent_time, user_lastpage = s3.session_page
+					FROM ($sql_select) s3
+					WHERE user_id = s3.session_user_id";
+				$db->sql_query($sql);
+			break;
 		}
-		$db->sql_freeresult($result);
 
-		if (count($del_user_id))
+		// Delete all expired sessions
+		$sql = 'DELETE FROM ' . SESSIONS_TABLE . '
+			WHERE session_time < ' . ($this->time_now - (int) $config['session_length']);
+		$db->sql_query($sql);
+
+		// Update gc timer
+		$config->set('session_last_gc', $this->time_now, false);
+
+		if ($config['max_autologin_time'])
 		{
-			// Delete expired sessions
-			$sql = 'DELETE FROM ' . SESSIONS_TABLE . '
-				WHERE ' . $db->sql_in_set('session_user_id', $del_user_id) . '
-					AND session_time < ' . ($this->time_now - $config['session_length']);
+			$sql = 'DELETE FROM ' . SESSIONS_KEYS_TABLE . '
+				WHERE last_login < ' . (time() - (86400 * (int) $config['max_autologin_time']));
 			$db->sql_query($sql);
 		}
 
-		if ($del_sessions < $batch_size)
-		{
-			// Less than 10 users, update gc timer ... else we want gc
-			// called again to delete other sessions
-			$config->set('session_last_gc', $this->time_now, false);
+		// only called from CRON; should be a safe workaround until the infrastructure gets going
+		/* @var \phpbb\captcha\factory $captcha_factory */
+		$captcha_factory = $phpbb_container->get('captcha.factory');
+		$captcha_factory->garbage_collect($config['captcha_plugin']);
 
-			if ($config['max_autologin_time'])
-			{
-				$sql = 'DELETE FROM ' . SESSIONS_KEYS_TABLE . '
-					WHERE last_login < ' . (time() - (86400 * (int) $config['max_autologin_time']));
-				$db->sql_query($sql);
-			}
-
-			// only called from CRON; should be a safe workaround until the infrastructure gets going
-			/* @var $captcha_factory \phpbb\captcha\factory */
-			$captcha_factory = $phpbb_container->get('captcha.factory');
-			$captcha_factory->garbage_collect($config['captcha_plugin']);
-
-			$sql = 'DELETE FROM ' . LOGIN_ATTEMPT_TABLE . '
-				WHERE attempt_time < ' . (time() - (int) $config['ip_login_limit_time']);
-			$db->sql_query($sql);
-		}
+		$sql = 'DELETE FROM ' . LOGIN_ATTEMPT_TABLE . '
+			WHERE attempt_time < ' . (time() - (int) $config['ip_login_limit_time']);
+		$db->sql_query($sql);
 
 		/**
 		* Event to trigger extension on session_gc
diff --git a/tests/session/fixtures/sessions_garbage.xml b/tests/session/fixtures/sessions_garbage.xml
index 5eace839d0..59a2dc2ebe 100644
--- a/tests/session/fixtures/sessions_garbage.xml
+++ b/tests/session/fixtures/sessions_garbage.xml
@@ -5,11 +5,23 @@
 		<column>username_clean</column>
 		<column>user_permissions</column>
 		<column>user_sig</column>
+		<column>user_lastpage</column>
+		<column>user_lastvisit</column>
 		<row>
 			<value>4</value>
 			<value>bar</value>
 			<value></value>
 			<value></value>
+			<value>oldpage_user_bar.php</value>
+			<value>1400000000</value>
+		</row>
+		<row>
+			<value>5</value>
+			<value>foo</value>
+			<value></value>
+			<value></value>
+			<value>oldpage_user_foo.php</value>
+			<value>1400000000</value>
 		</row>
 	</table>
 	<table name="phpbb_sessions">
@@ -18,12 +30,16 @@
 		<column>session_ip</column>
 		<column>session_browser</column>
 		<column>session_admin</column>
+		<column>session_page</column>
+		<column>session_time</column>
 		<row>
 			<value>anon_session00000000000000000000</value>
 			<value>1</value>
 			<value>127.0.0.1</value>
 			<value>anonymous user agent</value>
 			<value>0</value>
+			<value></value>
+			<value>1500000005</value>
 		</row>
 		<row>
 			<value>bar_session000000000000000000000</value>
@@ -31,6 +47,35 @@
 			<value>127.0.0.1</value>
 			<value>user agent</value>
 			<value>1</value>
+			<value>newpage_user_bar.php</value>
+			<value>1500000000</value>
+		</row>
+		<row>
+			<value>bar_session000000000000000000002</value>
+			<value>4</value>
+			<value>127.0.0.1</value>
+			<value>user agent</value>
+			<value>1</value>
+			<value>oldpage_user_bar.php</value>
+			<value>1400000000</value>
+		</row>
+		<row>
+			<value>foo_session000000000000000000000</value>
+			<value>5</value>
+			<value>127.0.0.1</value>
+			<value>user agent</value>
+			<value>0</value>
+			<value>newpage_user_foo.php</value>
+			<value>1500000000</value>
+		</row>
+		<row>
+			<value>foo_session000000000000000000002</value>
+			<value>5</value>
+			<value>127.0.0.1</value>
+			<value>user agent</value>
+			<value>0</value>
+			<value>oldpage_user_foo.php</value>
+			<value>1400000000</value>
 		</row>
 	</table>
 	<table name="phpbb_login_attempts">
diff --git a/tests/session/garbage_collection_test.php b/tests/session/garbage_collection_test.php
index d361e022da..ec248b2904 100644
--- a/tests/session/garbage_collection_test.php
+++ b/tests/session/garbage_collection_test.php
@@ -41,19 +41,91 @@ class phpbb_session_garbage_collection_test extends phpbb_session_test_case
 		);
 	}
 
+	public function test_session_gc()
+	{
+		global $config;
+		$config['session_length'] = 3600;
+
+		$this->check_expired_sessions_recent(
+			[
+				[
+					'session_user_id' => 4,
+					'recent_time' => 1500000000,
+				],
+				[
+					'session_user_id' => 5,
+					'recent_time' => 1500000000,
+				],
+			],
+			'Before test, should get recent expired sessions only.'
+		);
+
+		$this->check_user_session_data(
+			[
+				[
+					'username_clean' => 'bar',
+					'user_lastvisit' => 1400000000,
+					'user_lastpage' => 'oldpage_user_bar.php',
+				],
+				[
+					'username_clean' => 'foo',
+					'user_lastvisit' => 1400000000,
+					'user_lastpage' => 'oldpage_user_foo.php',
+				],
+			],
+			'Before test, users session data is not updated yet.'
+		);
+
+		// There is an error unless the captcha plugin is set
+		$config['captcha_plugin'] = 'core.captcha.plugins.nogd';
+		$this->session->session_gc();
+		$this->check_expired_sessions_recent(
+			[],
+			'After garbage collection, all expired sessions should be removed.'
+		);
+
+		$this->check_user_session_data(
+			[
+				[
+					'username_clean' => 'bar',
+					'user_lastvisit' => '1500000000',
+					'user_lastpage' => 'newpage_user_bar.php',
+				],
+				[
+					'username_clean' => 'foo',
+					'user_lastvisit' => '1500000000',
+					'user_lastpage' => 'newpage_user_foo.php',
+				],
+			],
+			'After garbage collection, users session data should be updated to the recent expired sessions data.'
+		);
+	}
+
 	public function test_cleanup_all()
 	{
 		$this->check_sessions_equals(
-			array(
-				array(
+			[
+				[
 					'session_id' => 'anon_session00000000000000000000',
 					'session_user_id' => 1,
-				),
-				array(
+				],
+				[
 					'session_id' => 'bar_session000000000000000000000',
 					'session_user_id' => 4,
-				),
-			),
+				],
+				[
+					'session_id' => 'bar_session000000000000000000002',
+					'session_user_id' => 4,
+				],
+				[
+					'session_id' => 'foo_session000000000000000000000',
+					'session_user_id' => 5,
+				],
+				[
+					'session_id' => 'foo_session000000000000000000002',
+					'session_user_id' => 5,
+				],
+			],
 			'Before test, should have some sessions.'
 		);
 		// Set session length so it clears all
@@ -63,7 +135,7 @@ class phpbb_session_garbage_collection_test extends phpbb_session_test_case
 		$config['captcha_plugin'] = 'core.captcha.plugins.nogd';
 		$this->session->session_gc();
 		$this->check_sessions_equals(
-			array(),
+			[],
 			'After setting session time to 0, should remove all.'
 		);
 	}
diff --git a/tests/test_framework/phpbb_session_test_case.php b/tests/test_framework/phpbb_session_test_case.php
index 02722c473e..530d8c6b48 100644
--- a/tests/test_framework/phpbb_session_test_case.php
+++ b/tests/test_framework/phpbb_session_test_case.php
@@ -48,11 +48,33 @@ abstract class phpbb_session_test_case extends phpbb_database_test_case
 			new phpbb_session_testable_facade($this->db, $this->session_factory);
 	}
 
+	protected function check_user_session_data($expected_session_data, $message)
+	{
+		$sql= 'SELECT username_clean, user_lastvisit, user_lastpage
+			FROM ' . USERS_TABLE . '
+			ORDER BY user_id';
+
+		$this->assertSqlResultEquals($expected_session_data, $sql, $message);
+	}
+
+	protected function check_expired_sessions_recent($expected_sessions, $message)
+	{
+		global $config;
+		$time_now = time();
+		$sql = 'SELECT session_user_id, MAX(session_time) AS recent_time
+			FROM ' . SESSIONS_TABLE . '
+			WHERE session_time < ' . ($time_now - (int) $config['session_length']) . '
+				AND session_user_id <> ' . ANONYMOUS . '
+			GROUP BY session_user_id';
+
+		$this->assertSqlResultEquals($expected_sessions, $sql, $message);
+	}
+
 	protected function check_sessions_equals($expected_sessions, $message)
 	{
 		$sql = 'SELECT session_id, session_user_id
 				FROM phpbb_sessions
-				ORDER BY session_user_id';
+				ORDER BY session_user_id, session_id';
 
 		$this->assertSqlResultEquals($expected_sessions, $sql, $message);
 	}