Reset content settings caches during provisional load start instead of commit.

chrome/renderer/content_settings_observer.cc

Index: chrome/renderer/content_settings_observer.cc
diff --git a/chrome/renderer/content_settings_observer.cc b/chrome/renderer/content_settings_observer.cc
index 2e48be56122421ad169c48629000ce9051e82c19..0574d4fb790ed13999c2c5a7b2e40b71899b72bb 100644
--- a/chrome/renderer/content_settings_observer.cc
+++ b/chrome/renderer/content_settings_observer.cc
@@ -8,7 +8,6 @@
 #include "base/metrics/histogram.h"
 #include "components/content_settings/content/common/content_settings_messages.h"
 #include "content/public/common/url_constants.h"
-#include "content/public/renderer/document_state.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_view.h"
 #include "third_party/WebKit/public/platform/URLConversion.h"
@@ -39,8 +38,6 @@ using blink::WebSecurityOrigin;
 using blink::WebString;
 using blink::WebURL;
 using blink::WebView;
-using content::DocumentState;
-using content::NavigationState;
 
 namespace {
 
@@ -233,17 +230,6 @@ void ContentSettingsObserver::DidCommitProvisionalLoad(
   WebFrame* frame = render_frame()->GetWebFrame();
   if (frame->parent())
     return;  // Not a top-level navigation.
-
-  if (!is_same_page_navigation) {
-    // Clear "block" flags for the new page. This needs to happen before any of
-    // |allowScript()|, |allowScriptFromSource()|, |allowImage()|, or
-    // |allowPlugins()| is called for the new page so that these functions can
-    // correctly detect that a piece of content flipped from "not blocked" to
-    // "blocked".
-    ClearBlockedContentSettings();
-    temporarily_allowed_plugins_.clear();
-  }
-
   GURL url = frame->document().url();
   // If we start failing this DCHECK, please makes sure we don't regress
   // this bug: http://code.google.com/p/chromium/issues/detail?id=79304
@@ -251,6 +237,20 @@ void ContentSettingsObserver::DidCommitProvisionalLoad(
          !url.SchemeIs(url::kDataScheme));
 }
 
+void ContentSettingsObserver::DidStartProvisionalLoad() {

[Bernhard Bauer, 2016/03/29 08:33:33]

Hm... isn't this called when we send out a network request for a new page? Then
it could happen that the network request doesn't actually succeed (i.e. no
navigation takes place), but we'll still have reset these.

[meacer, 2016/03/29 18:34:36]

It sounds like this would only affect temporarily allowed plugins though, since
blocked content settings will just be re-read. The next best point to do this is
DidCreateNewDocument, but there is at least one call to canExecuteScript between
DidStartProvisionalLoad and DidCreateNewDocument so it's probably best to keep
it in DidStartProvisionalLoad.

I think clearing the plugin exception when we hit a network error is an edge
case, but if you think we shouldn't do that, I can move
|temporarily_allowed_plugins_.clear()| line to DidCreateNewDocument.

+  // Clear "block" flags for the new page. This needs to happen before any of
+  // |allowScript()|, |allowScriptFromSource()|, |allowImage()|, or
+  // |allowPlugins()| is called for the new page so that these functions can
+  // correctly detect that a piece of content flipped from "not blocked" to
+  // "blocked".
+  // This also needs to happen before the document load begins, as parsing the
+  // document can trigger calls to ScriptController::canExecuteScripts.
+  // There is no need to check for same page navigations here as those don't
+  // trigger DidStartProvisionalLoad.
+  ClearBlockedContentSettings();
+  temporarily_allowed_plugins_.clear();
+}
+
 bool ContentSettingsObserver::allowDatabase(const WebString& name,
                                             const WebString& display_name,
                                             unsigned long estimated_size) {