Crash when taking screenshot onto Chrome OS Google Drive

chrome/browser/ui/ash/screenshot_taker.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/ash/screenshot_taker.h"
 
 #include <climits>
 #include <string>
 
 #include "ash/shell.h"
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/stringprintf.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_prefs.h"
 #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"
 #include "grit/ash_strings.h"
 #include "grit/theme_resources.h"
 #include "ui/aura/root_window.h"
 #include "ui/aura/window.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/image/image.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/drive/drive_file_system_util.h"
 #include "chrome/browser/chromeos/extensions/file_manager/file_manager_util.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #endif
 
 namespace {
 // The minimum interval between two screenshot commands.  It has to be
 // more than 1000 to prevent the conflict of filenames.
 const int kScreenshotMinimumIntervalInMS = 1000;
 
 const char kNotificationOriginUrl[] = "chrome://screenshot";
 
 // Delegate for a notification. This class has two roles: to implement callback
 // methods for notification, and to provide an identity of the associated
 // notification.
 class ScreenshotTakerNotificationDelegate : public NotificationDelegate {
  public:
-  ScreenshotTakerNotificationDelegate(const std::string& id_text,
+  ScreenshotTakerNotificationDelegate(const std::string& notification_id,
                                       bool success,
                                       const base::FilePath& screenshot_path)
-      : id_text_(id_text),
+      : notification_id_(notification_id),
         success_(success),
         screenshot_path_(screenshot_path) {
   }
 
   // Overridden from NotificationDelegate:
   virtual void Display() OVERRIDE {}
   virtual void Error() OVERRIDE {}
   virtual void Close(bool by_user) OVERRIDE {}
   virtual void Click() OVERRIDE {
     if (!success_)
       return;
 #if defined(OS_CHROMEOS)
     file_manager_util::ShowFileInFolder(screenshot_path_);
 #else
     // 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 notification_id_; }
   virtual content::RenderViewHost* GetRenderViewHost() const OVERRIDE {
     return NULL;
   }
 
  private:
   virtual ~ScreenshotTakerNotificationDelegate() {}
 
-  const std::string id_text_;
+  const std::string notification_id_;
   const bool success_;
   const base::FilePath screenshot_path_;
 
   DISALLOW_COPY_AND_ASSIGN(ScreenshotTakerNotificationDelegate);
 };
 
 typedef base::Callback<
   void(ScreenshotTakerObserver::Result screenshot_result,
        const base::FilePath& screenshot_path)> ShowNotificationCallback;
 
 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(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(callback, result, screenshot_path));
 }
 
 void SaveScreenshot(const ShowNotificationCallback& callback,
                     const base::FilePath& screenshot_path,
                     scoped_refptr<base::RefCountedBytes> png_data) {
   DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   DCHECK(!screenshot_path.empty());
 
   if (!file_util::CreateDirectory(screenshot_path.DirName())) {
     LOG(ERROR) << "Failed to ensure the existence of "
                << screenshot_path.DirName().value();
     content::BrowserThread::PostTask(
         content::BrowserThread::UI, FROM_HERE,
         base::Bind(callback,
                    ScreenshotTakerObserver::SCREENSHOT_CREATE_DIR_FAILED,
                    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 cash directory used for

[James Cook, 2013/04/19 17:25:34]

cash -> cache

[sschmitz, 2013/04/19 17:53:14]

Done.

+  // 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(
     const ShowNotificationCallback& callback,
     Profile* profile,
     const base::FilePath& screenshot_path,
     scoped_refptr<base::RefCountedBytes> png_data,
     drive::DriveFileError error) {
   // It is okay to fail with DRIVE_FILE_ERROR_EXISTS since anyway the directory
   // of the target file exists.
   if (error == drive::DRIVE_FILE_OK ||
       error == drive::DRIVE_FILE_ERROR_EXISTS) {
     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;
     callback.Run(ScreenshotTakerObserver::SCREENSHOT_CHECK_DIR_FAILED,
                  screenshot_path);
   }
 }
 
 void PostSaveScreenshotTask(const ShowNotificationCallback& callback,
                             Profile* profile,
@@ skipping 43 matching lines @@
   // TODO(mukai): remove this when the performance issue has been fixed.
   if (window->GetRootWindow()->GrabSnapshot(snapshot_bounds, png_data))
     return true;
 #endif  // OS_LINUX
 
   return chrome::GrabWindowSnapshotForUser(window, png_data, snapshot_bounds);
 }
 
 }  // 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() {
 }
 
 void ScreenshotTaker::HandleTakeScreenshotForAllRootWindows() {
   base::FilePath screenshot_directory;
   if (!screenshot_directory_for_test_.empty()) {
     screenshot_directory = screenshot_directory_for_test_;
   } else if (!ScreenshotSource::GetScreenshotDirectory(&screenshot_directory)) {
@@ skipping 18 matching lines @@
     scoped_refptr<base::RefCountedBytes> png_data(new base::RefCountedBytes);
     std::string basename = screenshot_basename;
     gfx::Rect rect = root_window->bounds();
     if (root_windows.size() > 1)
       basename += base::StringPrintf(" - Display %d", static_cast<int>(i + 1));
     base::FilePath screenshot_path =
         screenshot_directory.AppendASCII(basename + ".png");
     if (GrabWindowSnapshot(root_window, rect, &png_data->data())) {
       PostSaveScreenshotTask(
           base::Bind(&ScreenshotTaker::ShowNotification, factory_.GetWeakPtr()),
-          profile_,
+          GetProfile(),
           screenshot_path,
           png_data);
     } else {
       LOG(ERROR) << "Failed to grab the window screenshot for " << i;
       ShowNotification(
           ScreenshotTakerObserver::SCREENSHOT_GRABWINDOW_FULL_FAILED,
           screenshot_path);
     }
   }
   last_screenshot_timestamp_ = base::Time::Now();
@@ skipping 16 matching lines @@
 
   std::string screenshot_basename = !screenshot_basename_for_test_.empty() ?
       screenshot_basename_for_test_ :
       ScreenshotSource::GetScreenshotBaseFilename();
   base::FilePath screenshot_path =
       screenshot_directory.AppendASCII(screenshot_basename + ".png");
   if (GrabWindowSnapshot(window, rect, &png_data->data())) {
     last_screenshot_timestamp_ = base::Time::Now();
     PostSaveScreenshotTask(
         base::Bind(&ScreenshotTaker::ShowNotification, factory_.GetWeakPtr()),
-        profile_,
+        GetProfile(),
         screenshot_path,
         png_data);
   } else {
     LOG(ERROR) << "Failed to grab the window screenshot";
     ShowNotification(
         ScreenshotTakerObserver::SCREENSHOT_GRABWINDOW_PARTIAL_FAILED,
         screenshot_path);
   }
 }
 
 bool ScreenshotTaker::CanTakeScreenshot() {
   return last_screenshot_timestamp_.is_null() ||
       base::Time::Now() - last_screenshot_timestamp_ >
       base::TimeDelta::FromMilliseconds(
           kScreenshotMinimumIntervalInMS);
 }
 
 void ScreenshotTaker::ShowNotification(
     ScreenshotTakerObserver::Result screenshot_result,
     const base::FilePath& screenshot_path) {
   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("screenshot");
+  g_browser_process->notification_ui_manager()->CancelById(notification_id);

[James Cook, 2013/04/19 17:25:34]

Do you still need to cancel the old notification if you're showing one with the
same replace_id?  If not, don't do it.  If yes, this could use a comment.

[sschmitz, 2013/04/19 17:53:14]

Currently yes for two reasons. This may or may not change in the future.
1. to avoid crash that mukai@ is fixing
2. to popup a notification. otherwise nothing happends. This may change in the
future

Added a comment.

+  const string16 replace_id(UTF8ToUTF16(notification_id));
   bool success =
       (screenshot_result == ScreenshotTakerObserver::SCREENSHOT_SUCCESS);
   Notification notification(
       GURL(kNotificationOriginUrl),
       ui::ResourceBundle::GetSharedInstance().GetImageNamed(
           IDR_SCREENSHOT_NOTIFICATION_ICON),
       l10n_util::GetStringUTF16(
           success ?
           IDS_ASH_SCREENSHOT_NOTIFICATION_TITLE_SUCCESS :
           IDS_ASH_SCREENSHOT_NOTIFICATION_TITLE_FAIL),
       l10n_util::GetStringUTF16(
           success ?
           IDS_ASH_SCREENSHOT_NOTIFICATION_TEXT_SUCCESS :
           IDS_ASH_SCREENSHOT_NOTIFICATION_TEXT_FAIL),
       WebKit::WebTextDirectionDefault,
       string16(),
       replace_id,
-      new ScreenshotTakerNotificationDelegate(id_text,
+      new ScreenshotTakerNotificationDelegate(notification_id,

[James Cook, 2013/04/19 17:25:34]

Why not just make the notification ID string a constant and avoid passing it
around?

(It'll need to be a const char[] - you can't construct std::string as a
static/global.)

[sschmitz, 2013/04/19 17:53:14]

Done.

                                               success,
                                               screenshot_path));
-  g_browser_process->notification_ui_manager()->Add(notification, profile_);
+  g_browser_process->notification_ui_manager()->Add(notification,
+                                                    GetProfile());
 #endif
 
   FOR_EACH_OBSERVER(ScreenshotTakerObserver, observers_,
                     OnScreenshotCompleted(screenshot_result, screenshot_path));
 }
 
 void ScreenshotTaker::AddObserver(ScreenshotTakerObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 void ScreenshotTaker::RemoveObserver(ScreenshotTakerObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
 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;
+}