Fixes the cache lock issue.

net/http/http_cache.h

Index: net/http/http_cache.h
diff --git a/net/http/http_cache.h b/net/http/http_cache.h
index 2aae2fcf27e1cc0aed6fcbbe3b73eb3c3d293708..f41a8bedcf758104b6f0c7cf9926020e5bda80dd 100644
--- a/net/http/http_cache.h
+++ b/net/http/http_cache.h
@@ -229,15 +229,257 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
     kNumCacheEntryDataIndices
   };
 
+  class Transaction;
+  struct ActiveEntry;
+  typedef std::list<Transaction*> TransactionList;
+  typedef std::unordered_set<Transaction*> TransactionSet;
+
+  // SharedWriters represents the set of all HttpCache::Transactions that are
+  // reading from the network using the same network transaction and writing to
+  // the same cache entry. It is owned by the ActiveEntry.
+  //
+  // An instance of SharedWriters will be created when the first writer has
+  // written the new response headers in the cache.
+  // At creation:
+  // - The writer’s network transaction’s ownership will be transferred to
+  // SharedWriters at that time so that it can be used by any of the
+  // HttpCache::Transactions for subsequent reading from the network.
+  // - The pending_queue will be traversed to see if there are other
+  // transactions that can be added to SharedWriters.
+  //
+  // Destroyed when any of the following happen:
+  // - All the data has been read from the network and written to cache.
+  // - ActiveEntry is destroyed.
+  // - Last transaction part of SharedWriters is destroyed.
+  class SharedWriters {

[jkarlin, 2016/12/06 18:08:17]

Let's put this class in a new file and give it its own unittest file.

[shivanisha, 2016/12/06 21:53:35]

Good idea, done.

+   public:
+    // Creates a new SharedWriters object, transfers the ownership of network
+    // transaction to SharedWriters, and adds the cache transaction to
+    // all_writers_.
+    SharedWriters(HttpCache* cache,
+                  ActiveEntry* entry,
+                  Transaction* cache_trans,
+                  RequestPriority priority,
+                  std::unique_ptr<HttpTransaction> transaction);
+    ~SharedWriters();
+
+    // Adds a transaction to SharedWriters. Invokes SetShared on the transaction
+    // and either assigns the transaction to validating_trans_ or inserts it in
+    // waiting_for_validation_.
+    bool AddTransaction(Transaction* transaction);
+
+    // Removes a transaction from waiting_for_validation_ and invokes
+    // ResetShared on it.
+    bool RemoveWaitingTransaction(Transaction* transaction);
+
+    // Invokes Read on network_trans_, assigns this transaction to
+    // current_reader_ and returns what network_trans_->Read() returns.
+    // In case a read is already in progress then this transaction is instead
+    // added to waiting_writers_, read_in_progress is set to true and
+    // ERR_IO_PENDING is returned.
+    int Read(IOBuffer* buf,

[jkarlin, 2016/12/06 18:08:18]

It's against style to have an output argument (the IOBuffer) before input
arguments. But.. I guess it should stay since that's how the other Read()
methods are.

[shivanisha, 2016/12/06 21:53:35]

Good to know. Will leave intact in this case, though, for consistency.

+             int buf_len,
+             const CompletionCallback& callback,
+             Transaction* transaction,
+             bool& read_in_progress);

[jkarlin, 2016/12/06 18:08:17]

If it's an output variable, prefer to pass as bool*

[shivanisha, 2016/12/06 21:53:35]

done. 

+
+    // Callback to be invoked when the current_writer_ fails to write data to
+    // the cache. It transfers the ownership of the network transaction since
+    // shared writing can no longer continue, but current_writer_ can continue
+    // to read the remaning data from the network. Any waiting_writers_ will be
+    // notified of the failure. Any idle transactions will be set to a state so
+    // that they can fail any subsequent Read calls. If a validating_trans_
+    // exists, it can continue with its network transaction. Any transactions in
+    // waiting_for_validation_ will move to pending_queue.
+    HttpTransaction* OnCacheWriteFailure(Transaction* transaction);
+
+    // Callback to be invoked when the current_writer_ fails to read from the
+    // network. Both this transaction and any waiting_writers_ will return the
+    // failure code to their consumers. Any idle transactions will be set to a
+    // state so that they can fail any subsequent Read calls. If a
+    // validating_trans_ exists, it can continue with its network transaction.
+    // Any transactions in waiting_for_validation_ will move to pending_queue.
+    void OnNetworkReadFailure(const Transaction* trans, int result);
+
+    // Callback to be invoked when validating_trans_ wants to continue with the
+    // entry as the validation successfully matched it.
+    void OnValidationMatch(Transaction* transaction, RequestPriority priority);
+
+    // Callback to be invoked when validating_trans_ cannot continue with the
+    // entry as the validation did not return a 304.
+    // Its ok to continue writing to cache if this is the only transaction in
+    // SharedWriters, so network_trans ownership is transferred to
+    // SharedWriters. But if there are other transactions, doom the entry and
+    // this transaction will continue reading from the network so the return
+    // value will pass the ownership of the network transaction back to the
+    // transaction.
+    std::unique_ptr<HttpTransaction> OnValidationNoMatch(
+        Transaction* transactioan,
+        std::unique_ptr<HttpTransaction> network_trans,
+        RequestPriority priority);
+
+    // Callback to be invoked when current_writer_ successfully read from the
+    // network.
+    void OnNetworkReadSuccess(const Transaction* trans,
+                              scoped_refptr<IOBuffer> buf,
+                              int len);
+
+    // Callback to be invoked when current_writer_ successfully wrote to the
+    // cache.
+    void OnCacheWriteSuccess(const Transaction* trans, int result);
+
+    // Moves all transactions eligible for shared writing from entry's
+    // pending_queue to waiting_for_validation_.
+    void MoveFromPendingQueue();
+
+    // Posts a task for invoking the io callback of the first transaction in
+    // waiting_for_validation_.
+    void ProcessFirstWaitingValidation();
+
+    // Returns true if this object is empty: no transactions in all_writers and
+    // waiting_for_validation_ and validating_trans_.
+    bool empty();
+
+    // Removes a transaction which is in all_writers_ but not currently waiting
+    // on Read.
+    void RemoveIdleWriter(Transaction* trans);
+
+    // Callback to be invoked when the current_writer's consumer destroys it.
+    // Its ownership will then be transferred to SharedWriters till the read
+    // data is written to the cache.
+    void DoomCurrentWriter(std::unique_ptr<HttpTransaction> trans);
+
+    // Destroys doomed transaction. If this is part of an event that needs entry
+    // to be destroyed, destroy_entry will be set to true.
+    void FinalizeDoomedWriter(bool& out_destroy_entry);
+
+    // Removes a waiting_writer_ transaction.
+    void RemoveWaitingWriter(Transaction* trans);
+
+    // Removes the currently validating transaction.
+    void RemoveValidatingTransaction(Transaction* trans);
+
+    // Invoked when DoneReading is invoked on a shared transaction. If
+    // current_writer_ is set, do nothing, else consider it a read completion
+    // and process accordingly.
+    void DoneReading(Transaction* trans);
+
+    // Invoked when StopCaching is called for a shared writer transaction.
+    // It stops caching only if there are no other transactions in
+    // all_writers_.
+    std::unique_ptr<HttpTransaction> StopCaching(Transaction* transaction);
+
+    // Getter functions.
+    int GetTotalReceivedBytes() const;
+    int GetTotalSentBytes() const;
+    LoadState GetLoadState() const;
+    bool GetFullRequestHeaders(HttpRequestHeaders* headers) const;
+    bool GetLoadTimingInfo(LoadTimingInfo* load_timing_info) const;
+    bool GetRemoteEndpoint(IPEndPoint* endpoint) const;
+    void PopulateNetErrorDetails(NetErrorDetails* details) const;
+    void GetConnectionAttempts(ConnectionAttempts* out) const;
+
+    // Setter functions.
+    void SetPriority(RequestPriority priority);
+    void SetWebSocketHandshakeStreamCreateHelper(
+        WebSocketHandshakeStreamBase::CreateHelper* create_helper);
+
+    int ResumeNetworkStart();
+
+   private:
+    // Moves all transactions in waiting_for_validation_ to entry's
+    // pending_queue.
+    void MoveToPendingQueue();
+    // Notifies the waiting_writers_ of the result, by posting a task for each

[jkarlin, 2016/12/06 18:08:17]

There should be a blank line above comments to aid readability.

[shivanisha, 2016/12/06 21:53:34]

Done.

+    // of them. While processing the task for a transaction, it's IO callback
+    // will be invoked.
+    void ProcessWaitingWriters(int result);
+
+    void OnProcessFirstWaitingValidation();
+
+    // Removes the current_writer_ transaction from this object.
+    void ResetAndRemoveCurrentWriter(bool continue_network_reading = false);
+
+    // Resets the current_writer_ to null and deleted doomed_writer_ if it
+    // exists.
+    void ResetCurrentWriter();
+
+    // Sets the state of idle writers so that they can fail any subsequent
+    // Read.
+    void SetIdleWritersFailState();
+
+    void MoveIdleWritersToReaders();
+    void ValidationDoneContinue(Transaction* transaction,
+                                RequestPriority priority);
+
+    // Http Cache.
+    base::WeakPtr<HttpCache> cache_;
+    // The Active entry.
+    ActiveEntry* entry_;

[jkarlin, 2016/12/06 18:08:17]

Document that the ActiveEntry* owns this.

[shivanisha, 2016/12/06 21:53:35]

Done

+    // Network transaction created by the first HttpCache::Transaction that led
+    // to the creation of this SharedWriters object and later used by various
+    // other HttpCache::Transactions.
+    std::unique_ptr<HttpTransaction> network_trans_;
+    // The cache transaction that has currently invoked
+    // HttpNetworkTransaction::read() and is waiting for read to be completed.
+    // This is used to ensure there is at most one call to
+    // HttpNetworkTransaction::read(). After the read is successful and data
+    // written to cache, other waiting writers will be notified.
+    Transaction* current_writer_;
+    // This will point to the currently validating transaction.
+    // After the validation stage is successfully done, this transaction is
+    // added to all_writers_.
+    Transaction* validating_trans_;
+    // If a transaction is currently set in validating_trans_, these
+    // transactions will wait for that to complete and then the first of these
+    // will be assigned to validating_trans_.
+    TransactionList waiting_for_validation_;
+    // These transactions are waiting on their Read calls when current_writer_
+    // is not null. After current_writer_ completes writing the data to the
+    // cache, their buffer would be filled with the data and their callback
+    // would be invoked.
+    struct WaitingWriter {
+      Transaction* transaction;
+      scoped_refptr<IOBuffer> read_buf;
+      int read_buf_len;
+      int write_len;
+      WaitingWriter(Transaction* transaction,
+                    scoped_refptr<IOBuffer> read_buf,
+                    int len);
+      ~WaitingWriter();
+      WaitingWriter(const WaitingWriter&);
+    };
+    typedef std::list<WaitingWriter> WaitingWritersList;
+    WaitingWritersList waiting_writers_;
+    // Includes a transaction that is:
+    // 1. Currently reading or,
+    // 2. Currently waiting for the data being read by another transaction or,
+    // 3. Waiting for Read to be invoked from the consumer.
+    TransactionSet all_writers_;
+    // If the current_network_reader_'s consumer deletes it, we do not delete it
+    // but pass its ownership to SharedWriters for completing the async
+    // operation in progress. After the data is written in the cache, the
+    // transaction will be deleted.
+    std::unique_ptr<HttpTransaction> doomed_writer_;
+    // Current priority of the request. If a higher priority transaction is
+    // added to waiting_network_readers_, the priority will be increased.
+    // todo (shivanisha@): If the higher priority request dies, we do not change
+    // the priority back to a lower one. Fix this. The priority should be the
+    // highest of the existing requests.
+    RequestPriority priority_;
+    // Used to keep information that ehtry should be destroyed later in the

[jkarlin, 2016/12/06 18:08:18]

s/ehtry/entry/

[shivanisha, 2016/12/06 21:53:35]

done

+    // flow. iIt cannot be destroyed at this moment because doomed_writer_ needs

[jkarlin, 2016/12/06 18:08:18]

s/iIt/It/

[shivanisha, 2016/12/06 21:53:35]

done

+    // to be destroyed first.
+    bool destroy_entry_;
+    base::WeakPtrFactory<SharedWriters> weak_factory_;
+  };
+
   class MetadataWriter;
   class QuicServerInfoFactoryAdaptor;
