Fix sending multiple feedback reports within short durations of each other

chrome/browser/extensions/api/feedback_private/feedback_service.cc

Index: chrome/browser/extensions/api/feedback_private/feedback_service.cc
diff --git a/chrome/browser/extensions/api/feedback_private/feedback_service.cc b/chrome/browser/extensions/api/feedback_private/feedback_service.cc
index 28a72c6b9329ac0d3cbefbe64b148e5a48487a93..36ab902b27216786d1ce8c5f70674ad1960bf0a9 100644
--- a/chrome/browser/extensions/api/feedback_private/feedback_service.cc
+++ b/chrome/browser/extensions/api/feedback_private/feedback_service.cc
@@ -8,6 +8,7 @@
 
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
@@ -17,10 +18,15 @@
 using content::BrowserThread;
 using feedback::FeedbackData;
 
+namespace extensions {
+
 namespace {
 
-void PopulateSystemInfo(
-    extensions::SystemInformationList* sys_info_list,
+// Give unique IDs to each system information request, and send report request.
+FeedbackService::RequestId g_last_sys_info_request_Id = 0;
+FeedbackService::RequestId g_last_send_feedback_request_Id = 0;
+
+void PopulateSystemInfo(SystemInformationList* sys_info_list,
     const std::string& key,
     const std::string& value) {
   base::DictionaryValue sys_info_value;
@@ -35,7 +41,13 @@ void PopulateSystemInfo(
 
 }  // namespace
 
-namespace extensions {
+// Represents a request to send a feedback report. It is used to be able to
+// handle multiple send requests.
+struct FeedbackService::SendRequest {
+  RequestId request_id;
+  scoped_refptr<feedback::FeedbackData> feedback_data;
+  SendFeedbackCallback on_sent_callback;
+};
 
 FeedbackService::FeedbackService() {
 }
@@ -47,75 +59,95 @@ void FeedbackService::SendFeedback(
     Profile* profile,
     scoped_refptr<FeedbackData> feedback_data,
     const SendFeedbackCallback& callback) {
-  send_feedback_callback_ = callback;
-  feedback_data_ = feedback_data;
-  feedback_data_->set_locale(g_browser_process->GetApplicationLocale());
-  feedback_data_->set_user_agent(GetUserAgent());
+  // Give every send request a unique Id.
+  const RequestId id = ++g_last_send_feedback_request_Id;
+  SendRequest& request = send_feedback_requests_[id];
 
-  if (!feedback_data_->attached_file_uuid().empty()) {
+  request.request_id = id;
+  request.on_sent_callback = callback;
+  request.feedback_data = feedback_data;
+  request.feedback_data->set_locale(g_browser_process->GetApplicationLocale());
+  request.feedback_data->set_user_agent(GetUserAgent());
+
+  if (!request.feedback_data->attached_file_uuid().empty()) {
     // Self-deleting object.
     BlobReader* attached_file_reader = new BlobReader(
-        profile, feedback_data_->attached_file_uuid(),
-        base::Bind(&FeedbackService::AttachedFileCallback, AsWeakPtr()));
+        profile, request.feedback_data->attached_file_uuid(),
+        base::Bind(&FeedbackService::AttachedFileCallback, AsWeakPtr(), id));

[xiyuan, 2016/03/14 23:47:28]

It seems to me that we can bind the needed context via base::Bind and get rid of
|system_information_callbacks_| and |send_feedback_requests_|.

[afakhry, 2016/03/15 19:25:38]

I agree. Great idea. But I think we can only do this with
system_information_callbacks_. For the send requests however, we need a place to
store the different feedback_data of each report, that can be referred to and
modified by either or both of the corresponding callbacks
FeedbackService::AttachedFileCallback() and
FeedbackService::ScreenshotCallback().

[xiyuan, 2016/03/15 19:54:06]

feedback_data is a ref-counted object. It would be available as long as there is
an outstanding reference. base::Bind would cause a reference added to it and
FeedbackService::AttachedFileCallback can take it as a scoped_refptr<>.

i.e.

...
    base::Bind(&FeedbackService::AttachedFileCallback, AsWeakPtr(),
               feedback_data));
...

void FeedbackService::AttachedFileCallback(
    scoped_refptr<FeedbackData> feedback_data,
    scoped_ptr<std::string> data,
    int64_t /* total_blob_length */) {
  ...
}

[afakhry, 2016/03/16 00:45:34]

Right, right. Sorry I forgot that FeedbackData is RefCounted. This definitely
makes it much simpler. FeedbackService doesn't have any state now, we can
convert it to a bunch of static methods, and get rid of the SupportsWeakPtr.
What do you think?

[xiyuan, 2016/03/16 16:39:28]

Up to you. I am okay to do it now or in a later CL.

[afakhry, 2016/03/16 17:51:21]

Let's keep the scope of this CL small and do it in a followup CL.

     attached_file_reader->Start();
   }
 
-  if (!feedback_data_->screenshot_uuid().empty()) {
+  if (!request.feedback_data->screenshot_uuid().empty()) {
     // Self-deleting object.
     BlobReader* screenshot_reader = new BlobReader(
-        profile, feedback_data_->screenshot_uuid(),
-        base::Bind(&FeedbackService::ScreenshotCallback, AsWeakPtr()));
+        profile, request.feedback_data->screenshot_uuid(),
+        base::Bind(&FeedbackService::ScreenshotCallback, AsWeakPtr(), id));
     screenshot_reader->Start();
   }
 
-  CompleteSendFeedback();
+  CompleteSendFeedback(id);
 }
 
-void FeedbackService::AttachedFileCallback(scoped_ptr<std::string> data,
+void FeedbackService::AttachedFileCallback(RequestId request_id,
+                                           scoped_ptr<std::string> data,
                                            int64_t /* total_blob_length */) {
-  feedback_data_->set_attached_file_uuid(std::string());
+  DCHECK(ContainsKey(send_feedback_requests_, request_id));
+
+  SendRequest& request = send_feedback_requests_[request_id];
+  request.feedback_data->set_attached_file_uuid(std::string());
   if (data)
-    feedback_data_->AttachAndCompressFileData(std::move(data));
+    request.feedback_data->AttachAndCompressFileData(std::move(data));
 
-  CompleteSendFeedback();
+  CompleteSendFeedback(request_id);
 }
 
-void FeedbackService::ScreenshotCallback(scoped_ptr<std::string> data,
+void FeedbackService::ScreenshotCallback(RequestId request_id,
+                                         scoped_ptr<std::string> data,
                                          int64_t /* total_blob_length */) {
-  feedback_data_->set_screenshot_uuid(std::string());
+  DCHECK(ContainsKey(send_feedback_requests_, request_id));
+
+  SendRequest& request = send_feedback_requests_[request_id];
+  request.feedback_data->set_screenshot_uuid(std::string());
   if (data)
-    feedback_data_->set_image(std::move(data));
+    request.feedback_data->set_image(std::move(data));
 
-  CompleteSendFeedback();
+  CompleteSendFeedback(request_id);
 }
 
 void FeedbackService::GetSystemInformation(
     const GetSystemInformationCallback& callback) {
-  system_information_callback_ = callback;
+  // Give every system information request a unique ID.
+  const RequestId id = ++g_last_sys_info_request_Id;
+
+  system_information_callbacks_[id] = callback;
 
   system_logs::ScrubbedSystemLogsFetcher* fetcher =
       new system_logs::ScrubbedSystemLogsFetcher();
   fetcher->Fetch(
-      base::Bind(&FeedbackService::OnSystemLogsFetchComplete, AsWeakPtr()));
+      base::Bind(&FeedbackService::OnSystemLogsFetchComplete, AsWeakPtr(), id));
 }
 
 
 void FeedbackService::OnSystemLogsFetchComplete(
+    RequestId request_id,
     scoped_ptr<system_logs::SystemLogsResponse> sys_info_map) {
+  DCHECK(ContainsKey(system_information_callbacks_, request_id));
+
+  auto& callback = system_information_callbacks_[request_id];
+
   SystemInformationList sys_info_list;
-  if (!sys_info_map.get()) {
-    system_information_callback_.Run(sys_info_list);
-    return;
+  if (sys_info_map.get()) {
+    for (const auto& itr : *sys_info_map)
+      PopulateSystemInfo(&sys_info_list, itr.first, itr.second);
   }
 
-  for (system_logs::SystemLogsResponse::iterator it = sys_info_map->begin();
-       it != sys_info_map->end(); ++it)
-    PopulateSystemInfo(&sys_info_list, it->first, it->second);
+  callback.Run(sys_info_list);
 
-  system_information_callback_.Run(sys_info_list);
+  // We're done with this request.
+  system_information_callbacks_.erase(request_id);
 }
 
-void FeedbackService::CompleteSendFeedback() {
+void FeedbackService::CompleteSendFeedback(RequestId request_id) {
   // A particular data collection is considered completed if,
   // a.) The blob URL is invalid - this will either happen because we never had
   //     a URL and never needed to read this data, or that the data read failed
@@ -123,16 +155,25 @@ void FeedbackService::CompleteSendFeedback() {
   // b.) The associated data object exists, meaning that the data has been read
   //     and the read callback has updated the associated data on the feedback
   //     object.
-  bool attached_file_completed = feedback_data_->attached_file_uuid().empty();
-  bool screenshot_completed = feedback_data_->screenshot_uuid().empty();
+  DCHECK(ContainsKey(send_feedback_requests_, request_id));
+
+  SendRequest& request = send_feedback_requests_[request_id];
+  const bool attached_file_completed =
+      request.feedback_data->attached_file_uuid().empty();
+  const bool screenshot_completed =
+      request.feedback_data->screenshot_uuid().empty();
 
   if (screenshot_completed && attached_file_completed) {
     // Signal the feedback object that the data from the feedback page has been
     // filled - the object will manage sending of the actual report.
-    feedback_data_->OnFeedbackPageDataComplete();
+    request.feedback_data->OnFeedbackPageDataComplete();
+
     // TODO(rkc): Change this once we have FeedbackData/Util refactored to
     // report the status of the report being sent.
-    send_feedback_callback_.Run(true);
+    request.on_sent_callback.Run(true);
+
+    // We're done with this request.
+    send_feedback_requests_.erase(request_id);
   }
 }