Separate validation failures from other failures in ExecuteCodeFunction.

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

 // Copyright (c) 2012 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 "chrome/browser/extensions/api/tabs/tabs_api.h"
 
 #include <stddef.h>
 #include <algorithm>
 #include <limits>
 #include <memory>
@@ skipping 1777 matching lines @@
   Release();  // Balanced in Run()
 }
 
 ExecuteCodeInTabFunction::ExecuteCodeInTabFunction()
     : chrome_details_(this), execute_tab_id_(-1) {
 }
 
 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;
   }
   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() {
   content::WebContents* contents = NULL;
 
   // If |tab_id| is specified, look for the tab. Otherwise default to selected
   // tab in the current window.
   CHECK_GE(execute_tab_id_, 0);
   if (!GetTabById(execute_tab_id_, browser_context(), include_incognito(),
                   nullptr, nullptr, &contents, nullptr, &error_)) {
@@ skipping 262 matching lines @@
       params->tab_id
           ? ErrorUtils::FormatErrorMessage(keys::kCannotDiscardTab,
                                            base::IntToString(*params->tab_id))
           : keys::kCannotFindTabToDiscard));
 }
 
 TabsDiscardFunction::TabsDiscardFunction() {}
 TabsDiscardFunction::~TabsDiscardFunction() {}
 
 }  // namespace extensions