-  class Transaction;
   class WorkItem;
-  friend class Transaction;
   friend class ViewCacheHelper;
   struct PendingOp;  // Info for an entry under construction.
 
-  typedef std::list<Transaction*> TransactionList;
   typedef std::list<std::unique_ptr<WorkItem>> WorkItemList;
 
   struct ActiveEntry {
@@ -246,8 +488,9 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
 
     disk_cache::Entry* disk_entry;
     Transaction*       writer;
-    TransactionList    readers;
+    TransactionSet readers;

[jkarlin, 2016/12/06 18:08:18]

Is it okay to lose the ordering by changing from list to set?

[shivanisha, 2016/12/06 21:53:35]

Yes, since readers are only waiting for read to be invoked on them. There is no
role they play that requires ordering.

     TransactionList    pending_queue;
+    std::unique_ptr<SharedWriters> shared_writers;
     bool               will_process_pending_queue;
     bool               doomed;
   };
@@ -302,7 +545,8 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
   // cache entry.
   ActiveEntry* ActivateEntry(disk_cache::Entry* disk_entry);
 
-  // Deletes an ActiveEntry.
+  // Deletes an ActiveEntry. Expects all of the transactions to be reset to
+  // null, like writer, readers, shared_writers, pending_queue.

[jkarlin, 2016/12/06 18:08:17]

