Fixes possible crash in SessionBackend. I believe what's happening

here is we fail to create the file, and so current_session_file_.get()
is NULL and we crash. current_session_file_.get() is NULL if
OpenAndWriteHeader returns NULL (which is does if the full header
isn't written correctly.

Here's how I'm changing the code:
. The file is now truncated instead of closed/reopened. Hopefully this avoids the possibility of a scanner locking the file and the delete failing.
. Added a unit test for coverage of truncation.
. The file is opened in exclusive access. There is no reason why a scanner should open this file.
. Added null checks.

BUG=8476
TEST=none

Review URL: http://codereview.chromium.org/39275

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@11262 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 62 insertions, 7 deletions

diff --git a/chrome/browser/sessions/session_backend.cc b/chrome/browser/sessions/session_backend.cc
index ebadc31..13fe522 100644
--- a/chrome/browser/sessions/session_backend.cc
+++ b/chrome/browser/sessions/session_backend.cc
@@ -213,9 +213,14 @@ void SessionBackend::AppendCommands(
     std::vector<SessionCommand*>* commands,
     bool reset_first) {
   Init();
-  if ((reset_first && !empty_file_) || !current_session_file_->IsOpen())
+  // Make sure and check current_session_file_, if opening the file failed
+  // current_session_file_ will be NULL.
+  if ((reset_first && !empty_file_) || !current_session_file_.get() ||
+      !current_session_file_->IsOpen()) {
     ResetFile();
-  if (current_session_file_->IsOpen() &&
+  }
+  // Need to check current_session_file_ again, ResetFile may fail.
+  if (current_session_file_.get() && current_session_file_->IsOpen() &&
       !AppendCommandsToFile(current_session_file_.get(), *commands)) {
     current_session_file_.reset(NULL);
   }
@@ -313,7 +318,16 @@ bool SessionBackend::AppendCommandsToFile(net::FileStream* file,
 
 void SessionBackend::ResetFile() {
   DCHECK(inited_);
-  current_session_file_.reset(OpenAndWriteHeader(GetCurrentSessionPath()));
+  if (current_session_file_.get()) {
+    // File is already open, truncate it. We truncate instead of closing and
+    // reopening to avoid the possibility of scanners locking the file out
+    // from under us once we close it. If truncation fails, we'll try to
+    // recreate.
+    if (current_session_file_->Truncate(sizeof_header()) != sizeof_header())
+      current_session_file_.reset(NULL);
+  }
+  if (!current_session_file_.get())
+    current_session_file_.reset(OpenAndWriteHeader(GetCurrentSessionPath()));
   empty_file_ = true;
 }
 
@@ -321,7 +335,8 @@ net::FileStream* SessionBackend::OpenAndWriteHeader(const FilePath& path) {
   DCHECK(!path.empty());
   net::FileStream* file = new net::FileStream();
   file->Open(path, base::PLATFORM_FILE_CREATE_ALWAYS |
-             base::PLATFORM_FILE_WRITE);
+             base::PLATFORM_FILE_WRITE | base::PLATFORM_FILE_EXCLUSIVE_WRITE |
+             base::PLATFORM_FILE_EXCLUSIVE_READ);
   if (!file->IsOpen())
     return NULL;
   int32 header[2];
@@ -329,7 +344,7 @@ net::FileStream* SessionBackend::OpenAndWriteHeader(const FilePath& path) {
   header[1] = kFileCurrentVersion;
   int wrote = file->Write(reinterpret_cast<char*>(&header),
                           sizeof(header), NULL);
-  if (wrote != sizeof(header))
+  if (wrote != sizeof_header())
     return NULL;
   return file;
 }
diff --git a/chrome/browser/sessions/session_backend.h b/chrome/browser/sessions/session_backend.h
index 496a3ec..bfd446f 100644
--- a/chrome/browser/sessions/session_backend.h
+++ b/chrome/browser/sessions/session_backend.h
@@ -81,8 +81,12 @@ class SessionBackend : public base::RefCountedThreadSafe<SessionBackend> {
   void MoveCurrentSessionToLastSession();
 
  private:
-  // Recreates the current file such that it only contains the header and
-  // NO commands.
+  // If current_session_file_ is open, it is truncated so that it is essentially
+  // empty (only contains the header). If current_session_file_ isn't open, it
+  // is is opened and the header is written to it. After this
+  // current_session_file_ contains no commands.
+  // NOTE: current_session_file_ may be NULL if the file couldn't be opened or
+  // the header couldn't be written.
   void ResetFile();
 
   // Opens the current file and writes the header. On success a handle to
@@ -93,6 +97,13 @@ class SessionBackend : public base::RefCountedThreadSafe<SessionBackend> {
   bool AppendCommandsToFile(net::FileStream* file,
                             const std::vector<SessionCommand*>& commands);
 
+  // Returns the size of the header. The header is the first bytes written to
+  // the file, and is used to identify the file as one written by us.
+  int32 sizeof_header() const {
+    int32 header[2];
+    return sizeof(header);
+  }
+
   const BaseSessionService::SessionType type_;
 
   // Returns the path to the last file.
diff --git a/chrome/browser/sessions/session_backend_unittest.cc b/chrome/browser/sessions/session_backend_unittest.cc
index fb424f2..e5ba17d4 100644
--- a/chrome/browser/sessions/session_backend_unittest.cc
+++ b/chrome/browser/sessions/session_backend_unittest.cc
@@ -180,3 +180,32 @@ TEST_F(SessionBackendTest, EmptyCommand) {
   AssertCommandEqualsData(empty_command, commands[0]);
   STLDeleteElements(&commands);
 }
+
+// Writes a command, appends another command with reset to true, then reads
+// making sure we only get back the second command.
+TEST_F(SessionBackendTest, Truncate) {
+  scoped_refptr<SessionBackend> backend(
+      new SessionBackend(BaseSessionService::SESSION_RESTORE, path_));
+  struct TestData first_data = { 1,  "a" };
+  std::vector<SessionCommand*> commands;
+  commands.push_back(CreateCommandFromData(first_data));
+  backend->AppendCommands(new SessionCommands(commands), false);
+  commands.clear();
+
+  // Write another command, this time resetting the file when appending.
+  struct TestData second_data = { 2,  "b" };
+  commands.push_back(CreateCommandFromData(second_data));
+  backend->AppendCommands(new SessionCommands(commands), true);
+  commands.clear();
+
+  // Read it back in.
+  backend = NULL;
+  backend = new SessionBackend(BaseSessionService::SESSION_RESTORE, path_);
+  backend->ReadLastSessionCommandsImpl(&commands);
+
+  // And make sure we get back the expected data.
+  ASSERT_EQ(1U, commands.size());
+  AssertCommandEqualsData(second_data, commands[0]);
+
+  STLDeleteElements(&commands);
+}