Remember user decisions on invalid certificates behind a flag

content/public/browser/ssl_host_state_decisions.h

Index: content/public/browser/ssl_host_state_decisions.h
diff --git a/content/public/browser/ssl_host_state_decisions.h b/content/public/browser/ssl_host_state_decisions.h
new file mode 100644
index 0000000000000000000000000000000000000000..01d38d5d3647ea796927cd041f4a1123b28c5194
--- /dev/null
+++ b/content/public/browser/ssl_host_state_decisions.h
@@ -0,0 +1,44 @@
+// Copyright (c) 2014 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_SSL_HOST_STATE_DECISIONS_H_
+#define CONTENT_BROWSER_SSL_HOST_STATE_DECISIONS_H_
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "content/common/content_export.h"
+#include "net/cert/x509_certificate.h"
+
+class GURL;
+
+namespace content {
+
+// SSLHostStateDecisions must be implemented by the embedder, to provide the
+// storage strategy (if any) for certificate decisions.
+class CONTENT_EXPORT SSLHostStateDecisions
+    : public base::RefCountedThreadSafe<SSLHostStateDecisions> {

[Ryan Sleevi, 2014/07/08 23:53:29]

In general, I like to push back against introducing RefCounted classes, as they
generally lead to poor lifetime management.

Could you explain more why you can't have an explicit ownership transfer occur?

[jww, 2014/07/11 00:08:42]

So I'm pretty sure ownership transfer is out of the question. Since this,
ultimately, is returned as a profile service (like SpecialStoragePolicy, which I
modeled this on), it's effectively a singleton owned by the profile.

I'm not completely sure on this, but I'm under the impression that it's
theoretically possible for the profile to be pulled out from underneath code
(such as the interstitial code) that might have grabbed a reference to this,
thus we can't treat it as a raw pointer either. Thus, a ref counted pointer
seemed like the only option.

[Ryan Sleevi, 2014/07/11 00:48:49]

Except with RefCounting, you're always (potentially) doing an ownership
transfer. That's the real danger.

If not a transfer, a clear set of lifetimes and precedence should be
established.
That certainly highlights a bad code smell to me then. It seems wrong to keep
this alive even if the profile is gone.

[jww, 2014/07/11 01:52:07]

Ha. Looking again, I don't ever even assign it to a scoped_refptr, so clearly
this needs to go. Sorry about that.

+ public:
+  virtual void DenyCert(const GURL& url,
+                        net::X509Certificate* cert,
+                        net::CertStatus error) = 0;
+  virtual void AllowCert(const GURL& url,
+                         net::X509Certificate* cert,
+                         net::CertStatus error) = 0;
+  virtual void Clear() = 0;
+  virtual net::CertPolicy::Judgment QueryPolicy(const GURL& url,
+                                                net::X509Certificate* cert,
+                                                net::CertStatus error) = 0;
+  virtual void RevokeAllowAndDenyPreferences(const GURL& url) = 0;
+  virtual bool HasAllowedOrDeniedCert(const GURL& url) = 0;
+
+ protected:
+  virtual ~SSLHostStateDecisions() {}
+
+ private:
+  friend class base::RefCountedThreadSafe<SSLHostStateDecisions>;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_SSL_HOST_STATE_DECISIONS_H_