AppCache: Remove WeakPtrFactory from AppCacheResponseIO.

AppCacheResponseIO is an abstract (with pure virtual methods) base class
for classes that also use WeakPtrFactories. Some method in the base
class currently use the WeakPtrFactory embedded in the base class to
vend WeakPtrs, which will point to instances of the derived classes.
These pointers will be invalidated when the base class' WeakPtrFactory
is destroyed. So, there is a time window when the weak pointers are
valid, but the members in derived classes are being destroyed.

To avoid reasoning about such complexities, this CL removes the
WeakPtrFactory in the base class. Instead, base class methods use a
virtual method to get weak pointers from the derived classes' factories.

Change-Id: I343a0e1159d839b0f71fb09518704ce560b2c5ad
Reviewed-on: https://chromium-review.googlesource.com/1188825Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586360}

content/browser/appcache/appcache_response.cc

@@ -102,8 +102,7 @@ AppCacheResponseIO::AppCacheResponseIO(
     : response_id_(response_id),
       disk_cache_(std::move(disk_cache)),
       entry_(nullptr),
-      buffer_len_(0),
-      weak_factory_(this) {}
+      buffer_len_(0) {}
 
 AppCacheResponseIO::~AppCacheResponseIO() {
   if (entry_)
@@ -112,8 +111,8 @@ AppCacheResponseIO::~AppCacheResponseIO() {
 
 void AppCacheResponseIO::ScheduleIOCompletionCallback(int result) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&AppCacheResponseIO::OnIOComplete,
-                                weak_factory_.GetWeakPtr(), result));
+      FROM_HERE,
+      base::BindOnce(&AppCacheResponseIO::OnIOComplete, GetWeakPtr(), result));
 }
 
 void AppCacheResponseIO::InvokeUserCompletionCallback(int result) {
@@ -131,8 +130,7 @@ void AppCacheResponseIO::ReadRaw(int index, int offset,
   DCHECK(entry_);
   int rv = entry_->Read(
       index, offset, buf, buf_len,
-      base::Bind(&AppCacheResponseIO::OnRawIOComplete,
-                 weak_factory_.GetWeakPtr()));
+      base::BindOnce(&AppCacheResponseIO::OnRawIOComplete, GetWeakPtr()));
   if (rv != net::ERR_IO_PENDING)
     ScheduleIOCompletionCallback(rv);
 }
@@ -142,8 +140,7 @@ void AppCacheResponseIO::WriteRaw(int index, int offset,
   DCHECK(entry_);
   int rv = entry_->Write(
       index, offset, buf, buf_len,
-      base::Bind(&AppCacheResponseIO::OnRawIOComplete,
-                 weak_factory_.GetWeakPtr()));
+      base::BindOnce(&AppCacheResponseIO::OnRawIOComplete, GetWeakPtr()));
   if (rv != net::ERR_IO_PENDING)
     ScheduleIOCompletionCallback(rv);
 }
@@ -164,12 +161,12 @@ void AppCacheResponseIO::OpenEntryIfNeeded() {
     entry_ptr = new AppCacheDiskCacheInterface::Entry*;
     rv = disk_cache_->OpenEntry(
         response_id_, entry_ptr,
-        base::BindOnce(&AppCacheResponseIO::OpenEntryCallback,
-                       weak_factory_.GetWeakPtr(), entry_ptr));
+        base::BindOnce(&AppCacheResponseIO::OpenEntryCallback, GetWeakPtr(),
+                       entry_ptr));
   }
 
   if (rv != net::ERR_IO_PENDING)
-    OpenEntryCallback(weak_factory_.GetWeakPtr(), entry_ptr, rv);
+    OpenEntryCallback(GetWeakPtr(), entry_ptr, rv);
 }
 
 // static
@@ -320,6 +317,10 @@ void AppCacheResponseReader::OnOpenEntryComplete() {
     ContinueReadData();
 }
 
