New issue
Advanced search Search tips

Issue 642794 link

Starred by 3 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

tabs.executeScript calls are failing validation; killing renderers

Project Member Reported by rdevlin....@chromium.org, Aug 31 2016

Issue description

UMA [1] seems to indicate that tabs.executeScript calls are accounting for far too many process kills.  This happens when something on the browser fails validation in such a way that the *only* possible explanation would be a compromised (either fully or even just through our JS bindings) renderer, but it having a single API account for 98% of function kills seems suspicious.  We should investigate.

[1] https://uma.googleplex.com/histograms?endDate=08-30-2016&dayCount=7&histograms=Extensions.BadMessageFunctionName&fixupData=true&showMax=true&filters=channel%2Ceq%2C4%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial
 
Owner: lazyboy@chromium.org
Status: Started (was: Untriaged)
The theory here is ExecuteCodeFunction::Init() can easily fail during shutdown, that also explains why the second most offending one is also ExecuteCodeFunction related: TABS_INSERTCSS.

We should relax Init() to not produce fatal errors that can happen during shutdown, e.g. getting default tab through GetDefaultTab()
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0605ec9d2876c3c58b3f91ba5e82ca04c6ad5828

commit 0605ec9d2876c3c58b3f91ba5e82ca04c6ad5828
Author: lazyboy <lazyboy@chromium.org>
Date: Tue Jan 17 23:28:49 2017

Upload crash reports when ExtensionFunctions generate bad messages.

ExtensionFunctionDispatcher used to have its own implementation of
bad message handling, which missed calling DumpWithoutCrashing().
Use extensions::bad_message namespace's functions instead, with
little unfortunate deviation in logging to be consistent with the
existing UMA.

Note that this is not a fix for issue 642794. This CL will help
tracking 642794.

BUG=642794
Test=Note that we should start seeing crash reports for bad
message termination of ExtensionFunctions. This should give
us a clue narrow down reasons that cause crbug 642794.

Review-Url: https://codereview.chromium.org/2629783002
Cr-Commit-Position: refs/heads/master@{#444183}

[modify] https://crrev.com/0605ec9d2876c3c58b3f91ba5e82ca04c6ad5828/extensions/browser/bad_message.cc
[modify] https://crrev.com/0605ec9d2876c3c58b3f91ba5e82ca04c6ad5828/extensions/browser/bad_message.h
[modify] https://crrev.com/0605ec9d2876c3c58b3f91ba5e82ca04c6ad5828/extensions/browser/extension_function_dispatcher.cc

Issue 492702 has been merged into this issue.
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c93597557d1bee873c2e47ad18639d1f1b34d2b2

commit c93597557d1bee873c2e47ad18639d1f1b34d2b2
Author: lazyboy <lazyboy@chromium.org>
Date: Wed Jan 25 01:01:14 2017

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}

[modify] https://crrev.com/c93597557d1bee873c2e47ad18639d1f1b34d2b2/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/c93597557d1bee873c2e47ad18639d1f1b34d2b2/chrome/browser/extensions/api/tabs/tabs_api.h
[modify] https://crrev.com/c93597557d1bee873c2e47ad18639d1f1b34d2b2/chrome/browser/extensions/api/tabs/tabs_api_unittest.cc
[modify] https://crrev.com/c93597557d1bee873c2e47ad18639d1f1b34d2b2/chrome/browser/extensions/api/tabs/tabs_constants.cc
[modify] https://crrev.com/c93597557d1bee873c2e47ad18639d1f1b34d2b2/chrome/browser/extensions/api/tabs/tabs_constants.h
[modify] https://crrev.com/c93597557d1bee873c2e47ad18639d1f1b34d2b2/extensions/browser/api/execute_code_function.cc
[modify] https://crrev.com/c93597557d1bee873c2e47ad18639d1f1b34d2b2/extensions/browser/api/execute_code_function.h
[modify] https://crrev.com/c93597557d1bee873c2e47ad18639d1f1b34d2b2/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
[modify] https://crrev.com/c93597557d1bee873c2e47ad18639d1f1b34d2b2/extensions/browser/api/guest_view/web_view/web_view_internal_api.h

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e465202abc260c9aa8319f8e77bf721e0453ed2

commit 5e465202abc260c9aa8319f8e77bf721e0453ed2
Author: lazyboy <lazyboy@chromium.org>
Date: Sat Jan 28 01:13:47 2017

Better crash stacktraces for ExtensionFunction bad messages.

Record crash trace right where we set bad_message in ExtensionFunction

This will give us clue about which particular extension functions are
crashing. Previously, we used to record the trace from common response
callback where we already lost track of which function actually set
a bad_message.

Turn ExtensionFunction::set_bad_message(bool) to EF::SetBadMessage(void)
since one should only use this function to set bad_message state, not
clear it.

BUG=642794
Test=No visible change. Beware though, similar to
https://codereview.chromium.org/2629783002, this might show up as a
blame CL for extension function crashes. These will not really
be new crashes, so file a (non-release-blocking) bug.

Review-Url: https://codereview.chromium.org/2656013005
Cr-Commit-Position: refs/heads/master@{#446865}

[modify] https://crrev.com/5e465202abc260c9aa8319f8e77bf721e0453ed2/extensions/browser/extension_function.cc
[modify] https://crrev.com/5e465202abc260c9aa8319f8e77bf721e0453ed2/extensions/browser/extension_function.h
[modify] https://crrev.com/5e465202abc260c9aa8319f8e77bf721e0453ed2/extensions/browser/extension_function_dispatcher.cc

Cc: -catmulli...@chromium.org
Cc: -asargent@chromium.org

Sign in to add a comment