Separate validation failures from other failures in ExecuteCodeFunction.

chrome/browser/extensions/api/tabs/tabs_api.cc

Index: chrome/browser/extensions/api/tabs/tabs_api.cc
diff --git a/chrome/browser/extensions/api/tabs/tabs_api.cc b/chrome/browser/extensions/api/tabs/tabs_api.cc
index 7daaef12d8527b92feb6535113f5abdc0af7dd6b..72491a6dc532220852fee02cd7fbb0271ebcb212 100644
--- a/chrome/browser/extensions/api/tabs/tabs_api.cc
+++ b/chrome/browser/extensions/api/tabs/tabs_api.cc
@@ -1795,7 +1795,7 @@ ExecuteCodeInTabFunction::ExecuteCodeInTabFunction()
 ExecuteCodeInTabFunction::~ExecuteCodeInTabFunction() {}
 
 bool ExecuteCodeInTabFunction::HasPermission() {
-  if (Init() &&
+  if (Init() == SUCCESS &&

[Devlin, 2017/01/17 18:52:08]

Per our offline discussion, I'm not sure this does anything at all.  Can we add
a TODO(devlin/lazyboy) to remove it?  (Or tell me where my logic went wrong -
which is also totally possible :))

[lazyboy, 2017/01/18 02:29:10]

I've checked an extension with only "activeTab" permission, and what I've found
is if I execute script on browser action click, both line 1799 below returns
true and if I comment out that line, line 1803 also returns true.


So it seems like line 1799 isn't wrong because
ExtensionActionViewController::ExecuteAction() ->
extensions::ExtensionActionRunner::RunAction() ->
extensions::ActiveTabPermissionGranter::GrantIfRequested() would put
execute_tab_id_ in PermissionsData::tab_specific_permissions_.

I might be missing some other use case than browser action click?

[Devlin, 2017/01/24 00:11:43]

The ActiveTabPermissionGranter grants both kTab and kActiveTab when the
extension is invoked (which is frequently clicking the browser action, but not
always).  So yes, the branch here (and specifically the HasAPIPermissionForTab
check) will return true.  However, my point was that this permission check
doesn't really *do* anything.  The check on line 1803 will always return true,
because the tabs feature (including tabs.executeScript) has no dependencies.

In other words, as far as I can tell, this is essentially:

bool conditionally_true = GetConditionallyTrue();  // Maybe true, maybe false
if (conditionally_true)
  return true;
return true;

|conditionally_true| may not always be true - but it never changes the outcome
of this function.

[lazyboy, 2017/01/24 01:52:13]

Ah ok. I've added TODO to remove this.

       extension_->permissions_data()->HasAPIPermissionForTab(
           execute_tab_id_, APIPermission::kTab)) {
     return true;
@@ -1803,38 +1803,40 @@ bool ExecuteCodeInTabFunction::HasPermission() {
   return ExtensionFunction::HasPermission();
 }
 
-bool ExecuteCodeInTabFunction::Init() {
-  if (details_.get())
-    return true;
+ExecuteCodeFunction::InitResult ExecuteCodeInTabFunction::Init() {
+  if (init_result_)
+    return init_result_.value();
 
   // |tab_id| is optional so it's ok if it's not there.
   int tab_id = -1;
-  if (args_->GetInteger(0, &tab_id))
-    EXTENSION_FUNCTION_VALIDATE(tab_id >= 0);
+  if (args_->GetInteger(0, &tab_id) && tab_id < 0)
+    return set_init_result(VALIDATION_FAILURE);
 
   // |details| are not optional.
   base::DictionaryValue* details_value = NULL;
   if (!args_->GetDictionary(1, &details_value))
-    return false;
+    return set_init_result(VALIDATION_FAILURE);
   std::unique_ptr<InjectDetails> details(new InjectDetails());
   if (!InjectDetails::Populate(*details_value, details.get()))
-    return false;
+    return set_init_result(VALIDATION_FAILURE);
 
   // If the tab ID wasn't given then it needs to be converted to the
   // currently active tab's ID.
   if (tab_id == -1) {
     Browser* browser = chrome_details_.GetCurrentBrowser();
+    // Can happen during shutdown.
     if (!browser)
-      return false;
+      return set_init_result(FAILURE, keys::kNoCurrentWindowError);
     content::WebContents* web_contents = NULL;
+    // Can happen during shutdown.
     if (!ExtensionTabUtil::GetDefaultTab(browser, &web_contents, &tab_id))
-      return false;
+      return set_init_result(FAILURE, keys::kNoTabInBrowserWindowError);
   }
 
   execute_tab_id_ = tab_id;
   details_ = std::move(details);
   set_host_id(HostID(HostID::EXTENSIONS, extension()->id()));
-  return true;
+  return set_init_result(SUCCESS);
 }
 
 bool ExecuteCodeInTabFunction::CanExecuteScriptOnPage() {