Remove some usages of AsyncExtensionFunction::results_.

extensions/browser/api/management/management_api.cc

Index: extensions/browser/api/management/management_api.cc
diff --git a/extensions/browser/api/management/management_api.cc b/extensions/browser/api/management/management_api.cc
index 18f94189620adb1a0bb58b74cdd42ef885bf74a8..1a796dee2ba7a965ce527fef30eeaf903e840863 100644
--- a/extensions/browser/api/management/management_api.cc
+++ b/extensions/browser/api/management/management_api.cc
@@ -318,7 +318,8 @@ ManagementGetPermissionWarningsByIdFunction::Run() {
       management::GetPermissionWarningsById::Results::Create(warnings)));
 }
 
-bool ManagementGetPermissionWarningsByManifestFunction::RunAsync() {
+ExtensionFunction::ResponseAction
+ManagementGetPermissionWarningsByManifestFunction::Run() {
   std::unique_ptr<management::GetPermissionWarningsByManifest::Params> params(
       management::GetPermissionWarningsByManifest::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
@@ -335,11 +336,10 @@ bool ManagementGetPermissionWarningsByManifestFunction::RunAsync() {
     AddRef();
 
     // Response is sent async in OnParseSuccess/Failure().
-    return true;
+    return RespondLater();
   } else {
     // TODO(lfg) add error string
-    OnParseFailure("");
-    return false;
+    return RespondNow(Error(""));

[lazyboy, 2017/01/06 03:40:30]

There was a Release(Ref) call in OnParseFailure(), which seems wrong. Fixed now.

[Devlin, 2017/01/06 15:51:06]

let's make this kUnknownErrorDoNotUse.

[lazyboy, 2017/01/19 03:47:59]

Done.

   }
 }
 
@@ -352,29 +352,29 @@ void ManagementGetPermissionWarningsByManifestFunction::OnParseSuccess(
   const base::DictionaryValue* parsed_manifest =
       static_cast<const base::DictionaryValue*>(value.get());
 
+  std::string error;
   scoped_refptr<Extension> extension =
       Extension::Create(base::FilePath(), Manifest::INVALID_LOCATION,
-                        *parsed_manifest, Extension::NO_FLAGS, &error_);
+                        *parsed_manifest, Extension::NO_FLAGS, &error);
+  // TODO(lazyboy): Do we need to use |error|?
   if (!extension) {
     OnParseFailure(keys::kExtensionCreateError);
     return;
   }
 
   std::vector<std::string> warnings = CreateWarningsList(extension.get());
-  results_ =
-      management::GetPermissionWarningsByManifest::Results::Create(warnings);
-  SendResponse(true);
+  Respond(ArgumentList(
+      management::GetPermissionWarningsByManifest::Results::Create(warnings)));
 
-  // Matched with AddRef() in RunAsync().
+  // Matched with AddRef() in Run().
   Release();
 }
 
 void ManagementGetPermissionWarningsByManifestFunction::OnParseFailure(
     const std::string& error) {
-  error_ = error;
-  SendResponse(false);
+  Respond(Error(error));
 
-  // Matched with AddRef() in RunAsync().
+  // Matched with AddRef() in Run().
   Release();
 }
 
@@ -632,17 +632,13 @@ void ManagementCreateAppShortcutFunction::SetAutoConfirmForTest(
 }
 
 void ManagementCreateAppShortcutFunction::OnCloseShortcutPrompt(bool created) {
-  if (!created)
-    error_ = keys::kCreateShortcutCanceledError;
-  SendResponse(created);
+  Respond(created ? NoArguments() : Error(keys::kCreateShortcutCanceledError));
   Release();
 }
 
-bool ManagementCreateAppShortcutFunction::RunAsync() {
-  if (!user_gesture()) {
-    error_ = keys::kGestureNeededForCreateAppShortcutError;
-    return false;
-  }
+ExtensionFunction::ResponseAction ManagementCreateAppShortcutFunction::Run() {
+  if (!user_gesture())
+    return RespondNow(Error(keys::kGestureNeededForCreateAppShortcutError));
 
   std::unique_ptr<management::CreateAppShortcut::Params> params(
       management::CreateAppShortcut::Params::Create(*args_));
@@ -651,21 +647,18 @@ bool ManagementCreateAppShortcutFunction::RunAsync() {
       ExtensionRegistry::Get(browser_context())
           ->GetExtensionById(params->id, ExtensionRegistry::EVERYTHING);
   if (!extension) {
-    error_ =
-        ErrorUtils::FormatErrorMessage(keys::kNoExtensionError, params->id);
-    return false;
+    return RespondNow(Error(
+        ErrorUtils::FormatErrorMessage(keys::kNoExtensionError, params->id)));
   }
 
   if (!extension->is_app()) {
-    error_ = ErrorUtils::FormatErrorMessage(keys::kNotAnAppError, params->id);
-    return false;
+    return RespondNow(Error(
+        ErrorUtils::FormatErrorMessage(keys::kNotAnAppError, params->id)));
   }
 
 #if defined(OS_MACOSX)
-  if (!extension->is_platform_app()) {
-    error_ = keys::kCreateOnlyPackagedAppShortcutMac;
-    return false;
-  }
+  if (!extension->is_platform_app())
+    return RespondNow(Error(keys::kCreateOnlyPackagedAppShortcutMac));
 #endif
 
   if (auto_confirm_for_test != DO_NOT_SKIP) {
@@ -674,7 +667,7 @@ bool ManagementCreateAppShortcutFunction::RunAsync() {
 
     OnCloseShortcutPrompt(auto_confirm_for_test == PROCEED);
 
-    return true;
+    return RespondLater();

[Devlin, 2017/01/06 15:51:06]

This is unfortunately wrong, because OnCloseShortcutPrompt() Respond()s
synchronously.  This is a limitation of the response system.  One common work
around is to post a task to OnCloseShortcutPrompt (thereby making the
RespondLater here correct).  I feel like there should be a better, more general
fix by somehow allowing this, but I haven't thought of a way that would still
let us have the determinism we want.

[lazyboy, 2017/01/07 02:55:18]

To understand the problem: with callbacks, this already happens, right?:
RunAsync() {
  some_api->Call(base::Bind(::Callback,...))
  return true;
}
Callback() {
  SetResult(..)
  SendResponse(true);
}
^^ SendResponse() can be called before RunAsync returns...

"Always PostTask when there's callback" approach seems a bit more unnecessary
code execution. On top of that, this wouldn't guarantee the determinism either
:( because you cannot stop implementer from calling Respond synchronously
anyway.

Callback seems tough for these: you can call them synchronously, you can call
them more than once.

Would there be broken-ness if someone Respond()ed before returning
RespondLater()? We can relax the contract of RespondLater by saying you're on
your own to Respond(), within Run() or later point in time... Not sure if that
goes against all the efforts to achieve better response system though.

[Devlin, 2017/01/09 22:05:26]

Agreed that this isn't great, and I'd prefer something else (which is why I
haven't made these changes more widely) - but it *should* be guaranteed to be
correct.  If you post a task, it's guaranteed to execute after what you're
currently doing (% run loops, etc), so something like:
base::PostTask(OnCloseShortcutPrompt());
return RespondLater();

should always be safe and correct.

All that said, I don't want that to be the norm.
Yeah, that'd definitely makes me sad.  One of the biggest things here is that we
want to ensure that functions know what they're doing properly (which was
all-but-impossible in the old system).  If a function says RespondLater(), I'd
like it to be guaranteed that it has not yet responded.

The best solution I've come up with so far (which I'm not at all in love with)
would be to have an AlreadyResponded() ResponseAction.  At the very least, this
would require function implementations to be clear when they expect this to be
the case, and allow us to assert, etc.

   }
 
   std::string error;
@@ -684,12 +677,11 @@ bool ManagementCreateAppShortcutFunction::RunAsync() {
           ->CreateAppShortcutFunctionDelegate(this, extension, &error)) {
     // Matched with a Release() in OnCloseShortcutPrompt().
     AddRef();
+    // Response is sent async in OnCloseShortcutPrompt().
+    return RespondLater();
   } else {
-    SetError(error);
+    return RespondNow(Error(error));

[lazyboy, 2017/01/06 03:40:30]

This another bug fix where this branch's extension API call would never return
because this branch wouldn't result in OnCloseShortcutPrompt.

   }
-
-  // Response is sent async in OnCloseShortcutPrompt().
-  return true;
 }
 
 ExtensionFunction::ResponseAction ManagementSetLaunchTypeFunction::Run() {
@@ -753,23 +745,20 @@ void ManagementGenerateAppForLinkFunction::FinishCreateBookmarkApp(
     const Extension* extension,
     const WebApplicationInfo& web_app_info) {
   if (extension) {
-    results_ = management::GenerateAppForLink::Results::Create(
-        CreateExtensionInfo(*extension, browser_context()));
+    Respond(ArgumentList(management::GenerateAppForLink::Results::Create(

[Devlin, 2017/01/06 15:51:06]

nit: maybe cleaner as something like:
ResponseAction action = extension ?
    ArgumentList(management::GenerateAppForLink::Results::Create(...)) :
    Error(keys::...);
Respond(std::move(action));
Release();  // Balanced in...

[lazyboy, 2017/01/19 03:47:59]

Done.

+        CreateExtensionInfo(*extension, browser_context()))));
 
-    SendResponse(true);
     Release();
   } else {
-    error_ = keys::kGenerateAppForLinkInstallError;
-    SendResponse(false);
+    Respond(Error(keys::kGenerateAppForLinkInstallError));
+
     Release();
   }
 }
 
-bool ManagementGenerateAppForLinkFunction::RunAsync() {
-  if (!user_gesture()) {
-    error_ = keys::kGestureNeededForGenerateAppForLinkError;
-    return false;
-  }
+ExtensionFunction::ResponseAction ManagementGenerateAppForLinkFunction::Run() {
+  if (!user_gesture())
+    return RespondNow(Error(keys::kGestureNeededForGenerateAppForLinkError));
 
   std::unique_ptr<management::GenerateAppForLink::Params> params(
       management::GenerateAppForLink::Params::Create(*args_));
@@ -777,15 +766,12 @@ bool ManagementGenerateAppForLinkFunction::RunAsync() {
 
   GURL launch_url(params->url);
   if (!launch_url.is_valid() || !launch_url.SchemeIsHTTPOrHTTPS()) {
-    error_ =
-        ErrorUtils::FormatErrorMessage(keys::kInvalidURLError, params->url);
-    return false;
+    return RespondNow(Error(
+        ErrorUtils::FormatErrorMessage(keys::kInvalidURLError, params->url)));
   }
 
-  if (params->title.empty()) {
-    error_ = keys::kEmptyTitleError;
-    return false;
-  }
+  if (params->title.empty())
+    return RespondNow(Error(keys::kEmptyTitleError));
 
   app_for_link_delegate_ =
       ManagementAPI::GetFactoryInstance()
@@ -798,7 +784,7 @@ bool ManagementGenerateAppForLinkFunction::RunAsync() {
   AddRef();
 
   // Response is sent async in FinishCreateBookmarkApp().
-  return true;
+  return RespondLater();
 }
 
 ManagementEventRouter::ManagementEventRouter(content::BrowserContext* context)