Tidy up preload related states in blink::Resource

This CL cleans up preload related states in Resource as following:

 - Removes preload_count_ and introduces is_unused_preload_. With this
   change, a resource can be a preload for only one request, and hence
   tied to one ResourceFetcher at a time.
 - Renames IncreasePreloadCount and DecreasePreloadCount to
   MarkAsPreload and MatchPreload respectively.
 - Removed preload_result_.
 - Changes the timing of kPreloadNotReferenced => kPreloadReferenced
   (i.e., true => false for is_unused_preload_) state transition from
   when a ResourceClient is added to when MatchPreload is called.
 - Removes PreloadReferencePolicy.

Bug: 652228
Change-Id: I6564c0eed24f19051ee49c429cf87db746414023
Reviewed-on: https://chromium-review.googlesource.com/544363
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yoav Weiss <yoav@yoav.ws>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486730}

third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp

@@ -266,7 +266,7 @@ CSSPreloaderResourceClient::CSSPreloaderResourceClient(
                   : kScanOnly),
       preloader_(preloader),
       resource_(ToCSSStyleSheetResource(resource)) {
-  resource_->AddClient(this, Resource::kDontMarkAsReferenced);
+  resource_->AddClient(this);
 }
 
 CSSPreloaderResourceClient::~CSSPreloaderResourceClient() {}

third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp

