Make WebUIImpl a WebContentsObserver to re-use RenderViewCreated()

content/browser/webui/web_ui_impl.h

 // 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.
 
 #ifndef CONTENT_BROWSER_WEBUI_WEB_UI_IMPL_H_
 #define CONTENT_BROWSER_WEBUI_WEB_UI_IMPL_H_
 
 #include <map>
 #include <set>
 
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_ui.h"
-#include "ipc/ipc_listener.h"
 
 namespace content {
 class RenderFrameHost;
 class RenderViewHost;
 
 class CONTENT_EXPORT WebUIImpl : public WebUI,
-                                 public IPC::Listener,
+                                 public WebContentsObserver,
                                  public base::SupportsWeakPtr<WebUIImpl> {
  public:
   explicit WebUIImpl(WebContents* contents);
   ~WebUIImpl() override;
 
-  // Called by WebContentsImpl when the RenderView is first created. This is
-  // *not* called for every page load because in some cases
-  // RenderFrameHostManager will reuse RenderView instances.
-  void RenderViewCreated(RenderViewHost* render_view_host);
-
   // WebUI implementation:
   WebContents* GetWebContents() const override;
   WebUIController* GetController() const override;
   void SetController(WebUIController* controller) override;
   float GetDeviceScaleFactor() const override;
   const base::string16& GetOverriddenTitle() const override;
   void OverrideTitle(const base::string16& title) override;
   ui::PageTransition GetLinkTransitionType() const override;
   void SetLinkTransitionType(ui::PageTransition type) override;
   int GetBindings() const override;
@@ skipping 18 matching lines @@
                               const base::Value& arg3) override;
   void CallJavascriptFunction(const std::string& function_name,
                               const base::Value& arg1,
                               const base::Value& arg2,
                               const base::Value& arg3,
                               const base::Value& arg4) override;
   void CallJavascriptFunction(
       const std::string& function_name,
       const std::vector<const base::Value*>& args) override;
 
-  // IPC::Listener implementation:
+  // WebContentsObserver implementation:
   bool OnMessageReceived(const IPC::Message& message) override;
+  void RenderViewCreated(RenderViewHost* render_view_host) override;
 
  private:
   // IPC message handling.
   void OnWebUISend(const GURL& source_url,
                    const std::string& message,
                    const base::ListValue& args);
 
   // Execute a string of raw JavaScript on the page.
   void ExecuteJavascript(const base::string16& javascript);
 
@@ skipping 15 matching lines @@
   // bool options default to false. See the public getters for more information.
   base::string16 overridden_title_;  // Defaults to empty string.
   ui::PageTransition link_transition_type_;  // Defaults to LINK.
   int bindings_;  // The bindings from BindingsPolicy that should be enabled for
                   // this page.
 
   // The WebUIMessageHandlers we own.
   ScopedVector<WebUIMessageHandler> handlers_;
 
   // Non-owning pointer to the WebContents this WebUI is associated with.
   WebContents* web_contents_;

[Dan Beam, 2015/08/28 01:50:53]

NOTE: I attempted to use WebContentsObserver::web_contents() and remove
WebUIImpl::web_contents_, but WebContentsObserver::ResetWebContents() sets
WebContentsObserver::web_contents_ = nullptr[1] before
RenderFrameHostManager::web_ui_ dies[2].  Some webui handlers call
WebUIImpl::GetWebContents() in their destructors to clean up state, so they need
WebUIImpl::GetWebContents() to remain non-null.[3][4]

I assume this is safe but subtle.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
[4]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

[Charlie Reis, 2015/08/28 17:05:56]

Thanks for looking into this.  It seems a bit dangerous for WebUI objects to use
a mostly destructed WebContents, so it's worth seeing if we can fix this and use
WCO::web_contents().  That way we'll be consistent with other WebContents
observers, which get notified at a reasonably safe point in time.

Profile::FromWebUI just needs the BrowserContext from the WebContents.  Maybe we
could store that on WebUI instead of web_contents_, since it's guaranteed to
outlive the RFHM that keeps most WebUI objects alive?  (Not sure if there are
other uses of GetWebContents in various destructors, though.)

Alternatively, we could have ~WebContentsImpl tell the root RFHM to delete its
web_ui_ before calling ResetWebContents.  (The subframe RFHMs have already been
deleted by that point, so we don't need to worry about them.)

NOTE: This issue exists even before this CL lands, so I'm fine with addressing
it in a followup CL.  If you want to do that, just leave a TODO here.

[Dan Beam, 2015/08/28 17:13:16]

I'd rather nuke RenderFrameHostManager::web_ui_ earlier, yeah.  I'll see how
safe that is.

 
   // The name of the iframe this WebUI is embedded in (empty if not explicitly
   // overridden with OverrideJavaScriptFrame).
   std::string frame_name_;
 
   scoped_ptr<WebUIController> controller_;
 
   DISALLOW_COPY_AND_ASSIGN(WebUIImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_WEBUI_WEB_UI_IMPL_H_