Fixes the cache lock issue.

net/http/http_cache_transaction.h

Index: net/http/http_cache_transaction.h
diff --git a/net/http/http_cache_transaction.h b/net/http/http_cache_transaction.h
index 32bb23571f48a0a868288b492a1f55dacf48f9cd..a08b9d7850701f9ae89b29d4d691796ce7b6e822 100644
--- a/net/http/http_cache_transaction.h
+++ b/net/http/http_cache_transaction.h
@@ -107,7 +107,7 @@ class HttpCache::Transaction : public HttpTransaction {
   // deleting the active entry.
   bool AddTruncatedFlag();
 
-  HttpCache::ActiveEntry* entry() { return entry_; }
+  HttpCache::ActiveEntry* entry() const { return entry_; }
 
   // Returns the LoadState of the writer transaction of a given ActiveEntry. In
   // other words, returns the LoadState of this transaction without asking the
@@ -117,6 +117,8 @@ class HttpCache::Transaction : public HttpTransaction {
 
   const CompletionCallback& io_callback() { return io_callback_; }
 
+  base::WeakPtr<Transaction> GetWeakPtr() { return weak_factory_.GetWeakPtr(); }

[jkarlin, 2017/02/03 18:26:20]

Chrome convention is to call it AsWeakPtr()

[shivanisha, 2017/02/06 21:14:11]

In net/ I see many instances of GetWeakPtr but none for AsWeakPtr, although in
the rest of the chrome code base there are many instances of AsWeakPtr. Keeping
it as GetWeakPtr for consistency within net.

+
   const NetLogWithSource& net_log() const;
 
   // Bypasses the cache lock whenever there is lock contention.
@@ -163,6 +165,37 @@ class HttpCache::Transaction : public HttpTransaction {
       const BeforeHeadersSentCallback& callback) override;
   int ResumeNetworkStart() override;
   void GetConnectionAttempts(ConnectionAttempts* out) const override;
+  RequestPriority priority() const { return priority_; }
+
+  // Returns true if the transaction is eligible for reading from the
+  // network and writing to the cache along with other transactions.
+  // It is eligible if the following conditions are true:
+  // - Request’s mode is READ_WRITE. If mode is only READ then this
+  // transaction should wait until the whole response is in the cache.
+  // - Method is GET.
+  // - It is not a range request.
+  // - It is not a no-store data.
+  bool IsEligibleForSharedWriting() const;
+

[jkarlin, 2017/02/03 18:26:20]

Note that methods in .cc file should be defined in the same order that they're
declared in the header. I see that they don't match here (e.g., I see
ResetShared is defined earlier in the cc than SetShared).

[shivanisha, 2017/02/06 21:14:11]

Re-ordered

+  // Sets shared_ to true.
+  void SetShared();
+
+  // Sets shared_ to false. Arguments tell if the transaction will continue to
+  // read from the network and whether the transaction will continue to read
+  // from the cache.
+  void ResetShared(bool continue_network_reading = false,
+                   bool continue_cache_reading = false);
+
+  bool shared() const { return shared_; }
+
+  // Sets next_state_ such that any subsequent Read or IO callback should fail.
+  void SetSharedWritingFailState(int result);

[jkarlin, 2017/02/03 18:26:20]

It sounds like (from the comment) that this is a setter for next_state_, but it
does more than that.

[shivanisha, 2017/02/06 21:14:11]

Updated the comment.

+
+  // The transaction should now continue without shared writing and take the
+  // ownership of the network transaction object.
+  void ContinueWithoutSharedWriting(
+      std::unique_ptr<HttpTransaction> network_transaction,
+      bool needs_entry);
 
  private:
   static const size_t kNumValidationHeaders = 2;
@@ -215,16 +248,34 @@ class HttpCache::Transaction : public HttpTransaction {
     STATE_PARTIAL_HEADERS_RECEIVED,
     STATE_CACHE_READ_METADATA,
     STATE_CACHE_READ_METADATA_COMPLETE,
+    // Update the value if another state becomes the last state of the start

[jkarlin, 2017/02/03 18:26:20]

Better to put this comment on top of the enum. People are more likely to look
there.

[shivanisha, 2017/02/06 21:14:11]

Don't need this anymore since RemoveTransactionFromSharedWriters is now using a
switch statement.

+    // state machine.
+    STATE_START_STATE_MACHINE_FINAL = STATE_CACHE_READ_METADATA_COMPLETE,
 
+    // Start states should always be before Read states.
     // These states are entered from Read/AddTruncatedFlag.
     STATE_NETWORK_READ,
     STATE_NETWORK_READ_COMPLETE,
-    STATE_CACHE_READ_DATA,
-    STATE_CACHE_READ_DATA_COMPLETE,
     STATE_CACHE_WRITE_DATA,
     STATE_CACHE_WRITE_DATA_COMPLETE,
     STATE_CACHE_WRITE_TRUNCATED_RESPONSE,
-    STATE_CACHE_WRITE_TRUNCATED_RESPONSE_COMPLETE
+    STATE_CACHE_WRITE_TRUNCATED_RESPONSE_COMPLETE,
+
+    // Shared Writing states.
+    STATE_SHARED_NETWORK_READ,
+    STATE_SHARED_NETWORK_READ_COMPLETE,
+    STATE_SHARED_NETWORK_READ_WAIT_COMPLETE,
+    STATE_SHARED_CACHE_WRITE_DATA,
+    STATE_SHARED_CACHE_WRITE_DATA_COMPLETE,
+
+    // Cache read states, also used for shared writing transactions when they
+    // are behind the data already written to the cache.
+    STATE_CACHE_READ_DATA,
+    STATE_CACHE_READ_DATA_COMPLETE,
+
+    // This state is reached for a shared writing transaction when there is a
+    // failure and any subsequent Read or IO callback should fail.
+    STATE_SHARED_READ_WRITE_FAILED
   };
 
   // Used for categorizing validation triggers in histograms.
@@ -291,6 +342,12 @@ class HttpCache::Transaction : public HttpTransaction {
   int DoCacheWriteDataComplete(int result);
   int DoCacheWriteTruncatedResponse();
   int DoCacheWriteTruncatedResponseComplete(int result);
+  int DoSharedNetworkRead();
+  int DoSharedNetworkReadComplete(int result);
+  int DoSharedNetworkReadWaitComplete(int result);
+  int DoSharedCacheWriteData(int num_bytes);
+  int DoSharedCacheWriteDataComplete(int result);
+  int DoSharedReadWriteFailed();
 
   // Sets request_ and fields derived from it.
   void SetRequest(const NetLogWithSource& net_log,
@@ -374,7 +431,10 @@ class HttpCache::Transaction : public HttpTransaction {
   int OnWriteResponseInfoToEntryComplete(int result);
 
   // Called when we are done writing to the cache entry.
-  void DoneWritingToEntry(bool success);
+  // perform_entry_cleanup will be false if we just want to set the internal
+  // state of the transaction but do not want to invoke any cleanup operation on
+  // entry_ e.g. when it's already done by a SharedWriter callback function.
+  void DoneWritingToEntry(bool success, bool perform_entry_cleanup = true);
 
   // Returns an error to signal the caller that the current read failed. The
   // current operation |result| is also logged. If |restart| is true, the
@@ -409,10 +469,15 @@ class HttpCache::Transaction : public HttpTransaction {
   // |old_network_trans_load_timing_|, which must be NULL when this is called.
   void ResetNetworkTransaction();
 
-  // Returns true if we should bother attempting to resume this request if it is
-  // aborted while in progress. If |has_data| is true, the size of the stored
-  // data is considered for the result.
-  bool CanResume(bool has_data);
+  void SaveNetworkTransactionInfo(const HttpTransaction* transaction);
+  void SaveSharedNetworkTransactionInfo();
+
+  // Returns the currently active network transaction.
+  // At any given point, at most one of the following can be non-null:
+  // - network_trans_(network transaction for headers),
+  // - entry_->shared_writers->network_transaction_ (network transaction for
+  // shared writing of response body).
+  HttpTransaction* GetCurrentNetworkTransaction() const;
 
   // Setter for response_ and auth_response_. It updates its cache entry status,
   // if needed.
@@ -426,6 +491,21 @@ class HttpCache::Transaction : public HttpTransaction {
   void SyncCacheEntryStatusToResponse();
   void RecordHistograms();
 
+  // Checks if the transaction should create a SharedWriters object or not. If
+  // yes, creates one. Also checks if the transaction should continue to be part
+  // of an already created SharedWriters object or not based on its validation
+  // result.
+  void ProcessForSharedWriting();
+
+  // Returns true if the transaction is shared and still in the headers phase.
+  bool validating_shared() const;
+
+  // Destructor helper to remove the transaction from SharedWriters.
+  void RemoveTransactionFromSharedWriters();
+
+  // Helper function.
+  void OnCacheWriteDataComplete(bool was_shared, int* result);
+
   // Called to signal completion of asynchronous IO.
   void OnIOComplete(int result);
 
@@ -435,6 +515,7 @@ class HttpCache::Transaction : public HttpTransaction {
   NetLogWithSource net_log_;
   std::unique_ptr<HttpRequestInfo> custom_request_;
   HttpRequestHeaders request_headers_copy_;
+
   // If extra_headers specified a "if-modified-since" or "if-none-match",
   // |external_validation_| contains the value of those headers.
   ValidationHeaders external_validation_;
@@ -460,6 +541,18 @@ class HttpCache::Transaction : public HttpTransaction {
   bool couldnt_conditionalize_request_;
   bool bypass_lock_for_test_;  // A test is exercising the cache lock.
   bool fail_conditionalization_for_test_;  // Fail ConditionalizeRequest.
+
+  // Reads from the network and writes to the cache using a SharedWriters
+  // object.
+  bool shared_;
+
+  // Used for histograms.
+  bool initiate_shared_writing_;
+
+  // Result to be returned to the consumer on Read invocation when state is
+  // STATE_SHARED_READ_WRITE_FAILED.
+  int shared_read_write_failure_result_;
+
   scoped_refptr<IOBuffer> read_buf_;
   int io_buf_len_;
   int read_offset_;
@@ -503,6 +596,9 @@ class HttpCache::Transaction : public HttpTransaction {
   BeforeNetworkStartCallback before_network_start_callback_;
   BeforeHeadersSentCallback before_headers_sent_callback_;
 
+  bool have_full_request_headers_;
+  HttpRequestHeaders full_request_headers_;
+
   base::WeakPtrFactory<Transaction> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(Transaction);