Crash when taking screenshot onto Chrome OS Google Drive.

And, Don't stack screenshot notifications.

Changed profile handling. The profile is now retrieved everytime a screenshot is taken and
not only once in Ash init. The reason is that the profile can change, which caused this crash.

Changed behavior for a subsequent screenshot notification. It will replace the previous
notification (rather than add a new one).

When saving to Google Drive we have two path: screenshot path (used in notification) and
local path (for internal cash/work; used in writing). Add proper handling of both paths.


BUG=231707, 232876, 233574
R=jamescook@chromium.org
TEST=manual

Review URL: https://chromiumcodereview.appspot.com/14027009

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

chrome/browser/ui/ash/ash_init.cc

@@ -17,7 +17,6 @@
 #include "chrome/browser/chromeos/accessibility/accessibility_util.h"
 #include "chrome/browser/chromeos/accessibility/magnification_manager.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
-#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/ash/chrome_shell_delegate.h"
 #include "chrome/browser/ui/ash/event_rewriter.h"
 #include "chrome/browser/ui/ash/screenshot_taker.h"
@@ -69,9 +68,7 @@ void OpenAsh() {
   shell->event_rewriter_filter()->SetEventRewriterDelegate(
       scoped_ptr<ash::EventRewriterDelegate>(new EventRewriter).Pass());
   shell->accelerator_controller()->SetScreenshotDelegate(
-      scoped_ptr<ash::ScreenshotDelegate>(
-          new ScreenshotTaker(
-              ProfileManager::GetDefaultProfileOrOffTheRecord())).Pass());
+      scoped_ptr<ash::ScreenshotDelegate>(new ScreenshotTaker).Pass());
 #if defined(OS_CHROMEOS)
   shell->accelerator_controller()->SetBrightnessControlDelegate(
       scoped_ptr<ash::BrightnessControlDelegate>(

chrome/browser/ui/ash/screenshot_taker.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/notifications/notification.h"
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/webui/screenshot_source.h"
 #include "chrome/browser/ui/window_snapshot/window_snapshot.h"
 #include "content/public/browser/browser_thread.h"
@@ -43,6 +44,8 @@ namespace {
 // more than 1000 to prevent the conflict of filenames.
 const int kScreenshotMinimumIntervalInMS = 1000;
 
+const char kNotificationId[] = "screenshot";
+
 const char kNotificationOriginUrl[] = "chrome://screenshot";
 
 // Delegate for a notification. This class has two roles: to implement callback
@@ -50,11 +53,9 @@ const char kNotificationOriginUrl[] = "chrome://screenshot";
 // notification.
 class ScreenshotTakerNotificationDelegate : public NotificationDelegate {
  public:
-  ScreenshotTakerNotificationDelegate(const std::string& id_text,
-                                      bool success,
+  ScreenshotTakerNotificationDelegate(bool success,
                                       const base::FilePath& screenshot_path)
-      : id_text_(id_text),
-        success_(success),
+      : success_(success),
         screenshot_path_(screenshot_path) {
   }
 
@@ -71,7 +72,9 @@ class ScreenshotTakerNotificationDelegate : public NotificationDelegate {
     // TODO(sschmitz): perhaps add similar action for Windows.
 #endif
   }
-  virtual std::string id() const OVERRIDE { return id_text_; }
+  virtual std::string id() const OVERRIDE {
+    return std::string(kNotificationId);
+  }
   virtual content::RenderViewHost* GetRenderViewHost() const OVERRIDE {
     return NULL;
   }
@@ -79,7 +82,6 @@ class ScreenshotTakerNotificationDelegate : public NotificationDelegate {
  private:
   virtual ~ScreenshotTakerNotificationDelegate() {}
 
-  const std::string id_text_;
   const bool success_;
   const base::FilePath screenshot_path_;
 
@@ -92,16 +94,17 @@ typedef base::Callback<
 
 void SaveScreenshotInternal(const ShowNotificationCallback& callback,
                             const base::FilePath& screenshot_path,
+                            const base::FilePath& local_path,
                             scoped_refptr<base::RefCountedBytes> png_data) {
   DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
-  DCHECK(!screenshot_path.empty());
+  DCHECK(!local_path.empty());
   ScreenshotTakerObserver::Result result =
       ScreenshotTakerObserver::SCREENSHOT_SUCCESS;
   if (static_cast<size_t>(file_util::WriteFile(
-          screenshot_path,
+          local_path,
           reinterpret_cast<char*>(&(png_data->data()[0])),
           png_data->size())) != png_data->size()) {
-    LOG(ERROR) << "Failed to save to " << screenshot_path.value();
+    LOG(ERROR) << "Failed to save to " << local_path.value();
     result = ScreenshotTakerObserver::SCREENSHOT_WRITE_FILE_FAILED;
   }
   content::BrowserThread::PostTask(
@@ -125,25 +128,29 @@ void SaveScreenshot(const ShowNotificationCallback& callback,
                    screenshot_path));
     return;
   }
-  SaveScreenshotInternal(callback, screenshot_path, png_data);
+  SaveScreenshotInternal(callback, screenshot_path, screenshot_path, png_data);
 }
 
 // TODO(kinaba): crbug.com/140425, remove this ungly #ifdef dispatch.
 #if defined(OS_CHROMEOS)
 void SaveScreenshotToDrive(const ShowNotificationCallback& callback,
+                           const base::FilePath& screenshot_path,
                            scoped_refptr<base::RefCountedBytes> png_data,
                            drive::DriveFileError error,
                            const base::FilePath& local_path) {
+  // |screenshot_path| is used in the notification callback.
+  // |local_path| is a temporary file in a hidden cache directory used for
+  // internal work generated by drive::util::PrepareWritableFileAndRun.
   if (error != drive::DRIVE_FILE_OK) {
     LOG(ERROR) << "Failed to write screenshot image to Google Drive: " << error;
     content::BrowserThread::PostTask(
         content::BrowserThread::UI, FROM_HERE,
         base::Bind(callback,
                    ScreenshotTakerObserver::SCREENSHOT_CREATE_FILE_FAILED,
-                   local_path));
+                   screenshot_path));
     return;
   }
-  SaveScreenshotInternal(callback, local_path, png_data);
+  SaveScreenshotInternal(callback, screenshot_path, local_path, png_data);
 }
 
 void EnsureDirectoryExistsCallback(
@@ -159,7 +166,10 @@ void EnsureDirectoryExistsCallback(
     drive::util::PrepareWritableFileAndRun(
         profile,
         screenshot_path,
-        base::Bind(&SaveScreenshotToDrive, callback, png_data));
+        base::Bind(&SaveScreenshotToDrive,
+                   callback,
+                   screenshot_path,
+                   png_data));
   } else {
     LOG(ERROR) << "Failed to ensure the existence of the specified directory "
                << "in Google Drive: " << error;
@@ -223,10 +233,9 @@ bool GrabWindowSnapshot(aura::Window* window,
 
 }  // namespace
 
-ScreenshotTaker::ScreenshotTaker(Profile* profile)
-    : profile_(profile),
-      ALLOW_THIS_IN_INITIALIZER_LIST(factory_(this)) {
-  DCHECK(profile_);
+ScreenshotTaker::ScreenshotTaker()
+    : ALLOW_THIS_IN_INITIALIZER_LIST(factory_(this)),
+      profile_for_test_(NULL) {
 }
 
 ScreenshotTaker::~ScreenshotTaker() {
@@ -265,7 +274,7 @@ void ScreenshotTaker::HandleTakeScreenshotForAllRootWindows() {
     if (GrabWindowSnapshot(root_window, rect, &png_data->data())) {
       PostSaveScreenshotTask(
           base::Bind(&ScreenshotTaker::ShowNotification, factory_.GetWeakPtr()),
-          profile_,
+          GetProfile(),
           screenshot_path,
           png_data);
     } else {
@@ -302,7 +311,7 @@ void ScreenshotTaker::HandleTakePartialScreenshot(
     last_screenshot_timestamp_ = base::Time::Now();
     PostSaveScreenshotTask(
         base::Bind(&ScreenshotTaker::ShowNotification, factory_.GetWeakPtr()),
-        profile_,
+        GetProfile(),
         screenshot_path,
         png_data);
   } else {
@@ -326,9 +335,11 @@ void ScreenshotTaker::ShowNotification(
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 #if defined(OS_CHROMEOS)
   // TODO(sschmitz): make this work for Windows.
-  static int id = 0;
-  std::string id_text = base::StringPrintf("screenshot_%3.3d", ++id);
-  string16 replace_id = UTF8ToUTF16(id_text);
+  const std::string notification_id(kNotificationId);
+  // We cancel a previous screenshot notification, if any, to ensure we get
+  // a fresh notification pop-up.
+  g_browser_process->notification_ui_manager()->CancelById(notification_id);
+  const string16 replace_id(UTF8ToUTF16(notification_id));
   bool success =
       (screenshot_result == ScreenshotTakerObserver::SCREENSHOT_SUCCESS);
   Notification notification(
@@ -346,10 +357,9 @@ void ScreenshotTaker::ShowNotification(
       WebKit::WebTextDirectionDefault,
       string16(),
       replace_id,
-      new ScreenshotTakerNotificationDelegate(id_text,
-                                              success,
-                                              screenshot_path));
-  g_browser_process->notification_ui_manager()->Add(notification, profile_);
+      new ScreenshotTakerNotificationDelegate(success, screenshot_path));
+  g_browser_process->notification_ui_manager()->Add(notification,
+                                                    GetProfile());
 #endif
 
   FOR_EACH_OBSERVER(ScreenshotTakerObserver, observers_,
@@ -368,12 +378,22 @@ bool ScreenshotTaker::HasObserver(ScreenshotTakerObserver* observer) const {
   return observers_.HasObserver(observer);
 }
 
+Profile* ScreenshotTaker::GetProfile() {
+  if (profile_for_test_)
+    return profile_for_test_;
+  return ProfileManager::GetDefaultProfileOrOffTheRecord();
+}
+
 void ScreenshotTaker::SetScreenshotDirectoryForTest(
     const base::FilePath& directory) {
   screenshot_directory_for_test_ = directory;
 }
 
 void ScreenshotTaker::SetScreenshotBasenameForTest(
-    const std::string& basename){
+    const std::string& basename) {
   screenshot_basename_for_test_ = basename;
 }
+
+void ScreenshotTaker::SetScreenshotProfileForTest(Profile* profile) {
+  profile_for_test_ = profile;
+}

chrome/browser/ui/ash/screenshot_taker.h

@@ -49,7 +49,7 @@ class ScreenshotTakerObserver {
 
 class ScreenshotTaker : public ash::ScreenshotDelegate {
  public:
-  explicit ScreenshotTaker(Profile* profile);
+  ScreenshotTaker();
 
   virtual ~ScreenshotTaker();
 
@@ -70,10 +70,10 @@ class ScreenshotTaker : public ash::ScreenshotDelegate {
  private:
   friend class ash::test::ScreenshotTakerTest;
 
+  Profile* GetProfile();
   void SetScreenshotDirectoryForTest(const base::FilePath& directory);
   void SetScreenshotBasenameForTest(const std::string& basename);
-
-  Profile* profile_;
+  void SetScreenshotProfileForTest(Profile* profile);
 
   base::WeakPtrFactory<ScreenshotTaker> factory_;
 
@@ -81,8 +81,10 @@ class ScreenshotTaker : public ash::ScreenshotDelegate {
   base::Time last_screenshot_timestamp_;
 
   ObserverList<ScreenshotTakerObserver> observers_;
+
   base::FilePath screenshot_directory_for_test_;
   std::string screenshot_basename_for_test_;
+  Profile* profile_for_test_;
 
   DISALLOW_COPY_AND_ASSIGN(ScreenshotTaker);
 };

chrome/browser/ui/ash/screenshot_taker_unittest.cc

@@ -63,7 +63,7 @@ class ScreenshotTakerTest : public AshTestBase,
 
  protected:
   // ScreenshotTakerTest is a friend of ScreenshotTaker and therefore
-  // allowed to set the directory and basename.
+  // allowed to set the directory, basename and profile.
   void SetScreenshotDirectoryForTest(
       ScreenshotTaker* screenshot_taker,
       const base::FilePath& screenshot_directory) {
@@ -74,6 +74,11 @@ class ScreenshotTakerTest : public AshTestBase,
       const std::string& screenshot_basename) {
     screenshot_taker->SetScreenshotBasenameForTest(screenshot_basename);
   }
+  void SetScreenshotProfileForTest(
+      ScreenshotTaker* screenshot_taker,
+      Profile* profile) {
+    screenshot_taker->SetScreenshotProfileForTest(profile);
+  }
 
   void Wait() {
     if (screenshot_complete_)
@@ -97,12 +102,13 @@ class ScreenshotTakerTest : public AshTestBase,
 
 TEST_F(ScreenshotTakerTest, TakeScreenshot) {
   TestingProfile profile;
-  ScreenshotTaker screenshot_taker(&profile);
+  ScreenshotTaker screenshot_taker;
   screenshot_taker.AddObserver(this);
   base::ScopedTempDir directory;
   ASSERT_TRUE(directory.CreateUniqueTempDir());
   SetScreenshotDirectoryForTest(&screenshot_taker, directory.path());
   SetScreenshotBasenameForTest(&screenshot_taker, "Screenshot");
+  SetScreenshotProfileForTest(&screenshot_taker, &profile);
 
   EXPECT_TRUE(screenshot_taker.CanTakeScreenshot());
 
@@ -116,7 +122,7 @@ TEST_F(ScreenshotTakerTest, TakeScreenshot) {
 #if defined(OS_CHROMEOS)
   // Screenshot notifications on Windows not yet turned on.
   EXPECT_TRUE(g_browser_process->notification_ui_manager()->DoesIdExist(
-                  std::string("screenshot_001")));
+                  std::string("screenshot")));
   g_browser_process->notification_ui_manager()->CancelAll();
 #endif