Separate validation failures from other failures in ExecuteCodeFunction.

Separate validation failures from other failures in ExecuteCodeFunction.

ExecuteCodeFunction::Init() used to treat all failures as fatal, leading to
validation failures, resulting in renderer kills.
This is likely the cause of high number of tabs.executeScript/tabs.insertCSS
failures we see in UMA.

During shutdown, the browser window can be null or it might not have
any active tabs. Make ExecuteCodeInTabFunction return an error instead
of blowing up the renderer with bad_message.
This CL treats these kind of of errors non-fatal and returns error
from ExtensionFunction instead.

Add unit test to cover one case of non-fatal failure.

BUG=642794
Test=Have an extension keep the browser busy with running scripts [1] and
shutdown the browser (Ctrl-c) while it is running those scripts. Renderer
kill shouldn't happen anymore.
[1] example:
var currentIter = 0;
var runScript = function() {
  ++currentIter;
  if (currentIter > 1000) return;
  chrome.tabs.executeScript(undefined, {file:"content.js", runScript);
};
chrome.browserAction.onClicked.addListener(runScript);

Review-Url: https://codereview.chromium.org/2630753003
Cr-Commit-Position: refs/heads/master@{#445887}
Committed: https://chromium.googlesource.com/chromium/src/+/c93597557d1bee873c2e47ad18639d1f1b34d2b2

[lazyboy, 2017-01-13 23:07:01]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[lazyboy, 2017-01-13 23:07:25]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2017-01-13 23:07:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 23:59:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 23:59:02]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-01-17 18:52:09]

nice!

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1798: if (Init() == SUCCESS &&
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 :))

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api_unittest.cc (right):

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:278: scoped_refptr<const
Extension> extension = test_util::CreateEmptyExtension();
nit: it seems like this could pretty easily break if we change the ordering of
the permissions check.  Can we give the extension permissions to run so that we
know the only error we can return is the one we expect?

https://codereview.chromium.org/2630753003/diff/20001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.h (right):

https://codereview.chromium.org/2630753003/diff/20001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:32: // ExtensionFunction
validation failure.
We should describe the importance of these.  In particular, validation failures
kill the renderer and should only be used for this that should never, ever
happen; failures return an error and should be used when it could happen in
certain circumstances.

https://codereview.chromium.org/2630753003/diff/20001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:66: InitResult
set_init_result(InitResult init_result, const std::string& error) {
This can only be called with FAILURE, right?  Can we assign that directly, or
DCHECK if you want to continue passing it in?

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

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1798: if (Init() == SUCCESS &&
On 2017/01/17 18:52:08, Devlin (possibly slow) wrote:
> 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 :))

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?

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api_unittest.cc (right):

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:278: scoped_refptr<const
Extension> extension = test_util::CreateEmptyExtension();
On 2017/01/17 18:52:09, Devlin (possibly slow) wrote:
> nit: it seems like this could pretty easily break if we change the ordering of
> the permissions check.  Can we give the extension permissions to run so that
we
> know the only error we can return is the one we expect?

Done.

https://codereview.chromium.org/2630753003/diff/20001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.h (right):

https://codereview.chromium.org/2630753003/diff/20001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:32: // ExtensionFunction
validation failure.
On 2017/01/17 18:52:09, Devlin (possibly slow) wrote:
> We should describe the importance of these.  In particular, validation
failures
> kill the renderer and should only be used for this that should never, ever
> happen; failures return an error and should be used when it could happen in
> certain circumstances.

Done.

https://codereview.chromium.org/2630753003/diff/20001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:66: InitResult
set_init_result(InitResult init_result, const std::string& error) {
On 2017/01/17 18:52:09, Devlin (possibly slow) wrote:
> This can only be called with FAILURE, right?  Can we assign that directly, or
> DCHECK if you want to continue passing it in?

Assigned directly via renamed method set_init_result_failure.

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

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1798: if (Init() == SUCCESS &&
On 2017/01/18 02:29:10, lazyboy wrote:
> On 2017/01/17 18:52:08, Devlin (possibly slow) wrote:
> > 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 :))
> 
> 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?

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]

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/2630753003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1798: if (Init() == SUCCESS &&
On 2017/01/24 00:11:43, Devlin (catching up) wrote:
> On 2017/01/18 02:29:10, lazyboy wrote:
> > On 2017/01/17 18:52:08, Devlin (possibly slow) wrote:
> > > 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 :))
> > 
> > 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?
> 
> 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.
Ah ok. I've added TODO to remove this.
> 
> 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.

[Devlin, 2017-01-24 16:09:05]

lgtm

https://codereview.chromium.org/2630753003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api_unittest.cc (right):

https://codereview.chromium.org/2630753003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:292: const char* kArgs =
"[\"\", {\"code\": \"\"}]";
It looks like raw string literals are allowed now, so this might be more
readable as:
kArgs = R"["", {"code":""}]"

https://codereview.chromium.org/2630753003/diff/60001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/2630753003/diff/60001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:170: error_ =
init_error_.value();
nit: since you're here, may as well make this SetError()

