WebUI: DisallowJavascript only on Refresh and non-same-page navigations

content/browser/webui/web_ui_impl.cc

Index: content/browser/webui/web_ui_impl.cc
diff --git a/content/browser/webui/web_ui_impl.cc b/content/browser/webui/web_ui_impl.cc
index 8e3aef02d4a46377df213cb9d3a7d8db6266a367..2fdc9803266066423feaff0f3fb4589343c31475 100644
--- a/content/browser/webui/web_ui_impl.cc
+++ b/content/browser/webui/web_ui_impl.cc
@@ -18,6 +18,7 @@
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_ui_controller.h"

[Charlie Reis, 2016/06/24 23:15:00]

IWYU nit: web_contents_observer.h

[tommycli, 2016/06/24 23:55:36]

Done.

@@ -27,6 +28,25 @@
 
 namespace content {
 
+class WebUIImpl::MainFrameNavigationObserver : public WebContentsObserver {
+ public:
+  MainFrameNavigationObserver(WebUIImpl* parent, WebContents* contents)
+      : WebContentsObserver(contents), parent_(parent) {}
+  ~MainFrameNavigationObserver() override {}
+
+ private:
+  void DidFinishNavigation(NavigationHandle* navigation_handle) override {
+    if (!navigation_handle->IsInMainFrame() ||

[Charlie Reis, 2016/06/24 23:15:00]

// Only disallow Javascript on cross-document navigations in the main frame.

[tommycli, 2016/06/24 23:55:36]

Done.

+        !navigation_handle->HasCommitted() || navigation_handle->IsSamePage()) {
+      return;
+    }
+
+    parent_->DisallowJavascriptOnAllHandlers();
+  }
+
+  WebUIImpl* parent_;

[Charlie Reis, 2016/06/24 23:15:00]

web_ui_, perhaps?  (Parent is a confusing term, since it might refer to frames
or other hierarchical WebContents things like <webview>.)

[tommycli, 2016/06/24 23:55:36]

Done.

+};
+
 const WebUI::TypeID WebUI::kNoWebUI = NULL;
 
 // static
@@ -50,6 +70,7 @@ WebUIImpl::WebUIImpl(WebContents* contents, const std::string& frame_name)
     : link_transition_type_(ui::PAGE_TRANSITION_LINK),
       bindings_(BINDINGS_POLICY_WEB_UI),
       web_contents_(contents),
+      web_contents_observer_(new MainFrameNavigationObserver(this, contents)),
       frame_name_(frame_name) {
   DCHECK(contents);
 }
@@ -95,14 +116,10 @@ void WebUIImpl::RenderViewReused(RenderViewHost* render_view_host,
     GURL site_url = render_view_host->GetSiteInstance()->GetSiteURL();
     GetContentClient()->browser()->LogWebUIUrl(site_url);

[Charlie Reis, 2016/06/24 23:15:01]

Ooh, I wonder if we can get rid of RenderViewReused entirely by doing this
logging in DidFinishNavigation...  That would be really nice.

We probably shouldn't do it in this CL, though, to avoid the risk of affecting
logs and possibly needing to revert.  Does it sound worth trying in a followup
CL?

[tommycli, 2016/06/24 23:55:36]

Yeah I think that would be eminently doable. But yes, like you said, in a
separate CL.

   }
-
-  for (WebUIMessageHandler* handler : handlers_)
-    handler->RenderViewReused();
 }
 
 void WebUIImpl::RenderFrameHostSwappingOut() {
-  for (WebUIMessageHandler* handler : handlers_)
-    handler->DisallowJavascript();
+  DisallowJavascriptOnAllHandlers();
 }
 
 WebContents* WebUIImpl::GetWebContents() const {
@@ -284,4 +301,9 @@ void WebUIImpl::AddToSetIfFrameNameMatches(
     frame_set->insert(host);
 }
 
+void WebUIImpl::DisallowJavascriptOnAllHandlers() {
+  for (WebUIMessageHandler* handler : handlers_)
+    handler->DisallowJavascript();
+}
+
 }  // namespace content