Fix sending multiple feedback reports within short durations of each other

Those reports needed to be queued nicely and each report should be sent
with its correct respective system information not the one of the previous
report.

BUG=593452
Test=manually

Review URL: https://codereview.chromium.org/1794513002

Cr-Commit-Position: refs/heads/master@{#381768}

4 files changed, 216 insertions, 205 deletions

diff --git a/chrome/browser/extensions/api/feedback_private/feedback_service.cc b/chrome/browser/extensions/api/feedback_private/feedback_service.cc
index 28a72c6..9bd1e5f 100644
--- a/chrome/browser/extensions/api/feedback_private/feedback_service.cc
+++ b/chrome/browser/extensions/api/feedback_private/feedback_service.cc
@@ -17,12 +17,13 @@
 using content::BrowserThread;
 using feedback::FeedbackData;
 
+namespace extensions {
+
 namespace {
 
-void PopulateSystemInfo(
-    extensions::SystemInformationList* sys_info_list,
-    const std::string& key,
-    const std::string& value) {
+void PopulateSystemInfo(SystemInformationList* sys_info_list,
+                        const std::string& key,
+                        const std::string& value) {
   base::DictionaryValue sys_info_value;
   sys_info_value.Set("key", new base::StringValue(key));
   sys_info_value.Set("value", new base::StringValue(value));
@@ -35,8 +36,6 @@ void PopulateSystemInfo(
 
 }  // namespace
 
-namespace extensions {
-
 FeedbackService::FeedbackService() {
 }
 
@@ -47,75 +46,77 @@ 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());
+  feedback_data->set_locale(g_browser_process->GetApplicationLocale());
+  feedback_data->set_user_agent(GetUserAgent());
 
-  if (!feedback_data_->attached_file_uuid().empty()) {
+  if (!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()));
+    BlobReader* attached_file_reader =
+        new BlobReader(profile, feedback_data->attached_file_uuid(),
+                       base::Bind(&FeedbackService::AttachedFileCallback,
+                                  AsWeakPtr(), feedback_data, callback));
     attached_file_reader->Start();
   }
 
-  if (!feedback_data_->screenshot_uuid().empty()) {
+  if (!feedback_data->screenshot_uuid().empty()) {
     // Self-deleting object.
-    BlobReader* screenshot_reader = new BlobReader(
-        profile, feedback_data_->screenshot_uuid(),
-        base::Bind(&FeedbackService::ScreenshotCallback, AsWeakPtr()));
+    BlobReader* screenshot_reader =
+        new BlobReader(profile, feedback_data->screenshot_uuid(),
+                       base::Bind(&FeedbackService::ScreenshotCallback,
+                                  AsWeakPtr(), feedback_data, callback));
     screenshot_reader->Start();
   }
 
-  CompleteSendFeedback();
+  CompleteSendFeedback(feedback_data, callback);
 }
 
-void FeedbackService::AttachedFileCallback(scoped_ptr<std::string> data,
-                                           int64_t /* total_blob_length */) {
-  feedback_data_->set_attached_file_uuid(std::string());
-  if (data)
-    feedback_data_->AttachAndCompressFileData(std::move(data));
-
-  CompleteSendFeedback();
+void FeedbackService::GetSystemInformation(
+    const GetSystemInformationCallback& callback) {
+  system_logs::ScrubbedSystemLogsFetcher* fetcher =
+      new system_logs::ScrubbedSystemLogsFetcher();
+  fetcher->Fetch(base::Bind(&FeedbackService::OnSystemLogsFetchComplete,
+                            AsWeakPtr(), callback));
 }
 
-void FeedbackService::ScreenshotCallback(scoped_ptr<std::string> data,
-                                         int64_t /* total_blob_length */) {
-  feedback_data_->set_screenshot_uuid(std::string());
+void FeedbackService::AttachedFileCallback(
+    scoped_refptr<feedback::FeedbackData> feedback_data,
+    const SendFeedbackCallback& callback,
+    scoped_ptr<std::string> data,
+    int64_t /* total_blob_length */) {
+  feedback_data->set_attached_file_uuid(std::string());
   if (data)
-    feedback_data_->set_image(std::move(data));
+    feedback_data->AttachAndCompressFileData(std::move(data));
 
-  CompleteSendFeedback();
+  CompleteSendFeedback(feedback_data, callback);
 }
 
-void FeedbackService::GetSystemInformation(
-    const GetSystemInformationCallback& callback) {
-  system_information_callback_ = callback;
+void FeedbackService::ScreenshotCallback(
+    scoped_refptr<feedback::FeedbackData> feedback_data,
+    const SendFeedbackCallback& callback,
+    scoped_ptr<std::string> data,
+    int64_t /* total_blob_length */) {
+  feedback_data->set_screenshot_uuid(std::string());
+  if (data)
+    feedback_data->set_image(std::move(data));
 
-  system_logs::ScrubbedSystemLogsFetcher* fetcher =
-      new system_logs::ScrubbedSystemLogsFetcher();
-  fetcher->Fetch(
-      base::Bind(&FeedbackService::OnSystemLogsFetchComplete, AsWeakPtr()));
+  CompleteSendFeedback(feedback_data, callback);
 }
 
-
 void FeedbackService::OnSystemLogsFetchComplete(
+    const GetSystemInformationCallback& callback,
     scoped_ptr<system_logs::SystemLogsResponse> sys_info_map) {
   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);
-
-  system_information_callback_.Run(sys_info_list);
+  callback.Run(sys_info_list);
 }
 
-void FeedbackService::CompleteSendFeedback() {
+void FeedbackService::CompleteSendFeedback(
+    scoped_refptr<feedback::FeedbackData> feedback_data,
+    const SendFeedbackCallback& callback) {
   // 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 +124,18 @@ 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();
+  const bool attached_file_completed =
+      feedback_data->attached_file_uuid().empty();
+  const bool screenshot_completed = 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();
+    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);
+    callback.Run(true);
   }
 }
 
diff --git a/chrome/browser/extensions/api/feedback_private/feedback_service.h b/chrome/browser/extensions/api/feedback_private/feedback_service.h
index 9588c53..d806c77 100644
--- a/chrome/browser/extensions/api/feedback_private/feedback_service.h
+++ b/chrome/browser/extensions/api/feedback_private/feedback_service.h
@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include <vector>
+
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/linked_ptr.h"
@@ -23,16 +24,16 @@ using extensions::api::feedback_private::SystemInformation;
 
 namespace extensions {
 
-typedef std::vector<linked_ptr<SystemInformation> > SystemInformationList;
+using SystemInformationList = std::vector<linked_ptr<SystemInformation>>;
 
 // The feedback service provides the ability to gather the various pieces of
 // data needed to send a feedback report and then send the report once all
 // the pieces are available.
 class FeedbackService : public base::SupportsWeakPtr<FeedbackService> {
  public:
-  typedef base::Callback<void(bool)> SendFeedbackCallback;
-  typedef base::Callback<void(const SystemInformationList& sys_info)>
-      GetSystemInformationCallback;
+  using SendFeedbackCallback = base::Callback<void(bool)>;
+  using GetSystemInformationCallback =
+      base::Callback<void(const SystemInformationList&)>;
 
   FeedbackService();
   virtual ~FeedbackService();
@@ -48,22 +49,23 @@ class FeedbackService : public base::SupportsWeakPtr<FeedbackService> {
 
  private:
   // Callbacks to receive blob data.
-  void AttachedFileCallback(scoped_ptr<std::string> data,
+  void AttachedFileCallback(scoped_refptr<feedback::FeedbackData> feedback_data,
+                            const SendFeedbackCallback& callback,
+                            scoped_ptr<std::string> data,
                             int64_t total_blob_length);
-  void ScreenshotCallback(scoped_ptr<std::string> data,
+  void ScreenshotCallback(scoped_refptr<feedback::FeedbackData> feedback_data,
+                          const SendFeedbackCallback& callback,
+                          scoped_ptr<std::string> data,
                           int64_t total_blob_length);
 
-  // Checks if we have read all the blobs we need to; signals the feedback
-  // data object once all the requisite data has been populated.
-  void CompleteSendFeedback();
-
   void OnSystemLogsFetchComplete(
+      const GetSystemInformationCallback& callback,
       scoped_ptr<system_logs::SystemLogsResponse> sys_info);
 
-  GetSystemInformationCallback system_information_callback_;
-  SendFeedbackCallback send_feedback_callback_;
-
-  scoped_refptr<feedback::FeedbackData> feedback_data_;
+  // Checks if we have read all the blobs we need to; signals the feedback
+  // data object once all the requisite data has been populated.
+  void CompleteSendFeedback(scoped_refptr<feedback::FeedbackData> feedback_data,
+                            const SendFeedbackCallback& callback);
 
   DISALLOW_COPY_AND_ASSIGN(FeedbackService);
 };
diff --git a/chrome/browser/resources/feedback/js/event_handler.js b/chrome/browser/resources/feedback/js/event_handler.js
index 4ca6d12..3776ecd 100644
--- a/chrome/browser/resources/feedback/js/event_handler.js
+++ b/chrome/browser/resources/feedback/js/event_handler.js
@@ -19,38 +19,6 @@ var FEEDBACK_HEIGHT = 585;
  */
 var FEEDBACK_DEFAULT_WINDOW_ID = 'default_window';
 
-/**
- * The feedback info received initially when the feedback UI was first requested
- * to start.
- * @type {Object}
- */
-var initialFeedbackInfo = null;
-
-/**
- * The feedbak info that is ready to be sent later when the system information
- * becomes available.
- * @type {Object}
- */
-var finalFeedbackInfo = null;
-
-/**
- * The system information received from the C++ side.
- * @type {Object}
- */
-var systemInfo = null;
-
-/**
- * True if the system information has been received, false otherwise.
- * @type {boolean}
- */
-var isSystemInfoReady = false;
-
-/**
- * A callback to be invoked when the system information is ready.
- * @type {function(sysInfo)}
- */
-var onSystemInfoReadyCallback = null;
-
 // To generate a hashed extension ID, use a sha-256 hash, all in lower case.
 // Example:
 //   echo -n 'abcdefghijklmnopqrstuvwxyzabcdef' | sha1sum | \
@@ -111,6 +79,117 @@ var whitelistedExtensionIds = [
   '3BC3740BFC58F06088B300274B4CFBEA20136342', // http://crbug.com/541769
 ];
 
+/**
+ * Used to generate unique IDs for FeedbackRequest objects.
+ * @type {number}
+ */
+var lastUsedId = 0;
+
+/**
+ * A FeedbackRequest object represents a unique feedback report, requested by an
+ * instance of the feedback window. It contains the system information specific
+ * to this report, the full feedbackInfo, and callbacks to send the report upon
+ * request.
+ */
+class FeedbackRequest {
+  constructor(feedbackInfo) {
+    this.id_ = ++lastUsedId;
+    this.feedbackInfo_ = feedbackInfo;
+    this.onSystemInfoReadyCallback_ = null;
+    this.isSystemInfoReady_ = false;
+    this.reportIsBeingSent_ = false;
+    this.isRequestCanceled_ = false;
+    this.useSystemInfo_ = false;
+  }
+
+  /**
+   * Called when the system information is sent from the C++ side.
+   * @param {Object} sysInfo The received system information.
+   */
+  getSystemInformationCallback(sysInfo) {
+    if (this.isRequestCanceled_) {
+      // If the window had been closed before the system information was
+      // received, we skip the rest of the operations and return immediately.
+      return;
+    }
+
+    this.isSystemInfoReady_ = true;
+
+    // Combine the newly received system information with whatever system
+    // information we have in the feedback info (if any).
+    if (this.feedbackInfo_.systemInformation) {
+      this.feedbackInfo_.systemInformation =
+          this.feedbackInfo_.systemInformation.concat(sysInfo);
+    } else {
+      this.feedbackInfo_.systemInformation = sysInfo;
+    }
+
+    if (this.onSystemInfoReadyCallback_ != null) {
+      this.onSystemInfoReadyCallback_();
+      this.onSystemInfoReadyCallback_ = null;
+    }
+  }
+
+  /**
+   * Retrieves the system information for this request object.
+   * @param {function()} callback Invoked to notify the listener that the system
+   * information has been received.
+   */
+  getSystemInformation(callback) {
+    if (this.isSystemInfoReady_) {
+      callback();
+      return;
+    }
+
+    this.onSystemInfoReadyCallback_ = callback;
+    // The C++ side must reply to the callback specific to this object.
+    var boundCallback = this.getSystemInformationCallback.bind(this);
+    chrome.feedbackPrivate.getSystemInformation(boundCallback);
+  }
+
+  /**
+   * Sends the feedback report represented by the object, either now if system
+   * information is ready, or later once it is.
+   * @param {boolean} useSystemInfo True if the user would like the system
+   * information to be sent with the report.
+   */
+  sendReport(useSystemInfo) {
+    this.reportIsBeingSent_ = true;
+    this.useSystemInfo_ = useSystemInfo;
+    if (useSystemInfo && !this.isSystemInfoReady_) {
+      this.onSystemInfoReadyCallback_ = this.sendReportNow;
+      return;
+    }
+
+    this.sendReportNow();
+  }
+
+  /**
+   * Sends the report immediately and removes this object once the report is
+   * sent.
+   */
+  sendReportNow() {
+    if (!this.useSystemInfo_) {
+      // Clear the system information if the user doesn't want it to be sent.
+      this.feedbackInfo_.systemInformation = null;
+    }
+
+    /** @const */ var ID = this.id_;
+    chrome.feedbackPrivate.sendFeedback(this.feedbackInfo_,
+        function(result) {
+          console.log('Feedback: Report sent for request with ID ' + ID);
+        });
+  }
+
+  /**
+   * Handles the event when the feedback UI window corresponding to this
+   * FeedbackRequest instance is closed.
+   */
+  onWindowClosed() {
+    if (!this.reportIsBeingSent_)
+      this.isRequestCanceled_ = true;
+  }
+};
 
 /**
  * Function to determine whether or not a given extension id is whitelisted to
@@ -145,13 +224,10 @@ function senderWhitelisted(id, startFeedbackCallback, feedbackInfo) {
  * @param {function(Object)} sendResponse Callback for sending a response.
  */
 function feedbackReadyHandler(request, sender, sendResponse) {
-  if (request.ready) {
-    chrome.runtime.sendMessage(
-        {sentFromEventPage: true, data: initialFeedbackInfo});
-  }
+  if (request.ready)
+    chrome.runtime.sendMessage({sentFromEventPage: true});
 }
 
-
 /**
  * Callback which gets notified if another extension is requesting feedback.
  * @param {Object} request The message request object.
@@ -164,38 +240,6 @@ function requestFeedbackHandler(request, sender, sendResponse) {
 }
 
 /**
- * Called when the system information is sent from the C++ side.
- * @param {Object} sysInfo The received system information.
- */
-
-function getSystemInformationCallback(sysInfo) {
-  systemInfo = sysInfo;
-  isSystemInfoReady = true;
-  if (onSystemInfoReadyCallback != null)
-    onSystemInfoReadyCallback(sysInfo);
-}
-
-/**
- * If the user requested to send the report before the system information was
- * received, this callback will be invoked once the system information is ready
- * to send the report then.
- * @param {Object} sysInfo The received system information.
- */
-
-function onSysInfoReadyForSend(sysInfo) {
-  // Combine the newly received system information with whatever system
-  // information we have in the final feedback info (if any).
-  if (finalFeedbackInfo.systemInformation) {
-    finalFeedbackInfo.systemInformation =
-        finalFeedbackInfo.systemInformation.concat(sysInfo);
-  } else {
-    finalFeedbackInfo.systemInformation = sysInfo;
-  }
-
-  chrome.feedbackPrivate.sendFeedback(finalFeedbackInfo, function(result) {});
-}
-
-/**
  * Callback which starts up the feedback UI.
  * @param {Object} feedbackInfo Object containing any initial feedback info.
  */
@@ -213,46 +257,30 @@ function startFeedbackUI(feedbackInfo) {
       hidden: true,
       resizable: false },
       function(appWindow) {
-        // Initialize the state of the app only once upon the creation of the
-        // feedback UI window.
-        initialFeedbackInfo = feedbackInfo;
-        finalFeedbackInfo = null;
-        systemInfo = null;
-        isSystemInfoReady = false;
-        onSystemInfoReadyCallback = null;
+        var request = new FeedbackRequest(feedbackInfo);
+
+        // The feedbackInfo member of the new window should refer to the one in
+        // its corresponding FeedbackRequest object to avoid copying and
+        // duplicatations.
+        appWindow.contentWindow.feedbackInfo = request.feedbackInfo_;
 
         // Define some functions for the new window so that it can call back
         // into here.
 
         // Define a function for the new window to get the system information.
         appWindow.contentWindow.getSystemInformation = function(callback) {
-          if (!isSystemInfoReady) {
-            onSystemInfoReadyCallback = callback;
-            chrome.feedbackPrivate.getSystemInformation(
-                getSystemInformationCallback);
-            return;
-          }
-
-          callback(systemInfo);
+          request.getSystemInformation(callback);
         };
 
-        // Define a function to be called by the new window when the report is
-        // not ready yet, and has to be sent later when the system information
-        // is received.
-        appWindow.contentWindow.sendReportLater = function(feedbackInfo) {
-          finalFeedbackInfo = feedbackInfo;
-          if (!isSystemInfoReady) {
-            onSystemInfoReadyCallback = onSysInfoReadyForSend;
-            return;
-          }
-
-          onSysInfoReadyForSend(systemInfo);
+        // Define a function to request sending the feedback report.
+        appWindow.contentWindow.sendFeedbackReport = function(useSystemInfo) {
+          request.sendReport(useSystemInfo);
         };
 
-        // Returns whether the system information has been received or not.
-        appWindow.contentWindow.isSystemInfoReady = function() {
-          return isSystemInfoReady;
-        };
+        // Observe when the window is closed.
+        appWindow.onClosed.addListener(function() {
+          request.onWindowClosed();
+        });
       });
 }
 
diff --git a/chrome/browser/resources/feedback/js/feedback.js b/chrome/browser/resources/feedback/js/feedback.js
index 6cd19b6..065081e 100644
--- a/chrome/browser/resources/feedback/js/feedback.js
+++ b/chrome/browser/resources/feedback/js/feedback.js
@@ -63,7 +63,12 @@ var FeedbackFlow = {
 var attachedFileBlob = null;
 var lastReader = null;
 
-var feedbackInfo = null;
+/**
+ * Determines whether the system information associated with this instance of
+ * the feedback window has been received.
+ * @type {boolean}
+ */
+var isSystemInfoReady = false;
 
 /**
  * The callback used by the sys_info_page to receive the event that the system
@@ -176,29 +181,13 @@ function sendReport() {
   if (!$('screenshot-checkbox').checked)
     feedbackInfo.screenshot = null;
 
-  if (useSystemInfo) {
-    // The user chose to include the system information as part of the report.
-    // If they are not ready yet, we have to send the report later.
-    if (!isSystemInfoReady()) {
-      // The report will be sent later by the feedback extension's background
-      // page (see event_handler.js).
-      sendReportLater(feedbackInfo);
-
-      // No landing page for login screen feedback.
-      if (feedbackInfo.flow != FeedbackFlow.LOGIN)
-        window.open(FEEDBACK_LANDING_PAGE, '_blank');
-      window.close();
-      return true;
-    }
-  }
-
-  chrome.feedbackPrivate.sendFeedback(feedbackInfo, function(result) {
-    // No landing page for login screen feedback.
-    if (feedbackInfo.flow != FeedbackFlow.LOGIN)
-      window.open(FEEDBACK_LANDING_PAGE, '_blank');
-    window.close();
-  });
-
+  // Request sending the report, show the landing page (if allowed), and close
+  // this window right away. The FeedbackRequest object that represents this
+  // report will take care of sending the report in the background.
+  sendFeedbackReport(useSystemInfo);
+  if (feedbackInfo.flow != FeedbackFlow.LOGIN)
+    window.open(FEEDBACK_LANDING_PAGE, '_blank');
+  window.close();
   return true;
 }
 
@@ -267,18 +256,9 @@ function resizeAppWindow() {
 /**
  * A callback to be invoked when the background page of this extension receives
  * the system information.
- * @param {Object} sysInfo The received system information.
  */
-function onSystemInformation(sysInfo) {
-  // Concatenate the feedbackInfo's systemInformation with the newly received
-  // sysInfo once (if any).
-  if (feedbackInfo.systemInformation) {
-    feedbackInfo.systemInformation =
-      feedbackInfo.systemInformation.concat(sysInfo);
-  } else {
-    feedbackInfo.systemInformation = sysInfo;
-  }
-
+function onSystemInformation() {
+  isSystemInfoReady = true;
   // In case the sys_info_page needs to be notified by this event, do so.
   if (sysInfoPageOnSysInfoReadyCallback != null) {
     sysInfoPageOnSysInfoReadyCallback(feedbackInfo.systemInformation);
@@ -301,8 +281,6 @@ function initialize() {
   // Add listener to receive the feedback info object.
   chrome.runtime.onMessage.addListener(function(request, sender, sendResponse) {
     if (request.sentFromEventPage) {
-      feedbackInfo = request.data;
-
       if (!feedbackInfo.flow)
         feedbackInfo.flow = FeedbackFlow.REGULAR;
 
@@ -329,8 +307,8 @@ function initialize() {
         $('user-email-text').value = email;
       });
 
-      // Initiate getting the system info and use it to prepare the full
-      // feedback info.
+      // Initiate getting the system info.
+      isSystemInfoReady = false;
       getSystemInformation(onSystemInformation);
 
       // An extension called us with an attached file.
@@ -385,7 +363,7 @@ function initialize() {
                 // Gets the full system information for the new window.
                 appWindow.contentWindow.getFullSystemInfo =
                     function(callback) {
-                      if (isSystemInfoReady()) {
+                      if (isSystemInfoReady) {
                         callback(feedbackInfo.systemInformation);
                         return;
                       }