Fixed flakiness of context_lost tests on GPU bots.

content/browser/frame_host/navigation_controller_impl.cc

 // Copyright 2013 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 "content/browser/frame_host/navigation_controller_impl.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"  // Temporary
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
+#include "cc/base/switches.h"
 #include "content/browser/browser_url_handler_impl.h"
 #include "content/browser/dom_storage/dom_storage_context_wrapper.h"
 #include "content/browser/dom_storage/session_storage_namespace_impl.h"
 #include "content/browser/frame_host/debug_urls.h"
 #include "content/browser/frame_host/interstitial_page_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigation_entry_screenshot_manager.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"  // Temporary
 #include "content/browser/site_instance_impl.h"
 #include "content/common/frame_messages.h"
@@ skipping 981 matching lines @@
     new_entry->set_page_type(PAGE_TYPE_NORMAL);
     update_virtual_url = new_entry->update_virtual_url_with_url();
   } else {
     new_entry = new NavigationEntryImpl;
 
     // Find out whether the new entry needs to update its virtual URL on URL
     // change and set up the entry accordingly. This is needed to correctly
     // update the virtual URL when replaceState is called after a pushState.
     GURL url = params.url;
     bool needs_update = false;
-    BrowserURLHandlerImpl::GetInstance()->RewriteURLIfNecessary(
-        &url, browser_context_, &needs_update);
+    // Short-circuit the URL rewriting -- and consequent duplicate
+    // handling of debug URL handlers -- in the case where this is a
+    // debug URL and it would be handled in the BrowserURLHandlerImpl.
+    // URLs are rewritten twice, once when page navigation begins and
+    // once when the renderer completes the page navigation, and these
+    // debug URLs must be handled exactly once.

[Charlie Reis, 2014/05/09 23:21:51]

Sorry, it took me a while to understand what this comment meant, and thus why
this was correct.  Can we rewrite it to be a little more concrete?  Something
like:

"We call RewriteURLIfNecessary twice: once when page navigation begins in
CreateNavigationEntry, and once here when it commits.  For debug URLs, rewriting
the URL causes an action to occur and thus should not be done a second time."

[Ken Russell (switch to Gerrit), 2014/05/09 23:43:05]

Yes, thanks. Done.

+    bool skip_rewrite =
+        IsDebugURL(url) && base::CommandLine::ForCurrentProcess()->HasSwitch(
+            cc::switches::kEnableGpuBenchmarking);

[jam, 2014/05/09 22:47:46]

i can't tell what kEnableGpuBenchmarking has to do with this code?

[Charlie Reis, 2014/05/09 23:21:51]

+1.  I think we want to skip the second rewriting call for all debug URLs,
right?

[Ken Russell (switch to Gerrit), 2014/05/09 23:43:05]

Most of the time, the browser's debug URL handler will ignore debug URLs. I only
added support for processing them when kEnableGpuBenchmarking is enabled, since
most of the time they are navigated to by the omnibox. See Issue 308675 and
https://codereview.chromium.org/27603004 , and DebugURLHandler in
browser_url_handler_impl.cc.

I was aiming for the minimum behavioral change with this CL. I'd be glad to
expand the effects if you like, but I think it's worth being conservative.

[Charlie Reis, 2014/05/09 23:53:40]

Oh, I misunderstood what was happening.  Yes, let's keep the flag check here,
since it only matters in that case.  (It's admittedly a little awkward to have
this logic here, but maybe we can revisit how you're handling these URLs later.)

Accordingly, let's change the comment above to:

"We call RewriteURLIfNecessary twice: once when page navigation begins in
CreateNavigationEntry, and once here when it commits.  With the
kEnableGpuBenchmarking flag, the rewriting step includes handling debug URLs
which cause an action to occur, and thus we should not rewrite them a second
time."

[Ken Russell (switch to Gerrit), 2014/05/09 23:56:40]

Done.

+    if (!skip_rewrite) {
+      BrowserURLHandlerImpl::GetInstance()->RewriteURLIfNecessary(
+          &url, browser_context_, &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
     // update the virtual URL based on the new URL. For example, this is needed
     // to show chrome://bookmarks/#1 when the bookmarks webui extension changes
     // the URL.
     update_virtual_url = needs_update;
   }
 
   new_entry->SetURL(params.url);
@@ skipping 713 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content