Use base::File for DebugDaemonClient::GetDebugLogs

chrome/browser/ui/webui/net_internals/net_internals_ui.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/webui/net_internals/net_internals_ui.h"
 
 #include <algorithm>
 #include <list>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_member.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/cancelable_task_tracker.h"
-#include "base/threading/worker_pool.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "chrome/browser/browsing_data/browsing_data_remover.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/io_thread.h"
 #include "chrome/browser/net/chrome_net_log.h"
 #include "chrome/browser/net/chrome_network_delegate.h"
@@ skipping 161 matching lines @@
       content::WebUIDataSource::Create(chrome::kChromeUINetInternalsHost);
 
   source->SetUseJsonJSFormatV2();
   source->SetDefaultResource(IDR_NET_INTERNALS_INDEX_HTML);
   source->AddResourcePath("index.js", IDR_NET_INTERNALS_INDEX_JS);
   source->SetJsonPath("strings.js");
   return source;
 }
 
 #if defined(OS_CHROMEOS)
-// Small helper class used to create temporary log file and pass its
-// handle and error status to callback.
-// Use case:
-// DebugLogFileHelper* helper = new DebugLogFileHelper();
-// base::WorkerPool::PostTaskAndReply(FROM_HERE,
-//     base::Bind(&DebugLogFileHelper::DoWork, base::Unretained(helper), ...),
-//     base::Bind(&DebugLogFileHelper::Reply, base::Owned(helper), ...),
-//     false);
-class DebugLogFileHelper {
- public:
-  typedef base::Callback<void(base::File file,
-                              const base::FilePath& file_path)>
-      DebugLogFileCallback;
-
-  DebugLogFileHelper() {}
-
-  ~DebugLogFileHelper() {}
-
-  void DoWork(const base::FilePath& fileshelf) {
-    const base::FilePath::CharType kLogFileName[] =
-        FILE_PATH_LITERAL("debug-log.tgz");
-
-    file_path_ = fileshelf.Append(kLogFileName);
-    file_path_ = logging::GenerateTimestampedName(file_path_,
-                                                  base::Time::Now());
-
-    int flags =  base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;
-    file_.Initialize(file_path_, flags);
-  }
-
-  void Reply(const DebugLogFileCallback& callback) {
-    DCHECK(!callback.is_null());
-    callback.Run(file_.Pass(), file_path_);
-  }
-
- private:
-  base::File file_;
-  base::FilePath file_path_;
-
-  DISALLOW_COPY_AND_ASSIGN(DebugLogFileHelper);
-};
-
 // Following functions are used for getting debug logs. Logs are
 // fetched from /var/log/* and put on the fileshelf.
 
 // Called once StoreDebugLogs is complete. Takes two parameters:
 // - log_path: where the log file was saved in the case of success;
 // - succeeded: was the log file saved successfully.
 typedef base::Callback<void(const base::FilePath& log_path,
                             bool succeded)> StoreDebugLogsCallback;
 
-// Closes file handle, so, should be called on the WorkerPool thread.
-void CloseDebugLogFile(base::File file) {
-  file.Close();
-}
-
-// Closes file handle and deletes debug log file, so, should be called
-// on the WorkerPool thread.
-void CloseAndDeleteDebugLogFile(base::File file,
-                                const base::FilePath& file_path) {
-  file.Close();
-  base::DeleteFile(file_path, false);
-}
-
 // Called upon completion of |WriteDebugLogToFile|. Closes file
 // descriptor, deletes log file in the case of failure and calls
 // |callback|.
 void WriteDebugLogToFileCompleted(const StoreDebugLogsCallback& callback,
-                                  base::File file,
                                   const base::FilePath& file_path,
                                   bool succeeded) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!succeeded) {
-    bool posted = base::WorkerPool::PostTaskAndReply(FROM_HERE,
-        base::Bind(&CloseAndDeleteDebugLogFile, Passed(&file), file_path),
-        base::Bind(callback, file_path, false), false);
+    bool posted = BrowserThread::PostBlockingPoolTaskAndReply(
+        FROM_HERE,
+        base::Bind(base::IgnoreResult(&base::DeleteFile), file_path, false),
+        base::Bind(callback, file_path, false));
     DCHECK(posted);
     return;
   }
