Reland "[code caching] Handle small data with a single read/write"

This is a reland of d99ef40d

Patchset #8 modifies the original to handle Write and Fetch completions in
any order, which should fix the UAF crashes.

diff: https://chromium-review.googlesource.com/c/chromium/src/+/1846336/1..8

Original change's description:
> [code caching] Handle small data with a single read/write
>
> - Many small reads and writes are observed, especially with the JavaScript
>   code cache. Detect these and write all data into the entry's stream 0,
>   and clear stream 1. When reading, detect the entry size and for small
>   data, skip the stream 1 read and copy the data from the stream 0 read.
> - Changes the stream 0 data (again) to a header with response time and
>   data size. The data size, while not strictly necessary right now, will
>   be needed if we implement de-duplication of identical entries stored by
>   multiple origins. We can add the code hash to this header, and the size
>   field will disambiguate between small entries and large de-duplicated
>   entries where we use the hash as a key to the data.
> - This change should make small reads faster than before, as synchronous
>   completion of stream 0 reads is observed. Otherwise, there should be no
>   change in performance.
> - Renames the buffers and completion callbacks to reflect small/large data
>   distinction.
>
> Bug: chromium:992991
> Change-Id: I6fb5337ef1e4148dd9f300f0a8c85acb401be62e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1834562
> Commit-Queue: Bill Budge <bbudge@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Reviewed-by: Mythri Alle <mythria@chromium.org>
> Reviewed-by: Maks Orlovich <morlovich@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#702667}

Bug: chromium:992991,chromium:1011879
Change-Id: I9d589c45f567d93841536bb327381223be489928
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1846336Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Mythri Alle <mythria@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704616}

content/browser/code_cache/generated_code_cache.h

