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}

7 files changed, 76 insertions, 11 deletions

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 @@ BaseSessionService::ScheduleGetLastSessionCommands(
   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 e85799a..98f2379 100644
--- a/chrome/browser/sessions/base_session_service.h
+++ b/chrome/browser/sessions/base_session_service.h
@@ -156,7 +156,7 @@ class BaseSessionService {
 
   // 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 7b36c75..0a0ac5a 100644
--- a/chrome/browser/sessions/session_service.cc
+++ b/chrome/browser/sessions/session_service.cc
@@ -234,7 +234,8 @@ SessionService::SessionService(Profile* profile)
       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 @@ SessionService::SessionService(const base::FilePath& save_path)
       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 @@ base::CancelableTaskTracker::TaskId SessionService::GetLastSession(
   // 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 @@ class SessionService : public BaseSessionService,
   // 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 e9ee6da..0a1f982 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 @@ void SessionServiceTestHelper::SetService(SessionService* service) {
   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 7f176c7..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 @@ class SessionService;
 struct SessionTab;
 struct SessionWindow;
 
+namespace base {
+class RunLoop;
+}
+
 namespace sessions {
 class SerializedNavigationEntry;
 }
@@ -76,6 +81,9 @@ class SessionServiceTestHelper {
 
   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..67836f1 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 @@ class SessionServiceTest : public BrowserWithTestWindowTest,
 
   SessionService* service() { return helper_.service(); }
 
-  SessionBackend* backend() { return helper_.backend(); }
-
   const gfx::Rect window_bounds;
 
   SessionID window_id;
@@ -999,3 +999,52 @@ TEST_F(SessionServiceTest, IgnoreBlacklistedUrls) {
   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();
+}