-  bool posted = base::WorkerPool::PostTaskAndReply(FROM_HERE,
-      base::Bind(&CloseDebugLogFile, Passed(&file)),
-      base::Bind(callback, file_path, true), false);
-  DCHECK(posted);
+  callback.Run(file_path, true);
 }
 
 // Stores into |file_path| debug logs in the .tgz format. Calls
 // |callback| upon completion.
 void WriteDebugLogToFile(const StoreDebugLogsCallback& callback,
-                         base::File file,
+                         base::File* file,
                          const base::FilePath& file_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (!file.IsValid()) {
+  if (!file->IsValid()) {
     LOG(ERROR) <<
         "Can't create debug log file: " << file_path.AsUTF8Unsafe() << ", " <<
-        "error: " << file.error_details();
+        "error: " << file->error_details();
     return;
   }
   chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->GetDebugLogs(
-      file.GetPlatformFile(),
-      base::Bind(&WriteDebugLogToFileCompleted,
-                 callback, Passed(&file), file_path));
+      file->Pass(),

[mmenke, 2014/05/08 14:29:34]

Erm...I don't see Pass() defined anywhere for a base::File.

[hashimoto, 2014/05/08 15:13:12]

Like scoped_ptr, base::File contains MOVE_ONLY_TYPE_FOR_CPP_03 in its class
definition so it comes with Pass() method somehow implicitly.

[mmenke, 2014/05/08 15:43:50]

I'm having trouble understanding their explanation, but it looks to me like
we're leaking the file* (Though we are moving its internals correctly).  It
should be passed as a base::Owned, not base::Unretained, no?

[hashimoto, 2014/05/08 15:59:51]

StoreDebugLogs() below runs base::File::Initialize() on the blocking pool with
base::Unretained and runs WriteDebugLogToFile() on the UI thread with the
initialized base::File passed with base::Owned.
base::File object is kept alive until WriteDebugLogToFile() finishes and it gets
correctly destroyed after that.

+      base::Bind(&WriteDebugLogToFileCompleted, callback, file_path));
 }
 
 // Stores debug logs in the .tgz archive on the |fileshelf|. The file
 // is created on the worker pool, then writing to it is triggered from
 // the UI thread, and finally it is closed (on success) or deleted (on
 // failure) on the worker pool, prior to calling |callback|.
 void StoreDebugLogs(const base::FilePath& fileshelf,
                     const StoreDebugLogsCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!callback.is_null());
-  DebugLogFileHelper* helper = new DebugLogFileHelper();
-  bool posted = base::WorkerPool::PostTaskAndReply(FROM_HERE,
-      base::Bind(&DebugLogFileHelper::DoWork,
-          base::Unretained(helper), fileshelf),
-      base::Bind(&DebugLogFileHelper::Reply, base::Owned(helper),
-          base::Bind(&WriteDebugLogToFile, callback)), false);
+
+  const base::FilePath::CharType kLogFileName[] =
+      FILE_PATH_LITERAL("debug-log.tgz");
+
+  base::FilePath file_path = fileshelf.Append(kLogFileName);
+  file_path = logging::GenerateTimestampedName(file_path, base::Time::Now());
+
+  int flags =  base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;
+  base::File* file = new base::File;
+  bool posted = BrowserThread::PostBlockingPoolTaskAndReply(
+      FROM_HERE,
+      base::Bind(&base::File::Initialize,
+                 base::Unretained(file), file_path, flags),
+      base::Bind(&WriteDebugLogToFile, callback, base::Owned(file), file_path));
   DCHECK(posted);
 }
 #endif  // defined(OS_CHROMEOS)
 
 // This class receives javascript messages from the renderer.
 // Note that the WebUI infrastructure runs on the UI thread, therefore all of
 // this class's methods are expected to run on the UI thread.
 //
 // Since the network code we want to run lives on the IO thread, we proxy
 // almost everything over to NetInternalsMessageHandler::IOThreadImpl, which
@@ skipping 1547 matching lines @@
 }
 
 NetInternalsUI::NetInternalsUI(content::WebUI* web_ui)
     : WebUIController(web_ui) {
   web_ui->AddMessageHandler(new NetInternalsMessageHandler());
 
   // Set up the chrome://net-internals/ source.
   Profile* profile = Profile::FromWebUI(web_ui);
   content::WebUIDataSource::Add(profile, CreateNetInternalsHTMLSource());
 }