@@ -122,7 +122,7 @@ class CONTENT_EXPORT GeneratedCodeCache {
   enum Operation { kFetch, kWrite, kDelete, kGetBackend };
 
   // Data streams corresponding to each entry.
-  enum { kResponseTimeStream = 0, kDataStream = 1 };
+  enum { kSmallDataStream = 0, kLargeDataStream = 1 };
 
   // Creates a simple_disk_cache backend.
   void CreateBackend();
@@ -138,15 +138,17 @@ class CONTENT_EXPORT GeneratedCodeCache {
   void WriteEntryImpl(PendingOperation* op);
   void OpenCompleteForWrite(PendingOperation* op,
                             disk_cache::EntryResult result);
-  void WriteResponseTimeComplete(PendingOperation* op, int rv);
-  void WriteDataComplete(PendingOperation* op, int rv);
+  void WriteSmallBufferComplete(PendingOperation* op, int rv);
+  void WriteLargeBufferComplete(PendingOperation* op, int rv);
+  void WriteComplete(PendingOperation* op);
 
   // Fetches entry from cache.
   void FetchEntryImpl(PendingOperation* op);
   void OpenCompleteForRead(PendingOperation* op,
                            disk_cache::EntryResult result);
-  void ReadResponseTimeComplete(PendingOperation* op, int rv);
-  void ReadDataComplete(PendingOperation* op, int rv);
+  void ReadSmallBufferComplete(PendingOperation* op, int rv);
+  void ReadLargeBufferComplete(PendingOperation* op, int rv);
+  void ReadComplete(PendingOperation* op);
 
   // Deletes entry from cache.
   void DeleteEntryImpl(PendingOperation* op);

content/browser/code_cache/generated_code_cache_unittest.cc

@@ -17,6 +17,7 @@ namespace content {
 
 class GeneratedCodeCacheTest : public testing::Test {
  public:
+  static const int kLargeSizeInBytes = 8192;
   static const int kMaxSizeInBytes = 1024 * 1024;
   static constexpr char kInitialUrl[] = "http://example.com/script.js";
   static constexpr char kInitialOrigin[] = "http://example.com";
@@ -41,6 +42,10 @@ class GeneratedCodeCacheTest : public testing::Test {
     generated_code_cache_ = std::make_unique<GeneratedCodeCache>(
         cache_path_, kMaxSizeInBytes, cache_type);
 
+    GeneratedCodeCache::GetBackendCallback callback = base::BindOnce(
+        &GeneratedCodeCacheTest::GetBackendCallback, base::Unretained(this));
+    generated_code_cache_->GetBackend(std::move(callback));
+
     GURL url(kInitialUrl);
     GURL origin_lock = GURL(kInitialOrigin);
     WriteToCache(url, origin_lock, kInitialData, base::Time::Now());
@@ -76,6 +81,16 @@ class GeneratedCodeCacheTest : public testing::Test {
     generated_code_cache_->FetchEntry(url, origin_lock, callback);
   }
 
+  void DoomAll() {
+    net::CompletionOnceCallback callback = base::BindOnce(
+        &GeneratedCodeCacheTest::DoomAllCallback, base::Unretained(this));
+    backend_->DoomAllEntries(std::move(callback));
+  }
+
+  void GetBackendCallback(disk_cache::Backend* backend) { backend_ = backend; }
+
+  void DoomAllCallback(int rv) {}
+
   void FetchEntryCallback(const base::Time& response_time,
                           mojo_base::BigBuffer data) {
     if (data.size() == 0) {
@@ -100,6 +115,7 @@ class GeneratedCodeCacheTest : public testing::Test {
   bool received_;
   bool received_null_;
   base::FilePath cache_path_;
+  disk_cache::Backend* backend_;
 };
 
 constexpr char GeneratedCodeCacheTest::kInitialUrl[];
@@ -153,6 +169,23 @@ TEST_F(GeneratedCodeCacheTest, WriteEntry) {
   EXPECT_EQ(response_time, received_response_time_);
 }
 
+TEST_F(GeneratedCodeCacheTest, WriteLargeEntry) {
+  GURL new_url("http://example1.com/script.js");
+  GURL origin_lock = GURL(kInitialOrigin);
+
+  InitializeCache(GeneratedCodeCache::CodeCacheType::kJavaScript);
+  std::string large_data(kLargeSizeInBytes, 'x');
+  base::Time response_time = base::Time::Now();
+  WriteToCache(new_url, origin_lock, large_data, response_time);
+  task_environment_.RunUntilIdle();
+  FetchFromCache(new_url, origin_lock);
+  task_environment_.RunUntilIdle();
+
+  ASSERT_TRUE(received_);
+  EXPECT_EQ(large_data, received_data_);
+  EXPECT_EQ(response_time, received_response_time_);
+}
+
 TEST_F(GeneratedCodeCacheTest, DeleteEntry) {
   GURL url(kInitialUrl);
   GURL origin_lock = GURL(kInitialOrigin);
@@ -200,6 +233,28 @@ TEST_F(GeneratedCodeCacheTest, WriteEntryFailure) {
   EXPECT_EQ(base::Time(), received_response_time_);
 }
 
+TEST_F(GeneratedCodeCacheTest, WriteEntryFailureOutOfOrder) {
+  GURL url(kInitialUrl);
+  GURL origin_lock = GURL(kInitialOrigin);
+
+  InitializeCache(GeneratedCodeCache::CodeCacheType::kJavaScript);
+  // Dooming adds pending activity for all entries. This makes the following
+  // write block for stream 0, while the stream 1 write fails synchronously. The
+  // two callbacks are received in reverse order.
+  DoomAll();
+  base::Time response_time = base::Time::Now();
+  std::string too_big_data(kMaxSizeInBytes * 8, 0);
+  WriteToCache(url, origin_lock, too_big_data, response_time);
+  task_environment_.RunUntilIdle();
+  FetchFromCache(url, origin_lock);
+  task_environment_.RunUntilIdle();
+
+  // Fetch should return empty data, with invalid response time.
+  ASSERT_TRUE(received_);
+  ASSERT_TRUE(received_null_);
+  EXPECT_EQ(base::Time(), received_response_time_);
+}
+
 TEST_F(GeneratedCodeCacheTest, FetchEntryPendingOp) {
   GURL url(kInitialUrl);
   GURL origin_lock = GURL(kInitialOrigin);
@@ -229,6 +284,23 @@ TEST_F(GeneratedCodeCacheTest, WriteEntryPendingOp) {
   EXPECT_EQ(response_time, received_response_time_);
 }
 
+TEST_F(GeneratedCodeCacheTest, WriteLargeEntryPendingOp) {
+  GURL new_url("http://example1.com/script1.js");
+  GURL origin_lock = GURL(kInitialOrigin);
+
+  InitializeCache(GeneratedCodeCache::CodeCacheType::kJavaScript);
+  std::string large_data(kLargeSizeInBytes, 'x');
+  base::Time response_time = base::Time::Now();
+  WriteToCache(new_url, origin_lock, large_data, response_time);
+  task_environment_.RunUntilIdle();
+  FetchFromCache(new_url, origin_lock);
+  task_environment_.RunUntilIdle();
+
+  ASSERT_TRUE(received_);
+  EXPECT_EQ(large_data, received_data_);
+  EXPECT_EQ(response_time, received_response_time_);
+}
+
 TEST_F(GeneratedCodeCacheTest, DeleteEntryPendingOp) {
   GURL url(kInitialUrl);
   GURL origin_lock = GURL(kInitialOrigin);
@@ -259,6 +331,63 @@ TEST_F(GeneratedCodeCacheTest, UpdateDataOfExistingEntry) {
   EXPECT_EQ(response_time, received_response_time_);
 }
 
+TEST_F(GeneratedCodeCacheTest, UpdateDataOfSmallExistingEntry) {
+  GURL url(kInitialUrl);
+  GURL origin_lock = GURL(kInitialOrigin);
+
+  InitializeCache(GeneratedCodeCache::CodeCacheType::kJavaScript);
+  std::string new_data(kLargeSizeInBytes, 'x');
+  base::Time response_time = base::Time::Now();
+  WriteToCache(url, origin_lock, new_data, response_time);
+  task_environment_.RunUntilIdle();
+  FetchFromCache(url, origin_lock);
+  task_environment_.RunUntilIdle();
+
+  ASSERT_TRUE(received_);
+  EXPECT_EQ(new_data, received_data_);
+  EXPECT_EQ(response_time, received_response_time_);
+}
+
+TEST_F(GeneratedCodeCacheTest, UpdateDataOfLargeExistingEntry) {
+  GURL url(kInitialUrl);
+  GURL origin_lock = GURL(kInitialOrigin);
+
+  InitializeCache(GeneratedCodeCache::CodeCacheType::kJavaScript);
+  std::string large_data(kLargeSizeInBytes, 'x');
+  base::Time response_time = base::Time::Now();
+  WriteToCache(url, origin_lock, large_data, response_time);
+  std::string new_data = large_data + "Overwrite";
+  response_time = base::Time::Now();
+  WriteToCache(url, origin_lock, new_data, response_time);
+  task_environment_.RunUntilIdle();
+  FetchFromCache(url, origin_lock);
+  task_environment_.RunUntilIdle();
+
+  ASSERT_TRUE(received_);
+  EXPECT_EQ(new_data, received_data_);
+  EXPECT_EQ(response_time, received_response_time_);
+}
+
+TEST_F(GeneratedCodeCacheTest, TruncateDataOfLargeExistingEntry) {
+  GURL url(kInitialUrl);
+  GURL origin_lock = GURL(kInitialOrigin);
+
+  InitializeCache(GeneratedCodeCache::CodeCacheType::kJavaScript);
+  std::string large_data(kLargeSizeInBytes, 'x');
+  base::Time response_time = base::Time::Now();
+  WriteToCache(url, origin_lock, large_data, response_time);
+  std::string new_data = "SerializedCodeForScriptOverwrite";
+  response_time = base::Time::Now();
+  WriteToCache(url, origin_lock, new_data, response_time);
+  task_environment_.RunUntilIdle();
+  FetchFromCache(url, origin_lock);
+  task_environment_.RunUntilIdle();
+
+  ASSERT_TRUE(received_);
+  EXPECT_EQ(new_data, received_data_);
+  EXPECT_EQ(response_time, received_response_time_);
+}
+
 TEST_F(GeneratedCodeCacheTest, FetchFailsForNonexistingOrigin) {
   InitializeCache(GeneratedCodeCache::CodeCacheType::kJavaScript);
   GURL new_origin_lock = GURL("http://not-example.com");