s/null/nullptr/

[shivanisha, 2016/12/06 21:53:35]

done

   void DeactivateEntry(ActiveEntry* entry);
 
   // Deletes an ActiveEntry using an exhaustive search.
@@ -391,6 +635,51 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
 
   // Processes the backend creation notification.
   void OnBackendCreated(int result, PendingOp* pending_op);
+  void DestroyEntryAndRestartPendingQueueTxns(ActiveEntry* entry);

[jkarlin, 2016/12/06 18:08:17]

s/Txns/Transactions/

[shivanisha, 2016/12/06 21:53:35]

done

+
+  // Creates a SharedWriters object.
+  void CreateSharedWriters(Transaction* cacheTrans,
+                           std::unique_ptr<HttpTransaction> network_trans,
+                           RequestPriority priority);
+
+  // Resets if entry contains an empty SharedWriters object and calls
+  // ProcessPendingQueue.
+  void ResetSharedWritersProcessPendingQueue(ActiveEntry* entry);
+
+  // Invokes the io callback of the transaction with the result.
+  void NotifyTransaction(Transaction* transaction, int result);
+
+  // Callbacks invoked by a transaction part of SharedWriters, for various
+  // events.
+  HttpTransaction* CacheWriteFailedSharedWriters(Transaction* trans,
+                                                 ActiveEntry* entry);
+  void NetworkReadFailedSharedWriters(Transaction* trans,
+                                      ActiveEntry* entry,
+                                      int result);
+  void CacheWriteSuccessSharedWriters(Transaction* trans,
+                                      ActiveEntry* entry,
+                                      int result);
+  void DoneReadingSharedWriters(Transaction* trans, ActiveEntry* entry);
+  void DoomCurrentSharedWriter(std::unique_ptr<HttpTransaction> trans,
+                               ActiveEntry* entry);
+  void FinalizeDoomedSharedWriter(ActiveEntry* entry);
+  void RemoveIdleSharedWriter(Transaction* trans,
+                              ActiveEntry* entry,
+                              bool success);
+  void RemoveValidatingTransSharedWriters(Transaction* trans,
+                                          ActiveEntry* entry);
+  void ValidationMatchSharedWriters(Transaction* transaction,
+                                    RequestPriority priority,
+                                    ActiveEntry* entry);
+  std::unique_ptr<HttpTransaction> ValidationNoMatchSharedWriters(
+      const std::string& key,
+      Transaction* transaction,
+      std::unique_ptr<HttpTransaction> network_trans,
+      RequestPriority priority,
+      ActiveEntry* entry);
+  std::unique_ptr<HttpTransaction> StopCachingSharedWriters(
+      Transaction* transaction,
+      ActiveEntry* entry);
 
   // Variables ----------------------------------------------------------------