Fixes possible use after free in SessionService SessionService::GetLastSession used a base::Unretained but there was no guarantee that the SessionService would be valid by the time the callback was processed. BUG=399655 TEST=covered by test now R=marja@chromium.org Review URL: https://codereview.chromium.org/500143002 Cr-Commit-Position: refs/heads/master@{#291985}
diff --git a/chrome/browser/sessions/base_session_service.cc b/chrome/browser/sessions/base_session_service.cc index a130c89..c7e6e7a 100644 --- a/chrome/browser/sessions/base_session_service.cc +++ b/chrome/browser/sessions/base_session_service.cc
@@ -294,20 +294,17 @@ return id; } -bool BaseSessionService::RunTaskOnBackendThread( +void BaseSessionService::RunTaskOnBackendThread( const tracked_objects::Location& from_here, const base::Closure& task) { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool(); if (!pool->IsShutdownInProgress()) { - return pool->PostSequencedWorkerTask(sequence_token_, - from_here, - task); + pool->PostSequencedWorkerTask(sequence_token_, from_here, task); } else { // Fall back to executing on the main thread if the sequence // worker pool has been requested to shutdown (around shutdown // time). task.Run(); - return true; } }
diff --git a/chrome/browser/sessions/base_session_service.h b/chrome/browser/sessions/base_session_service.h index e85799a6..98f23795 100644 --- a/chrome/browser/sessions/base_session_service.h +++ b/chrome/browser/sessions/base_session_service.h
@@ -156,7 +156,7 @@ // This posts the task to the SequencedWorkerPool, or run immediately // if the SequencedWorkerPool has been shutdown. - bool RunTaskOnBackendThread(const tracked_objects::Location& from_here, + void RunTaskOnBackendThread(const tracked_objects::Location& from_here, const base::Closure& task); // Max number of navigation entries in each direction we'll persist.
diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc index 7b36c75f..0a0ac5a 100644 --- a/chrome/browser/sessions/session_service.cc +++ b/chrome/browser/sessions/session_service.cc
@@ -234,7 +234,8 @@ save_delay_in_millis_(base::TimeDelta::FromMilliseconds(2500)), save_delay_in_mins_(base::TimeDelta::FromMinutes(10)), save_delay_in_hrs_(base::TimeDelta::FromHours(8)), - force_browser_not_alive_with_no_windows_(false) { + force_browser_not_alive_with_no_windows_(false), + weak_factory_(this) { Init(); } @@ -245,7 +246,8 @@ save_delay_in_millis_(base::TimeDelta::FromMilliseconds(2500)), save_delay_in_mins_(base::TimeDelta::FromMinutes(10)), save_delay_in_hrs_(base::TimeDelta::FromHours(8)), - force_browser_not_alive_with_no_windows_(false) { + force_browser_not_alive_with_no_windows_(false), + weak_factory_(this) { Init(); } @@ -571,7 +573,7 @@ // the callback. return ScheduleGetLastSessionCommands( base::Bind(&SessionService::OnGotSessionCommands, - base::Unretained(this), callback), + weak_factory_.GetWeakPtr(), callback), tracker); }
diff --git a/chrome/browser/sessions/session_service.h b/chrome/browser/sessions/session_service.h index 10c337d..8720d27 100644 --- a/chrome/browser/sessions/session_service.h +++ b/chrome/browser/sessions/session_service.h
@@ -11,6 +11,7 @@ #include "base/basictypes.h" #include "base/callback.h" #include "base/memory/scoped_vector.h" +#include "base/memory/weak_ptr.h" #include "base/task/cancelable_task_tracker.h" #include "base/time/time.h" #include "chrome/browser/defaults.h" @@ -493,6 +494,8 @@ // without quitting. bool force_browser_not_alive_with_no_windows_; + base::WeakPtrFactory<SessionService> weak_factory_; + DISALLOW_COPY_AND_ASSIGN(SessionService); };
diff --git a/chrome/browser/sessions/session_service_test_helper.cc b/chrome/browser/sessions/session_service_test_helper.cc index e9ee6da2..0a1f9828 100644 --- a/chrome/browser/sessions/session_service_test_helper.cc +++ b/chrome/browser/sessions/session_service_test_helper.cc
@@ -5,6 +5,7 @@ #include "chrome/browser/sessions/session_service_test_helper.h" #include "base/memory/scoped_vector.h" +#include "base/message_loop/message_loop.h" #include "chrome/browser/sessions/session_backend.h" #include "chrome/browser/sessions/session_service.h" #include "chrome/browser/sessions/session_types.h" @@ -111,3 +112,8 @@ content::BrowserThread::GetBlockingPool()->FlushForTesting(); } +void SessionServiceTestHelper::RunTaskOnBackendThread( + const tracked_objects::Location& from_here, + const base::Closure& task) { + service_->RunTaskOnBackendThread(from_here, task); +}
diff --git a/chrome/browser/sessions/session_service_test_helper.h b/chrome/browser/sessions/session_service_test_helper.h index 7f176c72..8daadd8 100644 --- a/chrome/browser/sessions/session_service_test_helper.h +++ b/chrome/browser/sessions/session_service_test_helper.h
@@ -10,6 +10,7 @@ #include "base/basictypes.h" #include "base/memory/scoped_ptr.h" +#include "base/message_loop/message_loop.h" #include "components/sessions/session_id.h" class SessionBackend; @@ -18,6 +19,10 @@ struct SessionTab; struct SessionWindow; +namespace base { +class RunLoop; +} + namespace sessions { class SerializedNavigationEntry; } @@ -76,6 +81,9 @@ SessionBackend* backend(); + void RunTaskOnBackendThread(const tracked_objects::Location& from_here, + const base::Closure& task); + private: scoped_ptr<SessionService> service_;
diff --git a/chrome/browser/sessions/session_service_unittest.cc b/chrome/browser/sessions/session_service_unittest.cc index a4f2a44..67836f19 100644 --- a/chrome/browser/sessions/session_service_unittest.cc +++ b/chrome/browser/sessions/session_service_unittest.cc
@@ -9,9 +9,11 @@ #include "base/memory/scoped_ptr.h" #include "base/memory/scoped_vector.h" #include "base/path_service.h" +#include "base/run_loop.h" #include "base/stl_util.h" #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" +#include "base/synchronization/waitable_event.h" #include "base/time/time.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/chrome_notification_types.h" @@ -164,8 +166,6 @@ SessionService* service() { return helper_.service(); } - SessionBackend* backend() { return helper_.backend(); } - const gfx::Rect window_bounds; SessionID window_id; @@ -999,3 +999,52 @@ helper_.AssertTabEquals(window_id, tab_id, 0, 0, 1, *tab); helper_.AssertNavigationEquals(nav1, tab->navigations[0]); } + +// Functions used by GetSessionsAndDestroy. +namespace { + +void OnGotPreviousSession(ScopedVector<SessionWindow> windows, + SessionID::id_type ignored_active_window) { + FAIL() << "SessionService was destroyed, this shouldn't be reached."; +} + +void PostBackToThread(base::MessageLoop* message_loop, + base::RunLoop* run_loop) { + message_loop->PostTask(FROM_HERE, + base::Bind(&base::RunLoop::Quit, + base::Unretained(run_loop))); +} + +} // namespace + +// Verifies that SessionService::GetLastSession() works correctly if the +// SessionService is deleted during processing. To verify the problematic case +// does the following: +// 1. Sends a task to the background thread that blocks. +// 2. Asks SessionService for the last session commands. This is blocked by 1. +// 3. Posts another task to the background thread, this too is blocked by 1. +// 4. Deletes SessionService. +// 5. Signals the semaphore that 2 and 3 are waiting on, allowing +// GetLastSession() to continue. +// 6. runs the message loop, this is quit when the task scheduled in 3 posts +// back to the ui thread to quit the run loop. +// The call to get the previous session should never be invoked because the +// SessionService was destroyed before SessionService could process the results. +TEST_F(SessionServiceTest, GetSessionsAndDestroy) { + base::CancelableTaskTracker cancelable_task_tracker; + base::RunLoop run_loop; + base::WaitableEvent event(true, false); + helper_.RunTaskOnBackendThread(FROM_HERE, + base::Bind(&base::WaitableEvent::Wait, + base::Unretained(&event))); + service()->GetLastSession(base::Bind(&OnGotPreviousSession), + &cancelable_task_tracker); + helper_.RunTaskOnBackendThread( + FROM_HERE, + base::Bind(&PostBackToThread, + base::Unretained(base::MessageLoop::current()), + base::Unretained(&run_loop))); + delete helper_.ReleaseService(); + event.Signal(); + run_loop.Run(); +}