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

chrome/renderer/content_settings_observer.cc

 // Copyright (c) 2012 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 "chrome/renderer/content_settings_observer.h"
 
 #include "base/command_line.h"
 #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"
 #include "third_party/WebKit/public/platform/WebContentSettingCallbacks.h"
 #include "third_party/WebKit/public/platform/WebSecurityOrigin.h"
 #include "third_party/WebKit/public/platform/WebURL.h"
 #include "third_party/WebKit/public/web/WebDataSource.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebFrameClient.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
@@ skipping 10 matching lines @@
 #endif
 
 using blink::WebContentSettingCallbacks;
 using blink::WebDataSource;
 using blink::WebDocument;
 using blink::WebFrame;
 using blink::WebSecurityOrigin;
 using blink::WebString;
 using blink::WebURL;
 using blink::WebView;
-using content::DocumentState;
-using content::NavigationState;
 
 namespace {
 
 // This enum is histogrammed, so do not add, reorder, or remove values.
 enum {
   INSECURE_CONTENT_DISPLAY = 0,
   INSECURE_CONTENT_DISPLAY_HOST_GOOGLE,      // deprecated
   INSECURE_CONTENT_DISPLAY_HOST_WWW_GOOGLE,  // deprecated
   INSECURE_CONTENT_DISPLAY_HTML,
   INSECURE_CONTENT_RUN,
@@ skipping 172 matching lines @@
 
   return false;
 }
 
 void ContentSettingsObserver::DidCommitProvisionalLoad(
     bool is_new_navigation,
     bool is_same_page_navigation) {
   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
   DCHECK(frame->document().getSecurityOrigin().toString() == "null" ||
          !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) {
   WebFrame* frame = render_frame()->GetWebFrame();
   if (frame->getSecurityOrigin().isUnique() ||
       frame->top()->getSecurityOrigin().isUnique())
     return false;
 
   bool result = false;
   Send(new ChromeViewHostMsg_AllowDatabase(
@@ skipping 363 matching lines @@
 
   // If the scheme is file:, an empty file name indicates a directory listing,
   // which requires JavaScript to function properly.
   if (base::EqualsASCII(protocol, url::kFileScheme)) {
     return document_url.SchemeIs(url::kFileScheme) &&
            document_url.ExtractFileName().empty();
   }
 
   return false;
 }