@@ -92,7 +92,6 @@ TEST_F(CSSPreloadScannerTest, ScanFromResourceClient) {
   const char* data = "@import url('http://127.0.0.1/preload.css');";
   resource->AppendData(data, strlen(data));
 
-  EXPECT_EQ(Resource::kPreloadNotReferenced, resource->GetPreloadResult());
   EXPECT_EQ(1u, resource_client->preload_urls_.size());
   EXPECT_EQ("http://127.0.0.1/preload.css",
             resource_client->preload_urls_.front());
@@ -212,7 +211,6 @@ TEST_F(CSSPreloadScannerTest, ReferrerPolicyHeader) {
   const char* data = "@import url('http://127.0.0.1/preload.css');";
   resource->AppendData(data, strlen(data));
 
-  EXPECT_EQ(Resource::kPreloadNotReferenced, resource->GetPreloadResult());
   EXPECT_EQ(1u, resource_client->preload_urls_.size());
   EXPECT_EQ("http://127.0.0.1/preload.css",
             resource_client->preload_urls_.front());

third_party/WebKit/Source/core/loader/LinkLoader.cpp

@@ -82,11 +82,9 @@ class LinkLoader::FinishObserver final
   USING_PRE_FINALIZER(FinishObserver, ClearResource);
 
  public:
-  FinishObserver(LinkLoader* loader,
-                 Resource* resource,
-                 Resource::PreloadReferencePolicy reference_policy)
+  FinishObserver(LinkLoader* loader, Resource* resource)
       : loader_(loader), resource_(resource) {
-    resource_->AddFinishObserver(this, reference_policy);
+    resource_->AddFinishObserver(this);
   }
 
   // ResourceFinishObserver implementation
@@ -484,10 +482,8 @@ bool LinkLoader::LoadLink(
     resource = PrefetchIfNeeded(document, href, rel_attribute, cross_origin,
                                 referrer_policy);
   }
-  if (resource) {
-    finish_observer_ =
-        new FinishObserver(this, resource, Resource::kDontMarkAsReferenced);
-  }
+  if (resource)
+    finish_observer_ = new FinishObserver(this, resource);
 
   if (const unsigned prerender_rel_types =
           PrerenderRelTypesFromRelAttribute(rel_attribute, document)) {

third_party/WebKit/Source/core/loader/resource/ImageResource.cpp

@@ -120,7 +120,7 @@ class ImageResource::ImageResourceInfoImpl final
 
   void SetDecodedSize(size_t size) override { resource_->SetDecodedSize(size); }
   void WillAddClientOrObserver() override {
-    resource_->WillAddClientOrObserver(Resource::kMarkAsReferenced);
+    resource_->WillAddClientOrObserver();
   }
   void DidRemoveClientOrObserver() override {
     resource_->DidRemoveClientOrObserver();
@@ -280,7 +280,7 @@ void ImageResource::DestroyDecodedDataForFailedRevalidation() {
 
 void ImageResource::DestroyDecodedDataIfPossible() {
   GetContent()->DestroyDecodedData();
-  if (GetContent()->HasImage() && !IsPreloaded() &&
+  if (GetContent()->HasImage() && !IsUnusedPreload() &&
       GetContent()->IsRefetchableDataFromDiskCache()) {
     UMA_HISTOGRAM_MEMORY_KB("Memory.Renderer.EstimatedDroppableEncodedSize",
                             EncodedSize() / 1024);

third_party/WebKit/Source/platform/loader/fetch/Resource.cpp

@@ -262,12 +262,10 @@ void Resource::ServiceWorkerResponseCachedMetadataHandler::SendToPlatform() {
 Resource::Resource(const ResourceRequest& request,
                    Type type,
                    const ResourceLoaderOptions& options)
-    : preload_result_(kPreloadNotReferenced),
-      type_(type),
+    : type_(type),
       status_(ResourceStatus::kNotStarted),
       load_finish_time_(0),
       identifier_(0),
-      preload_count_(0),
       preload_discovery_time_(0.0),
       encoded_size_(0),
       encoded_size_memory_usage_(0),
@@ -402,7 +400,7 @@ void Resource::FinishAsError(const ResourceError& error) {
   error_ = error;
   is_revalidating_ = false;
 
-  if ((error_.IsCancellation() || !IsPreloaded()) && IsMainThread())
+  if ((error_.IsCancellation() || !is_unused_preload_) && IsMainThread())
     GetMemoryCache()->Remove(this);
 
   if (!ErrorOccurred())
@@ -526,6 +524,7 @@ const ResourceRequest& Resource::LastResourceRequest() const {
 
 void Resource::SetRevalidatingRequest(const ResourceRequest& request) {
   SECURITY_CHECK(redirect_chain_.IsEmpty());
+  SECURITY_CHECK(!is_unused_preload_);
   DCHECK(!request.IsNull());
   CHECK(!is_revalidation_start_forbidden_);
   is_revalidating_ = true;
@@ -605,13 +604,6 @@ String Resource::ReasonNotDeletable() const {
       builder.Append(' ');
     builder.Append("loader_");
   }
-  if (preload_count_) {
-    if (!builder.IsEmpty())
-      builder.Append(' ');
-    builder.Append("preload_count_(");
-    builder.AppendNumber(preload_count_);
-    builder.Append(')');
-  }
   if (IsMainThread() && GetMemoryCache()->Contains(this)) {
     if (!builder.IsEmpty())
       builder.Append(' ');
@@ -648,28 +640,16 @@ static bool TypeNeedsSynchronousCacheHit(Resource::Type type) {
   return false;
 }
 
-void Resource::WillAddClientOrObserver(PreloadReferencePolicy policy) {
-  if (policy == kMarkAsReferenced && preload_result_ == kPreloadNotReferenced) {
-    preload_result_ = kPreloadReferenced;
-
-    if (preload_discovery_time_) {
-      int time_since_discovery = static_cast<int>(
-          1000 * (MonotonicallyIncreasingTime() - preload_discovery_time_));
-      DEFINE_STATIC_LOCAL(CustomCountHistogram, preload_discovery_histogram,
-                          ("PreloadScanner.ReferenceTime", 0, 10000, 50));
-      preload_discovery_histogram.Count(time_since_discovery);
-    }
-  }
+void Resource::WillAddClientOrObserver() {
   if (!HasClientsOrObservers()) {
     is_alive_ = true;
   }
 }
 
-void Resource::AddClient(ResourceClient* client,
-                         PreloadReferencePolicy policy) {
+void Resource::AddClient(ResourceClient* client) {
   CHECK(!is_add_remove_client_prohibited_);
 
-  WillAddClientOrObserver(policy);
+  WillAddClientOrObserver();
 
   if (is_revalidating_) {
     clients_.insert(client);
@@ -721,12 +701,11 @@ void Resource::RemoveClient(ResourceClient* client) {
   DidRemoveClientOrObserver();
 }
 
-void Resource::AddFinishObserver(ResourceFinishObserver* client,
-                                 PreloadReferencePolicy policy) {
+void Resource::AddFinishObserver(ResourceFinishObserver* client) {
   CHECK(!is_add_remove_client_prohibited_);
   DCHECK(!finish_observers_.Contains(client));
 
-  WillAddClientOrObserver(policy);
+  WillAddClientOrObserver();
   finish_observers_.insert(client);
   if (IsLoaded())
     TriggerNotificationForFinishObservers();
@@ -1025,6 +1004,24 @@ void Resource::RevalidationFailed() {
   is_revalidating_ = false;
 }
 
+void Resource::MarkAsPreload() {
+  DCHECK(!is_unused_preload_);
+  is_unused_preload_ = true;
+}
+
+void Resource::MatchPreload() {
+  DCHECK(is_unused_preload_);
+  is_unused_preload_ = false;
+
+  if (preload_discovery_time_) {
+    int time_since_discovery = static_cast<int>(
+        1000 * (MonotonicallyIncreasingTime() - preload_discovery_time_));
+    DEFINE_STATIC_LOCAL(CustomCountHistogram, preload_discovery_histogram,
+                        ("PreloadScanner.ReferenceTime", 0, 10000, 50));
+    preload_discovery_histogram.Count(time_since_discovery);
+  }
+}
+
 bool Resource::CanReuseRedirectChain() const {
   for (auto& redirect : redirect_chain_) {
     if (!CanUseResponse(redirect.redirect_response_, response_timestamp_))

third_party/WebKit/Source/platform/loader/fetch/Resource.h

@@ -91,13 +91,6 @@ class PLATFORM_EXPORT Resource : public GarbageCollectedFinalized<Resource>,
   };
   static const int kLastResourceType = kMock + 1;
 
-  // Whether a resource client for a preload should mark the preload as
-  // referenced.
-  enum PreloadReferencePolicy {
-    kMarkAsReferenced,
-    kDontMarkAsReferenced,
-  };
-
   // Used by reloadIfLoFiOrPlaceholderImage().
   enum ReloadLoFiOrPlaceholderPolicy {
     kReloadIfNeeded,
@@ -153,21 +146,13 @@ class PLATFORM_EXPORT Resource : public GarbageCollectedFinalized<Resource>,
     return ResourcePriority();
   }
 
-  // The reference policy indicates that the client should not affect whether
-  // a preload is considered referenced or not. This allows for "passive"
-  // resource clients that simply observe the resource.
-  void AddClient(ResourceClient*, PreloadReferencePolicy = kMarkAsReferenced);
+  void AddClient(ResourceClient*);
   void RemoveClient(ResourceClient*);
 
-  void AddFinishObserver(ResourceFinishObserver*,
-                         PreloadReferencePolicy = kMarkAsReferenced);
+  void AddFinishObserver(ResourceFinishObserver*);
   void RemoveFinishObserver(ResourceFinishObserver*);
 
-  enum PreloadResult : uint8_t {
-    kPreloadNotReferenced,
-    kPreloadReferenced,
-  };
-  PreloadResult GetPreloadResult() const { return preload_result_; }
+  bool IsUnusedPreload() const { return is_unused_preload_; }
 
   ResourceStatus GetStatus() const { return status_; }
   void SetStatus(ResourceStatus status) { status_ = status; }
@@ -247,18 +232,8 @@ class PLATFORM_EXPORT Resource : public GarbageCollectedFinalized<Resource>,
   }
   void SetDataBufferingPolicy(DataBufferingPolicy);
 
-  // The IsPreloaded() flag is using a counter in order to make sure that even
-  // when multiple ResourceFetchers are preloading the resource, it will remain
-  // marked as preloaded until *all* of them have used it.
-  bool IsUnusedPreload() const {
-    return IsPreloaded() && GetPreloadResult() == kPreloadNotReferenced;
-  }
-  bool IsPreloaded() const { return preload_count_; }
-  void IncreasePreloadCount() { ++preload_count_; }
-  void DecreasePreloadCount() {
-    DCHECK(preload_count_);
-    --preload_count_;
-  }
+  void MarkAsPreload();
+  void MatchPreload();
 
   bool CanReuseRedirectChain() const;
   bool MustRevalidateDueToCacheHeaders() const;
@@ -378,7 +353,7 @@ class PLATFORM_EXPORT Resource : public GarbageCollectedFinalized<Resource>,
   void FinishPendingClients();
 
   virtual void DidAddClient(ResourceClient*);
-  void WillAddClientOrObserver(PreloadReferencePolicy);
+  void WillAddClientOrObserver();
 
   void DidRemoveClientOrObserver();
   virtual void AllClientsAndObserversRemoved();
@@ -446,7 +421,6 @@ class PLATFORM_EXPORT Resource : public GarbageCollectedFinalized<Resource>,
   // MemoryCoordinatorClient overrides:
   void OnPurgeMemory() override;
 
-  PreloadResult preload_result_;
   Type type_;
   ResourceStatus status_;
   CORSStatus cors_status_;
@@ -460,7 +434,6 @@ class PLATFORM_EXPORT Resource : public GarbageCollectedFinalized<Resource>,
 
   unsigned long identifier_;
 
-  unsigned preload_count_;
   double preload_discovery_time_;
 
   size_t encoded_size_;
@@ -481,6 +454,7 @@ class PLATFORM_EXPORT Resource : public GarbageCollectedFinalized<Resource>,
   bool is_alive_;
   bool is_add_remove_client_prohibited_;
   bool is_revalidation_start_forbidden_ = false;
+  bool is_unused_preload_ = false;
 
   ResourceIntegrityDisposition integrity_disposition_;
   IntegrityMetadataSet integrity_metadata_;

third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp

@@ -516,10 +516,8 @@ void ResourceFetcher::RemovePreload(Resource* resource) {
   auto it = preloads_.find(PreloadKey(resource->Url(), resource->GetType()));
   if (it == preloads_.end())
     return;
-  if (it->value == resource) {
-    resource->DecreasePreloadCount();
+  if (it->value == resource)
     preloads_.erase(it);
-  }
 }
 
 ResourceFetcher::PrepareRequestResult ResourceFetcher::PrepareRequest(
@@ -908,7 +906,7 @@ Resource* ResourceFetcher::MatchPreload(const FetchParameters& params,
   if (!IsReusableAlsoForPreloading(params, resource, false))
     return nullptr;
 
-  resource->DecreasePreloadCount();
+  resource->MatchPreload();
   preloads_.erase(it);
   matched_preloads_.push_back(resource);
   return resource;
@@ -927,7 +925,7 @@ void ResourceFetcher::InsertAsPreloadIfNecessary(Resource* resource,
   PreloadKey key(params.Url(), type);
   if (preloads_.find(key) == preloads_.end()) {
     preloads_.insert(key, resource);
-    resource->IncreasePreloadCount();
+    resource->MarkAsPreload();
     if (preloaded_urls_for_test_)
       preloaded_urls_for_test_->insert(resource->Url().GetString());
   }
@@ -997,6 +995,14 @@ ResourceFetcher::DetermineRevalidationPolicy(
     return kReload;
   }
 
+  // It's hard to share a not-yet-referenced preloads via MemoryCache correctly.
+  // A not-yet-matched preloads made by a foreign ResourceFetcher and stored in
+  // the memory cache could be used without this block.
+  if ((fetch_params.IsLinkPreload() || fetch_params.IsSpeculativePreload()) &&
+      existing_resource->IsUnusedPreload()) {
+    return kReload;
+  }
+
   // Checks if the resource has an explicit policy about integrity metadata.
   //
   // This is necessary because ScriptResource and CSSStyleSheetResource objects
@@ -1120,6 +1126,11 @@ ResourceFetcher::DetermineRevalidationPolicy(
   if (request.GetCachePolicy() == WebCachePolicy::kValidatingCacheData ||
       existing_resource->MustRevalidateDueToCacheHeaders() ||
       request.CacheControlContainsNoCache()) {
+    // Revalidation is harmful for non-matched preloads because it may lead to
+    // sharing one preloaded resource among multiple ResourceFetchers.
+    if (existing_resource->IsUnusedPreload())
+      return kReload;
+
     // See if the resource has usable ETag or Last-modified headers. If the page
     // is controlled by the ServiceWorker, we choose the Reload policy because
     // the revalidation headers should not be exposed to the
@@ -1217,9 +1228,7 @@ void ResourceFetcher::ClearPreloads(ClearPreloadsPolicy policy) {
   for (const auto& pair : preloads_) {
     Resource* resource = pair.value;
     if (policy == kClearAllPreloads || !resource->IsLinkPreload()) {
-      resource->DecreasePreloadCount();
-      if (resource->GetPreloadResult() == Resource::kPreloadNotReferenced)
-        GetMemoryCache()->Remove(resource);
+      GetMemoryCache()->Remove(resource);
       keys_to_be_removed.push_back(pair.key);
     }
   }
@@ -1231,8 +1240,7 @@ void ResourceFetcher::ClearPreloads(ClearPreloadsPolicy policy) {
 void ResourceFetcher::WarnUnusedPreloads() {
   for (const auto& pair : preloads_) {
     Resource* resource = pair.value;
-    if (resource && resource->IsLinkPreload() &&
-        resource->GetPreloadResult() == Resource::kPreloadNotReferenced) {
+    if (resource && resource->IsLinkPreload() && resource->IsUnusedPreload()) {
       Context().AddConsoleMessage(
           "The resource " + resource->Url().GetString() +
               " was preloaded using link preload but not used within a few "
@@ -1525,8 +1533,7 @@ void ResourceFetcher::LogPreloadStats(ClearPreloadsPolicy policy) {
         policy == kClearSpeculativeMarkupPreloads) {
       continue;
     }
-    int miss_count =
-        resource->GetPreloadResult() == Resource::kPreloadNotReferenced ? 1 : 0;
+    int miss_count = resource->IsUnusedPreload() ? 1 : 0;
     switch (resource->GetType()) {
       case Resource::kImage:
         images++;

third_party/WebKit/Source/platform/loader/fetch/ResourceFetcherTest.cpp

@@ -500,7 +500,7 @@ TEST_F(ResourceFetcherTest, PreloadResourceTwice) {
   fetcher->ClearPreloads(ResourceFetcher::kClearAllPreloads);
   EXPECT_FALSE(fetcher->ContainsAsPreload(resource));
   EXPECT_FALSE(GetMemoryCache()->Contains(resource));
-  EXPECT_FALSE(resource->IsPreloaded());
+  EXPECT_TRUE(resource->IsUnusedPreload());
 }
 
 TEST_F(ResourceFetcherTest, LinkPreloadResourceAndUse) {
@@ -534,7 +534,7 @@ TEST_F(ResourceFetcherTest, LinkPreloadResourceAndUse) {
   // DCL reached
   fetcher->ClearPreloads(ResourceFetcher::kClearSpeculativeMarkupPreloads);
   EXPECT_TRUE(GetMemoryCache()->Contains(resource));
-  EXPECT_FALSE(resource->IsPreloaded());
+  EXPECT_FALSE(resource->IsUnusedPreload());
 }
 
 TEST_F(ResourceFetcherTest, PreloadMatchWithBypassingCache) {
@@ -593,21 +593,21 @@ TEST_F(ResourceFetcherTest, RepetitiveLinkPreloadShouldBeMerged) {
 
   Resource* resource1 = MockResource::Fetch(fetch_params_for_preload, fetcher);
   ASSERT_TRUE(resource1);
-  EXPECT_TRUE(resource1->IsPreloaded());
+  EXPECT_TRUE(resource1->IsUnusedPreload());
   EXPECT_TRUE(fetcher->ContainsAsPreload(resource1));
   Platform::Current()->GetURLLoaderMockFactory()->ServeAsynchronousRequests();
 
   // The second preload fetch returns the first preload.
   Resource* resource2 = MockResource::Fetch(fetch_params_for_preload, fetcher);
   EXPECT_TRUE(fetcher->ContainsAsPreload(resource1));
-  EXPECT_TRUE(resource1->IsPreloaded());
+  EXPECT_TRUE(resource1->IsUnusedPreload());
   EXPECT_EQ(resource1, resource2);
 
   // preload matching
   Resource* resource3 = MockResource::Fetch(fetch_params_for_request, fetcher);
   EXPECT_EQ(resource1, resource3);
   EXPECT_FALSE(fetcher->ContainsAsPreload(resource1));
-  EXPECT_FALSE(resource1->IsPreloaded());
+  EXPECT_FALSE(resource1->IsUnusedPreload());
 }
 
 TEST_F(ResourceFetcherTest, RepetitiveSpeculativePreloadShouldBeMerged) {
@@ -623,21 +623,21 @@ TEST_F(ResourceFetcherTest, RepetitiveSpeculativePreloadShouldBeMerged) {
 
   Resource* resource1 = MockResource::Fetch(fetch_params_for_preload, fetcher);
   ASSERT_TRUE(resource1);
-  EXPECT_TRUE(resource1->IsPreloaded());
+  EXPECT_TRUE(resource1->IsUnusedPreload());
   EXPECT_TRUE(fetcher->ContainsAsPreload(resource1));
   Platform::Current()->GetURLLoaderMockFactory()->ServeAsynchronousRequests();
 
   // The second preload fetch returns the first preload.
   Resource* resource2 = MockResource::Fetch(fetch_params_for_preload, fetcher);
   EXPECT_TRUE(fetcher->ContainsAsPreload(resource1));
-  EXPECT_TRUE(resource1->IsPreloaded());
+  EXPECT_TRUE(resource1->IsUnusedPreload());
   EXPECT_EQ(resource1, resource2);
 
   // preload matching
   Resource* resource3 = MockResource::Fetch(fetch_params_for_request, fetcher);
   EXPECT_EQ(resource1, resource3);
   EXPECT_FALSE(fetcher->ContainsAsPreload(resource1));
-  EXPECT_FALSE(resource1->IsPreloaded());
+  EXPECT_FALSE(resource1->IsUnusedPreload());
 }
 
 TEST_F(ResourceFetcherTest, SpeculativePreloadShouldBePromotedToLinkePreload) {
@@ -657,7 +657,7 @@ TEST_F(ResourceFetcherTest, SpeculativePreloadShouldBePromotedToLinkePreload) {
   Resource* resource1 =
       MockResource::Fetch(fetch_params_for_speculative_preload, fetcher);
   ASSERT_TRUE(resource1);
-  EXPECT_TRUE(resource1->IsPreloaded());
+  EXPECT_TRUE(resource1->IsUnusedPreload());
   EXPECT_FALSE(resource1->IsLinkPreload());
   EXPECT_TRUE(fetcher->ContainsAsPreload(resource1));
   Platform::Current()->GetURLLoaderMockFactory()->ServeAsynchronousRequests();
@@ -666,7 +666,7 @@ TEST_F(ResourceFetcherTest, SpeculativePreloadShouldBePromotedToLinkePreload) {
   Resource* resource2 =
       MockResource::Fetch(fetch_params_for_link_preload, fetcher);
   EXPECT_TRUE(fetcher->ContainsAsPreload(resource1));
-  EXPECT_TRUE(resource1->IsPreloaded());
+  EXPECT_TRUE(resource1->IsUnusedPreload());
   EXPECT_TRUE(resource1->IsLinkPreload());
   EXPECT_EQ(resource1, resource2);
 
@@ -674,7 +674,7 @@ TEST_F(ResourceFetcherTest, SpeculativePreloadShouldBePromotedToLinkePreload) {
   Resource* resource3 = MockResource::Fetch(fetch_params_for_request, fetcher);
   EXPECT_EQ(resource1, resource3);
   EXPECT_FALSE(fetcher->ContainsAsPreload(resource1));
-  EXPECT_FALSE(resource1->IsPreloaded());
+  EXPECT_FALSE(resource1->IsUnusedPreload());
   EXPECT_FALSE(resource1->IsLinkPreload());
 }