Notify SSLManager when all password fields on a page are gone

components/password_manager/content/browser/content_password_manager_driver.cc

Index: components/password_manager/content/browser/content_password_manager_driver.cc
diff --git a/components/password_manager/content/browser/content_password_manager_driver.cc b/components/password_manager/content/browser/content_password_manager_driver.cc
index 493384977d5f62767cf4b6ddfedf24b6246de217..f56614c9e9828d8e9e0798cf7c4ef5ab2eab9768 100644
--- a/components/password_manager/content/browser/content_password_manager_driver.cc
+++ b/components/password_manager/content/browser/content_password_manager_driver.cc
@@ -4,6 +4,8 @@
 
 #include "components/password_manager/content/browser/content_password_manager_driver.h"
 
+#include <set>
+
 #include "components/autofill/content/browser/content_autofill_driver.h"
 #include "components/autofill/core/common/form_data.h"
 #include "components/autofill/core/common/password_form.h"
@@ -16,6 +18,7 @@
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
@@ -30,13 +33,88 @@ namespace password_manager {
 
 namespace {
 
-void MaybeNotifyPasswordInputShownOnHttp(content::RenderFrameHost* rfh) {
-  content::WebContents* web_contents =
-      content::WebContents::FromRenderFrameHost(rfh);
-  if (!content::IsOriginSecure(web_contents->GetVisibleURL())) {
-    web_contents->OnPasswordInputShownOnHttp();
+const char kVisiblePasswordObserverWebContentsUserDataKey[] =
+    "visible_password_observer";
+
+// This class tracks password visibility notifications for the
+// RenderFrameHosts in a WebContents. There is one

[vabr (Chromium), 2016/11/03 14:57:44]

Could this WebContentsObserver be simply merged with
ContentPasswordManagerDriverFactory?

The code structure right now is that ContentPasswordManagerDriverFactory
observes a WebContent and records all frames within it, keeping one
ContentPasswordManagerDriver per frame. It seems natural to enhance
ContentPasswordManagerDriverFactory to also keep information about the number of
password forms in each frame.

The current CL duplicates this for every frame within one WebContents.

[estark, 2016/11/03 15:13:35]

I did think about putting this logic in ContentPasswordManagerDriverFactory, but
I thought it would be a bit odd. I figured that counting password fields has
nothing to do with ContentPasswordManagerDriverFactory's job (which is to
produce ContentPasswordManagerDrivers) and I would be sticking it there simply
because its lifetime was the lifetime that I wanted for my object. So to me it
felt like a bit of an abuse of ContentPasswordManagerDriverFactory to put this
logic there. However, if you'd prefer it there, I'm happy to do that instead.

(Note that the VisiblePasswordObserver is not duplicated for every frame within
a WebContents; there only exists one per WebContents and each frame gets or
creates it as necessary.)

[vabr (Chromium), 2016/11/03 15:19:59]

Good point about keeping the factory focused on just creating an owning the
frames. And also you are right about the duplication not happening, I missed
that.

Still, it seems a bit confusing to have this crammed into the same file as the
driver. What about putting it in its own file?

+// VisiblePasswordObserver per WebContents. When a RenderFrameHost has a
+// visible password field and the top-level URL is HTTP, the
+// VisiblePasswordObserver notifies the WebContents, which allows the
+// content embedder to adjust security UI. Similarly, when all
+// RenderFrameHosts have hidden their password fields (either because
+// the renderer sent a message reporting that all password fields are
+// gone, or because the renderer crashed), the WebContents is notified
+// so that security warnings can be removed by the embedder.
+class VisiblePasswordObserver : public content::WebContentsObserver,
+                                public base::SupportsUserData::Data {

[dcheng, 2016/11/03 05:33:34]

I'd recommend using WebContentsUserData, which helps abstract out some of this
stuff.

[estark, 2016/11/03 15:13:35]

Noted, will do once I work out with vabr (above) where this code should live.

+ public:
+  static VisiblePasswordObserver* GetOrCreateForWebContents(
+      content::WebContents* web_contents) {
+    VisiblePasswordObserver* observer =

[vabr (Chromium), 2016/11/03 15:29:41]

In addition to moving this out to its own .h/.cc file, please also move the
method definitions out of line (i.e., declarations in .h, definitions in .cc).

[estark, 2016/11/03 18:54:13]

Done.

+        static_cast<VisiblePasswordObserver*>(web_contents->GetUserData(
+            kVisiblePasswordObserverWebContentsUserDataKey));
+    if (observer) {
+      return observer;
+    }
+
+    observer = new VisiblePasswordObserver(web_contents);
+    web_contents->SetUserData(kVisiblePasswordObserverWebContentsUserDataKey,
+                              observer);
+    return observer;
   }
-}
+
+  void RenderFrameHasVisiblePasswordField(
+      content::RenderFrameHost* render_frame_host) {
+    frame_tree_nodes_with_visible_password_fields_.insert(
+        render_frame_host->GetFrameTreeNodeId());
+    MaybeNotifyPasswordInputShownOnHttp();
+  }
+
+  void RenderFrameHasNoVisiblePasswordFields(
+      content::RenderFrameHost* render_frame_host) {
+    frame_tree_nodes_with_visible_password_fields_.erase(
+        render_frame_host->GetFrameTreeNodeId());
+    MaybeNotifyAllFieldsInvisible();
+  }
+
+  // WebContentsObserver:
+  void RenderFrameDeleted(
+      content::RenderFrameHost* render_frame_host) override {
+    // If a renderer process crashes, it won't send notifications that
+    // the password fields have been hidden, so watch for crashing
+    // processes and remove them from
+    // |frame_tree_nodes_with_visible_password_fields_|.
+    frame_tree_nodes_with_visible_password_fields_.erase(
+        render_frame_host->GetFrameTreeNodeId());
+    MaybeNotifyAllFieldsInvisible();
+  }
+
+ private:
+  VisiblePasswordObserver(content::WebContents* web_contents)
+      : content::WebContentsObserver(web_contents),
+        web_contents_(web_contents) {}
+
+  ~VisiblePasswordObserver() override {}
+
+  void MaybeNotifyPasswordInputShownOnHttp() {
+    if (!content::IsOriginSecure(web_contents_->GetVisibleURL())) {

[vabr (Chromium), 2016/11/03 15:29:41]

Just a question: the status of the UI indication depends also on the URL. Yet,
this code does not listen to changes in there (i.e., in navigations). I suppose
that is OK, because each such change triggers one of the RenderFrameHas...
notifications anyway?

[estark, 2016/11/03 18:54:13]

That's right. On navigation, we get messages from the renderer process that its
password fields are hidden as the Document detaches. It's only if the renderer
process has crashed that we wouldn't get the expected notifications on
navigation. I'll add a comment somewhere explaining this.

+      web_contents_->OnPasswordInputShownOnHttp();
+    }
+  }
+
+  void MaybeNotifyAllFieldsInvisible() {
+    if (frame_tree_nodes_with_visible_password_fields_.empty() &&
+        !content::IsOriginSecure(web_contents_->GetVisibleURL())) {
+      web_contents_->OnAllPasswordInputsHiddenOnHttp();
+    }
+  }
+
+  content::WebContents* web_contents_;
+  std::set<int> frame_tree_nodes_with_visible_password_fields_;
+
+  DISALLOW_COPY_AND_ASSIGN(VisiblePasswordObserver);
+};
 
 }  // namespace
 
@@ -208,14 +286,18 @@ void ContentPasswordManagerDriver::OnFocusedPasswordFormFound(
 }
 
 void ContentPasswordManagerDriver::PasswordFieldVisibleInInsecureContext() {
-  MaybeNotifyPasswordInputShownOnHttp(render_frame_host_);
+  VisiblePasswordObserver* observer =
+      VisiblePasswordObserver::GetOrCreateForWebContents(
+          content::WebContents::FromRenderFrameHost(render_frame_host_));
+  observer->RenderFrameHasVisiblePasswordField(render_frame_host_);
 }
 
 void ContentPasswordManagerDriver::
     AllPasswordFieldsInInsecureContextInvisible() {
-  // TODO(estark): if all frames in the frame tree have their password
-  // fields hidden, then notify the WebContents that there are no
-  // visible password fields left. https://crbug.com/658764
+  VisiblePasswordObserver* observer =
+      VisiblePasswordObserver::GetOrCreateForWebContents(
+          content::WebContents::FromRenderFrameHost(render_frame_host_));
+  observer->RenderFrameHasNoVisiblePasswordFields(render_frame_host_);
 }
 
 void ContentPasswordManagerDriver::DidNavigateFrame(