https://codereview.chromium.org/2630753003/diff/60001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.h (right):

https://codereview.chromium.org/2630753003/diff/60001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:71: InitResult
set_init_result_failure(const std::string& error) {
set_init_result_failure() is a little ambiguous, since we also have validation
failures.  I wonder if we should s/failure/error (and so have validation_failure
and error), for more parity with e.g. SetError(), lastError, et al?

[lazyboy, 2017-01-24 23:35:58]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[lazyboy, 2017-01-24 23:36:08]

https://codereview.chromium.org/2630753003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api_unittest.cc (right):

https://codereview.chromium.org/2630753003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api_unittest.cc:292: const char* kArgs =
"[\"\", {\"code\": \"\"}]";
On 2017/01/24 16:09:05, Devlin (catching up) wrote:
> It looks like raw string literals are allowed now, so this might be more
> readable as:
> kArgs = R"["", {"code":""}]"

Neat. Done.

https://codereview.chromium.org/2630753003/diff/60001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.cc (right):

https://codereview.chromium.org/2630753003/diff/60001/extensions/browser/api/...
extensions/browser/api/execute_code_function.cc:170: error_ =
init_error_.value();
On 2017/01/24 16:09:05, Devlin (catching up) wrote:
> nit: since you're here, may as well make this SetError()

Done.

https://codereview.chromium.org/2630753003/diff/60001/extensions/browser/api/...
File extensions/browser/api/execute_code_function.h (right):

https://codereview.chromium.org/2630753003/diff/60001/extensions/browser/api/...
extensions/browser/api/execute_code_function.h:71: InitResult
set_init_result_failure(const std::string& error) {
On 2017/01/24 16:09:05, Devlin (catching up) wrote:
> set_init_result_failure() is a little ambiguous, since we also have validation
> failures.  I wonder if we should s/failure/error (and so have
validation_failure
> and error), for more parity with e.g. SetError(), lastError, et al?

OK. Done.

[commit-bot: I haz the power, 2017-01-24 23:36:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 00:47:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 00:47:16]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-01-25 00:53:06]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2017-01-25 00:53:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2630753003/#ps80001
(title: "address comments")

[commit-bot: I haz the power, 2017-01-25 00:54:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 01:01:25]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485305586363490,
"parent_rev": "70bbb59493f1111361e3b1bedd04a14fedb334f4", "commit_rev":
"c93597557d1bee873c2e47ad18639d1f1b34d2b2"}

[commit-bot: I haz the power, 2017-01-25 01:01:55]

Description was changed from

==========
Separate validation failures from other failures in ExecuteCodeFunction.

ExecuteCodeFunction::Init() used to treat all failures as fatal, leading to
validation failures, resulting in renderer kills.
This is likely the cause of high number of tabs.executeScript/tabs.insertCSS
failures we see in UMA.

During shutdown, the browser window can be null or it might not have
any active tabs. Make ExecuteCodeInTabFunction return an error instead
of blowing up the renderer with bad_message.
This CL treats these kind of of errors non-fatal and returns error
from ExtensionFunction instead.

Add unit test to cover one case of non-fatal failure.

BUG=642794
Test=Have an extension keep the browser busy with running scripts [1] and
shutdown the browser (Ctrl-c) while it is running those scripts. Renderer
kill shouldn't happen anymore.
[1] example:
var currentIter = 0;
var runScript = function() {
  ++currentIter;
  if (currentIter > 1000) return;
  chrome.tabs.executeScript(undefined, {file:"content.js", runScript);
};
chrome.browserAction.onClicked.addListener(runScript);
==========

to

==========
Separate validation failures from other failures in ExecuteCodeFunction.

ExecuteCodeFunction::Init() used to treat all failures as fatal, leading to
validation failures, resulting in renderer kills.
This is likely the cause of high number of tabs.executeScript/tabs.insertCSS
failures we see in UMA.

During shutdown, the browser window can be null or it might not have
any active tabs. Make ExecuteCodeInTabFunction return an error instead
of blowing up the renderer with bad_message.
This CL treats these kind of of errors non-fatal and returns error
from ExtensionFunction instead.

Add unit test to cover one case of non-fatal failure.

BUG=642794
Test=Have an extension keep the browser busy with running scripts [1] and
shutdown the browser (Ctrl-c) while it is running those scripts. Renderer
kill shouldn't happen anymore.
[1] example:
var currentIter = 0;
var runScript = function() {
  ++currentIter;
  if (currentIter > 1000) return;
  chrome.tabs.executeScript(undefined, {file:"content.js", runScript);
};
chrome.browserAction.onClicked.addListener(runScript);

Review-Url: https://codereview.chromium.org/2630753003
Cr-Commit-Position: refs/heads/master@{#445887}
Committed:
https://chromium.googlesource.com/chromium/src/+/c93597557d1bee873c2e47ad1863...
==========

[commit-bot: I haz the power, 2017-01-25 01:01:56]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c93597557d1bee873c2e47ad1863...