[Extensions] Isolate ExtensionFunction results_ and error_

extensions/browser/extension_function.cc

 // Copyright 2013 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 "extensions/browser/extension_function.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/macros.h"
@@ skipping 61 matching lines @@
                                   histogram_value);
     }
     UMA_HISTOGRAM_TIMES("Extensions.Functions.FailedTotalExecutionTime",
                         elapsed_time);
   }
 }
 
 class ArgumentListResponseValue
     : public ExtensionFunction::ResponseValueObject {
  public:
-  ArgumentListResponseValue(const std::string& function_name,
-                            const char* title,
-                            ExtensionFunction* function,
-                            std::unique_ptr<base::ListValue> result)
-      : function_name_(function_name), title_(title) {
-    if (function->GetResultList()) {
-      DCHECK_EQ(function->GetResultList(), result.get())
-          << "The result set on this function (" << function_name_ << ") "
-          << "either by calling SetResult() or directly modifying |result_| is "
-          << "different to the one passed to " << title_ << "(). "
-          << "The best way to fix this problem is to exclusively use " << title_
-          << "(). SetResult() and |result_| are deprecated.";
-    } else {
-      function->SetResultList(std::move(result));
-    }
+  ArgumentListResponseValue(ExtensionFunction* function,
+                            std::unique_ptr<base::ListValue> result) {
+    SetFunctionResults(function, std::move(result));
     // It would be nice to DCHECK(error.empty()) but some legacy extension
     // function implementations... I'm looking at chrome.input.ime... do this
     // for some reason.
   }
 
   ~ArgumentListResponseValue() override {}
 
   bool Apply() override { return true; }
-
- private:
-  std::string function_name_;
-  const char* title_;
 };
 
 class ErrorWithArgumentsResponseValue : public ArgumentListResponseValue {
  public:
-  ErrorWithArgumentsResponseValue(const std::string& function_name,
-                                  const char* title,
-                                  ExtensionFunction* function,
+  ErrorWithArgumentsResponseValue(ExtensionFunction* function,
                                   std::unique_ptr<base::ListValue> result,
                                   const std::string& error)
-      : ArgumentListResponseValue(function_name,
-                                  title,
-                                  function,
-                                  std::move(result)) {
-    function->SetError(error);
+      : ArgumentListResponseValue(function, std::move(result)) {
+    SetFunctionError(function, error);
   }
 
   ~ErrorWithArgumentsResponseValue() override {}
 
   bool Apply() override { return false; }
 };
 
 class ErrorResponseValue : public ExtensionFunction::ResponseValueObject {
  public:
   ErrorResponseValue(ExtensionFunction* function, const std::string& error) {
     // It would be nice to DCHECK(!error.empty()) but too many legacy extension
     // function implementations don't set error but signal failure.
-    function->SetError(error);
+    SetFunctionError(function, error);
   }
 
   ~ErrorResponseValue() override {}
 
   bool Apply() override { return false; }
 };
 
 class BadMessageResponseValue : public ExtensionFunction::ResponseValueObject {
  public:
   explicit BadMessageResponseValue(ExtensionFunction* function) {
@@ skipping 68 matching lines @@
 }
 
 void UserGestureForTests::DecrementCount() {
   base::AutoLock autolock(lock_);
   --count_;
 }
 
 
 }  // namespace
 
+void ExtensionFunction::ResponseValueObject::SetFunctionResults(
+    ExtensionFunction* function,
+    std::unique_ptr<base::ListValue> results) {
+  DCHECK(!function->results_) << "Function " << function->name_
+                              << "already has results set.";
+  function->results_ = std::move(results);
+}
+
+void ExtensionFunction::ResponseValueObject::SetFunctionError(
+    ExtensionFunction* function,
+    const std::string& error) {
+  DCHECK(function->error_.empty()) << "Function " << function->name_
+                                   << "already has an error.";
+  function->error_ = error;
+}
+
 // static
 bool ExtensionFunction::ignore_all_did_respond_for_testing_do_not_use = false;
 
 // static
 const char* ExtensionFunction::kUnknownErrorDoNotUse = "Unknown error.";
 
 // static
 void ExtensionFunctionDeleteTraits::Destruct(const ExtensionFunction* x) {
   x->Destruct();
 }
@@ skipping 68 matching lines @@
 void ExtensionFunction::OnQuotaExceeded(const std::string& violation_error) {
   error_ = violation_error;
   SendResponseImpl(false);
 }
 
 void ExtensionFunction::SetArgs(const base::ListValue* args) {
   DCHECK(!args_.get());  // Should only be called once.
   args_ = args->CreateDeepCopy();
 }
 
-void ExtensionFunction::SetResult(std::unique_ptr<base::Value> result) {
-  results_.reset(new base::ListValue());
-  results_->Append(std::move(result));
-}
-
-void ExtensionFunction::SetResultList(
-    std::unique_ptr<base::ListValue> results) {
-  results_ = std::move(results);
-}
-
 const base::ListValue* ExtensionFunction::GetResultList() const {
   return results_.get();
 }
 
-std::string ExtensionFunction::GetError() const {
+const std::string& ExtensionFunction::GetError() const {
   return error_;
 }
 
 void ExtensionFunction::SetError(const std::string& error) {
   error_ = error;
 }
 
 bool ExtensionFunction::user_gesture() const {
   return user_gesture_ || UserGestureForTests::GetInstance()->HaveGesture();
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::NoArguments() {
-  return ResponseValue(new ArgumentListResponseValue(
-      name(), "NoArguments", this, base::MakeUnique<base::ListValue>()));
+  return ResponseValue(
+      new ArgumentListResponseValue(this, base::MakeUnique<base::ListValue>()));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::OneArgument(
     std::unique_ptr<base::Value> arg) {
   std::unique_ptr<base::ListValue> args(new base::ListValue());
   args->Append(std::move(arg));
-  return ResponseValue(new ArgumentListResponseValue(name(), "OneArgument",
-                                                     this, std::move(args)));
+  return ResponseValue(new ArgumentListResponseValue(this, std::move(args)));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::TwoArguments(
     std::unique_ptr<base::Value> arg1,
     std::unique_ptr<base::Value> arg2) {
   std::unique_ptr<base::ListValue> args(new base::ListValue());
   args->Append(std::move(arg1));
   args->Append(std::move(arg2));
-  return ResponseValue(new ArgumentListResponseValue(name(), "TwoArguments",
-                                                     this, std::move(args)));
+  return ResponseValue(new ArgumentListResponseValue(this, std::move(args)));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::ArgumentList(
     std::unique_ptr<base::ListValue> args) {
-  return ResponseValue(new ArgumentListResponseValue(name(), "ArgumentList",
-                                                     this, std::move(args)));
+  return ResponseValue(new ArgumentListResponseValue(this, std::move(args)));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::Error(
     const std::string& error) {
   return ResponseValue(new ErrorResponseValue(this, error));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::Error(
     const std::string& format,
     const std::string& s1) {
@@ skipping 14 matching lines @@
     const std::string& s1,
     const std::string& s2,
     const std::string& s3) {
   return ResponseValue(new ErrorResponseValue(
       this, ErrorUtils::FormatErrorMessage(format, s1, s2, s3)));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::ErrorWithArguments(
     std::unique_ptr<base::ListValue> args,
     const std::string& error) {
-  return ResponseValue(new ErrorWithArgumentsResponseValue(
-      name(), "ErrorWithArguments", this, std::move(args), error));
+  return ResponseValue(
+      new ErrorWithArgumentsResponseValue(this, std::move(args), error));
 }
 
 ExtensionFunction::ResponseValue ExtensionFunction::BadMessage() {
   return ResponseValue(new BadMessageResponseValue(this));
 }
 
 ExtensionFunction::ResponseAction ExtensionFunction::RespondNow(
     ResponseValue result) {
   return ResponseAction(new RespondNowAction(
       std::move(result),
@@ skipping 66 matching lines @@
 UIThreadExtensionFunction::~UIThreadExtensionFunction() {
   if (dispatcher() && render_frame_host())
     dispatcher()->OnExtensionFunctionCompleted(extension());
   // The extension function should always respond to avoid leaks in the
   // renderer, dangling callbacks, etc. The exception is if the system is
   // shutting down.
   // TODO(devlin): Duplicate this check in IOThreadExtensionFunction. It's
   // tricky because checking IsShuttingDown has to be called from the UI thread.
   extensions::ExtensionsBrowserClient* browser_client =
       extensions::ExtensionsBrowserClient::Get();
-  DCHECK(!browser_client || browser_client->IsShuttingDown() || did_respond_ ||
+  DCHECK(!browser_client || browser_client->IsShuttingDown() || did_respond() ||
          ignore_all_did_respond_for_testing_do_not_use)
-      << name_;
+      << name();
 }
 
 UIThreadExtensionFunction*
 UIThreadExtensionFunction::AsUIThreadExtensionFunction() {
   return this;
 }
 
 bool UIThreadExtensionFunction::PreRunValidation(std::string* error) {
   if (!ExtensionFunction::PreRunValidation(error))
     return false;
@@ skipping 83 matching lines @@
   return this;
 }
 
 void IOThreadExtensionFunction::Destruct() const {
   BrowserThread::DeleteOnIOThread::Destruct(this);
 }
 
 AsyncExtensionFunction::AsyncExtensionFunction() {
 }
 
+void AsyncExtensionFunction::SetError(const std::string& error) {
+  error_ = error;
+}
+
+const std::string& AsyncExtensionFunction::GetError() const {
+  return error_.empty() ? UIThreadExtensionFunction::GetError() : error_;
+}
+
 AsyncExtensionFunction::~AsyncExtensionFunction() {
 }
 
+void AsyncExtensionFunction::SetResult(std::unique_ptr<base::Value> result) {
+  results_.reset(new base::ListValue());
+  results_->Append(std::move(result));
+}
+
+void AsyncExtensionFunction::SetResultList(
+    std::unique_ptr<base::ListValue> results) {
+  results_ = std::move(results);
+}
+
 ExtensionFunction::ScopedUserGestureForTests::ScopedUserGestureForTests() {
   UserGestureForTests::GetInstance()->IncrementCount();
 }
 
 ExtensionFunction::ScopedUserGestureForTests::~ScopedUserGestureForTests() {
   UserGestureForTests::GetInstance()->DecrementCount();
 }
 
 ExtensionFunction::ResponseAction AsyncExtensionFunction::Run() {
-  return RunAsync() ? RespondLater() : RespondNow(Error(error_));
+  if (RunAsync())
+    return RespondLater();
+  DCHECK(!results_);
+  return RespondNow(Error(error_));
 }
 
 // static
 bool AsyncExtensionFunction::ValidationFailure(
     AsyncExtensionFunction* function) {
   return false;
 }
 
 void AsyncExtensionFunction::SendResponse(bool success) {
-  Respond(success ? ArgumentList(std::move(results_)) : Error(error_));
+  ResponseValue response;
+  if (success) {
+    response = ArgumentList(std::move(results_));
+  } else {
+    response = results_ ? ErrorWithArguments(std::move(results_), error_)
+                        : Error(error_);
+  }
+  Respond(std::move(response));
 }