+base::WeakPtr<AppCacheResponseIO> AppCacheResponseReader::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 // AppCacheResponseWriter ----------------------------------------------
 
 AppCacheResponseWriter::AppCacheResponseWriter(
@@ -485,6 +486,10 @@ void AppCacheResponseWriter::OnCreateEntryComplete(
     writer->ContinueWriteData();
 }
 
+base::WeakPtr<AppCacheResponseIO> AppCacheResponseWriter::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 // AppCacheResponseMetadataWriter ----------------------------------------------
 
 AppCacheResponseMetadataWriter::AppCacheResponseMetadataWriter(
@@ -528,4 +533,8 @@ void AppCacheResponseMetadataWriter::OnIOComplete(int result) {
   // Note: |this| may have been deleted by the completion callback.
 }
 
+base::WeakPtr<AppCacheResponseIO> AppCacheResponseMetadataWriter::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 }  // namespace content

content/browser/appcache/appcache_response.h

@@ -137,6 +137,10 @@ class CONTENT_EXPORT AppCacheResponseIO {
   void WriteRaw(int index, int offset, net::IOBuffer* buf, int buf_len);
   void OpenEntryIfNeeded();
 
+  // Methods in this class use weak pointers. The weak pointer factories must be
+  // defined in the subclasses, to avoid use-after-free situations.
+  virtual base::WeakPtr<AppCacheResponseIO> GetWeakPtr() = 0;
+
   const int64_t response_id_;
   base::WeakPtr<AppCacheDiskCacheInterface> disk_cache_;
   AppCacheDiskCacheInterface::Entry* entry_;
@@ -145,7 +149,6 @@ class CONTENT_EXPORT AppCacheResponseIO {
   int buffer_len_;
   OnceCompletionCallback callback_;
   net::CompletionOnceCallback open_callback_;
-  base::WeakPtrFactory<AppCacheResponseIO> weak_factory_;
 
  private:
   void OnRawIOComplete(int result);
@@ -158,8 +161,7 @@ class CONTENT_EXPORT AppCacheResponseIO {
 // and there is a read in progress, the implementation will return
 // immediately but will take care of any side effect of cancelling the
 // operation.  In other words, instances are safe to delete at will.
-class CONTENT_EXPORT AppCacheResponseReader
-    : public AppCacheResponseIO {
+class CONTENT_EXPORT AppCacheResponseReader : public AppCacheResponseIO {
  public:
   ~AppCacheResponseReader() override;
 
@@ -206,6 +208,8 @@ class CONTENT_EXPORT AppCacheResponseReader
 
   void OnIOComplete(int result) override;
   void OnOpenEntryComplete() override;
+  base::WeakPtr<AppCacheResponseIO> GetWeakPtr() override;
+
   void ContinueReadInfo();
   void ContinueReadData();
 
@@ -221,8 +225,7 @@ class CONTENT_EXPORT AppCacheResponseReader
 // and there is a write in progress, the implementation will return
 // immediately but will take care of any side effect of cancelling the
 // operation. In other words, instances are safe to delete at will.
-class CONTENT_EXPORT AppCacheResponseWriter
-    : public AppCacheResponseIO {
+class CONTENT_EXPORT AppCacheResponseWriter : public AppCacheResponseIO {
  public:
   ~AppCacheResponseWriter() override;
 
@@ -273,6 +276,8 @@ class CONTENT_EXPORT AppCacheResponseWriter
   };
 
   void OnIOComplete(int result) override;
+  base::WeakPtr<AppCacheResponseIO> GetWeakPtr() override;
+
   void ContinueWriteInfo();
   void ContinueWriteData();
   void CreateEntryIfNeededAndContinue();
@@ -325,6 +330,7 @@ class CONTENT_EXPORT AppCacheResponseMetadataWriter
  private:
   void OnIOComplete(int result) override;
   void OnOpenEntryComplete() override;
+  base::WeakPtr<AppCacheResponseIO> GetWeakPtr() override;
 
   int write_amount_;
   base::WeakPtrFactory<AppCacheResponseMetadataWriter> weak_factory_;