Commit eabfe191 authored by kbr@chromium.org's avatar kbr@chromium.org

Fixed flakiness of context_lost tests on GPU bots.

Two failures were observed:

1. If the GPU process crashed between EstablishGpuChannel and
   CreateViewCommandBuffer, the channel would never be properly marked
   as lost. Now, if CreateViewCommandBuffer fails, mark the
   GpuChannelHost as lost on the renderer side. The RenderThreadImpl
   will then establish a new connection to the new GPU process.

2. In Issue 308675, support was added to allow Telemetry to navigate
   to about:gpucrash, specifically for this test. It turns out that
   each navigation from Telemetry was provoking *two* GPU process
   crashes. Depending on when they came in, the test would
   intermittently receive two lost context events and fail. Squelch
   the second debug URL handling in NavigationControllerImpl.

These fix the locally reproducible failures of this test.

BUG=365904
TEST=src/content/test/gpu/run_gpu_test.py context_lost --browser=debug --show-stdout -v --page-filter=WebGLContextLostFromGPUProcessExit --page-repeat=30

Review URL: https://codereview.chromium.org/277113002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269763 0039d316-1c4b-4281-b951-d872f2087c98
parent 42db8ec1
...@@ -43,6 +43,9 @@ bool HandleDebugURL(const GURL& url, PageTransition transition) { ...@@ -43,6 +43,9 @@ bool HandleDebugURL(const GURL& url, PageTransition transition) {
if (!(transition & PAGE_TRANSITION_FROM_ADDRESS_BAR)) if (!(transition & PAGE_TRANSITION_FROM_ADDRESS_BAR))
return false; return false;
// NOTE: when you add handling of any URLs to this function, also
// update IsDebugURL, below.
if (url.host() == kChromeUIBrowserCrashHost) { if (url.host() == kChromeUIBrowserCrashHost) {
// Induce an intentional crash in the browser process. // Induce an intentional crash in the browser process.
CHECK(false); CHECK(false);
...@@ -80,6 +83,19 @@ bool HandleDebugURL(const GURL& url, PageTransition transition) { ...@@ -80,6 +83,19 @@ bool HandleDebugURL(const GURL& url, PageTransition transition) {
return false; return false;
} }
bool IsDebugURL(const GURL& url) {
// NOTE: when you add any URLs to this list, also update
// HandleDebugURL, above.
return IsRendererDebugURL(url) ||
(url.is_valid() &&
(url.host() == kChromeUIBrowserCrashHost ||
url == GURL(kChromeUIGpuCleanURL) ||
url == GURL(kChromeUIGpuCrashURL) ||
url == GURL(kChromeUIGpuHangURL) ||
url == GURL(kChromeUIPpapiFlashCrashURL) ||
url == GURL(kChromeUIPpapiFlashHangURL)));
}
bool IsRendererDebugURL(const GURL& url) { bool IsRendererDebugURL(const GURL& url) {
if (!url.is_valid()) if (!url.is_valid())
return false; return false;
......
...@@ -15,6 +15,11 @@ namespace content { ...@@ -15,6 +15,11 @@ namespace content {
// handles it and returns true. // handles it and returns true.
bool HandleDebugURL(const GURL& url, PageTransition transition); bool HandleDebugURL(const GURL& url, PageTransition transition);
// Returns whether this given url is a debugging url. It is a superset
// of IsRendererDebugURL, below, and debugging urls that are handled
// in the browser process.
bool IsDebugURL(const GURL& url);
// Returns whether the given url is either a debugging url handled in the // Returns whether the given url is either a debugging url handled in the
// renderer process, such as one that crashes or hangs the renderer, or a // renderer process, such as one that crashes or hangs the renderer, or a
// javascript: URL that operates on the current page in the renderer. Such URLs // javascript: URL that operates on the current page in the renderer. Such URLs
......
...@@ -5,12 +5,14 @@ ...@@ -5,12 +5,14 @@
#include "content/browser/frame_host/navigation_controller_impl.h" #include "content/browser/frame_host/navigation_controller_impl.h"
#include "base/bind.h" #include "base/bind.h"
#include "base/command_line.h"
#include "base/debug/trace_event.h" #include "base/debug/trace_event.h"
#include "base/logging.h" #include "base/logging.h"
#include "base/strings/string_number_conversions.h" // Temporary #include "base/strings/string_number_conversions.h" // Temporary
#include "base/strings/string_util.h" #include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h" #include "base/strings/utf_string_conversions.h"
#include "base/time/time.h" #include "base/time/time.h"
#include "cc/base/switches.h"
#include "content/browser/browser_url_handler_impl.h" #include "content/browser/browser_url_handler_impl.h"
#include "content/browser/dom_storage/dom_storage_context_wrapper.h" #include "content/browser/dom_storage/dom_storage_context_wrapper.h"
#include "content/browser/dom_storage/session_storage_namespace_impl.h" #include "content/browser/dom_storage/session_storage_namespace_impl.h"
...@@ -1012,8 +1014,18 @@ void NavigationControllerImpl::RendererDidNavigateToNewPage( ...@@ -1012,8 +1014,18 @@ void NavigationControllerImpl::RendererDidNavigateToNewPage(
// update the virtual URL when replaceState is called after a pushState. // update the virtual URL when replaceState is called after a pushState.
GURL url = params.url; GURL url = params.url;
bool needs_update = false; bool needs_update = false;
BrowserURLHandlerImpl::GetInstance()->RewriteURLIfNecessary( // We call RewriteURLIfNecessary twice: once when page navigation
&url, browser_context_, &needs_update); // begins in CreateNavigationEntry, and once here when it commits.
// With the kEnableGpuBenchmarking flag, the rewriting includes
// handling debug URLs which cause an action to occur, and thus we
// should not rewrite them a second time.
bool skip_rewrite =
IsDebugURL(url) && base::CommandLine::ForCurrentProcess()->HasSwitch(
cc::switches::kEnableGpuBenchmarking);
if (!skip_rewrite) {
BrowserURLHandlerImpl::GetInstance()->RewriteURLIfNecessary(
&url, browser_context_, &needs_update);
}
new_entry->set_update_virtual_url_with_url(needs_update); new_entry->set_update_virtual_url_with_url(needs_update);
// When navigating to a new page, give the browser URL handler a chance to // When navigating to a new page, give the browser URL handler a chance to
......
...@@ -144,6 +144,19 @@ CommandBufferProxyImpl* GpuChannelHost::CreateViewCommandBuffer( ...@@ -144,6 +144,19 @@ CommandBufferProxyImpl* GpuChannelHost::CreateViewCommandBuffer(
int32 route_id = GenerateRouteID(); int32 route_id = GenerateRouteID();
if (!factory_->CreateViewCommandBuffer(surface_id, init_params, route_id)) { if (!factory_->CreateViewCommandBuffer(surface_id, init_params, route_id)) {
LOG(ERROR) << "GpuChannelHost::CreateViewCommandBuffer failed."; LOG(ERROR) << "GpuChannelHost::CreateViewCommandBuffer failed.";
// The most likely reason CreateViewCommandBuffer will fail is
// that the GPU process crashed. In this case the GPU channel
// needs to be considered lost. The caller will then set up a new
// connection, and the GPU channel and any view command buffers
// will all be associated with the same GPU process.
DCHECK(MessageLoopProxy::current().get());
scoped_refptr<base::MessageLoopProxy> io_loop = factory_->GetIOLoopProxy();
io_loop->PostTask(FROM_HERE,
base::Bind(&GpuChannelHost::MessageFilter::OnChannelError,
channel_filter_.get()));
return NULL; return NULL;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment