Added proper DCHECKs to ensure proper resource provider synchronization.

Proper DCHECKs have been added to make sure resource provider users
add all the proper synchronization calls. All the unittests have also
been fixed up to properly synchronize and insert sync tokens when
necessary.

BUG=584381
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1717873002

Cr-Commit-Position: refs/heads/master@{#378533}

3 files changed, 91 insertions, 9 deletions

diff --git a/cc/resources/resource_provider.cc b/cc/resources/resource_provider.cc
index 53cd750..d4a7087 100644
--- a/cc/resources/resource_provider.cc
+++ b/cc/resources/resource_provider.cc
@@ -1040,7 +1040,6 @@ ResourceProvider::ScopedWriteLockSoftware::ScopedWriteLockSoftware(
 
 ResourceProvider::ScopedWriteLockSoftware::~ScopedWriteLockSoftware() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  resource_->SetSynchronized();
   resource_provider_->UnlockForWrite(resource_);
 }
 
@@ -1068,6 +1067,7 @@ ResourceProvider::ScopedWriteLockGpuMemoryBuffer::
   resource_provider_->LazyCreateImage(resource_);
   resource_->dirty_image = true;
   resource_->is_overlay_candidate = true;
+  resource_->SetSynchronized();
 
   // GpuMemoryBuffer provides direct access to the memory used by the GPU.
   // Read lock fences are required to ensure that we're not trying to map a
@@ -1327,7 +1327,20 @@ void ResourceProvider::PrepareSendToParent(const ResourceIdArray& resource_ids,
   std::vector<Resource*> resources;
   resources.reserve(resource_ids.size());
   for (const ResourceId id : resource_ids) {
-    resources.push_back(GetResource(id));
+    Resource* resource = GetResource(id);
+    // Check the synchronization and sync token state when delegated sync points
+    // are required. The only case where we allow a sync token to not be set is
+    // the case where the image is dirty. In that case we will bind the image
+    // lazily and generate a sync token at that point.
+    DCHECK(!output_surface_->capabilities().delegated_sync_points_required ||
+           resource->dirty_image || !resource->needs_sync_token());
+
+    // If we are validating the resource to be sent, the resource cannot be
+    // in a LOCALLY_USED state. It must have been properly synchronized.
+    DCHECK(!output_surface_->capabilities().delegated_sync_points_required ||
+           Resource::LOCALLY_USED != resource->synchronization_state());
+
+    resources.push_back(resource);
   }
 
   // Lazily create any mailboxes and verify all unverified sync tokens.
@@ -1362,6 +1375,7 @@ void ResourceProvider::PrepareSendToParent(const ResourceIdArray& resource_ids,
   }
   for (Resource* resource : need_synchronization_resources) {
     resource->UpdateSyncToken(new_sync_token);
+    resource->SetSynchronized();
   }
 
   // Transfer Resources
diff --git a/cc/resources/resource_provider.h b/cc/resources/resource_provider.h
index 1e1ab1c..399ac1d 100644
--- a/cc/resources/resource_provider.h
+++ b/cc/resources/resource_provider.h
@@ -483,19 +483,20 @@ class CC_EXPORT ResourceProvider
       // a sync token arrives from an external resource (such as a child or
       // parent), it is automatically initialized as NEEDS_WAIT as well
       // since we still need to wait on it before the resource is synchronized
-      // on the current context.
+      // on the current context. It is an error to use the resource locally for
+      // reading or writing if the resource is in this state.
       NEEDS_WAIT,
 
       // The SYNCHRONIZED state indicates that the resource has been properly
       // synchronized locally. This can either synchronized externally (such
       // as the case of software rasterized bitmaps), or synchronized
       // internally using a sync token that has been waited upon. In the
-      // former case which was synchronized externally, a corresponding sync
-      // token will not exist. In the latter case which was synchronized from
-      // the NEEDS_WAIT state, a corresponding sync token will exist which
-      // is assocaited with the resource. This sync token is still valid and
-      // still associated with the resource and can be passed as an external
-      // resource for others to wait on.
+      // former case where the resource was synchronized externally, a
+      // corresponding sync token will not exist. In the latter case which was
+      // synchronized from the NEEDS_WAIT state, a corresponding sync token will
+      // exist which is assocaited with the resource. This sync token is still
+      // valid and still associated with the resource and can be passed as an
+      // external resource for others to wait on.
       SYNCHRONIZED,
     };
 
diff --git a/cc/resources/resource_provider_unittest.cc b/cc/resources/resource_provider_unittest.cc
index e971734..d3eb264 100644
--- a/cc/resources/resource_provider_unittest.cc
+++ b/cc/resources/resource_provider_unittest.cc
@@ -672,6 +672,10 @@ TEST_P(ResourceProviderTest, TransferGLResources) {
     resource_ids_to_transfer.push_back(id2);
     resource_ids_to_transfer.push_back(id3);
     resource_ids_to_transfer.push_back(id4);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                   &list);
@@ -932,6 +936,8 @@ TEST_P(ResourceProviderTestNoSyncToken, TransferGLResources) {
     resource_ids_to_transfer.push_back(id2);
     resource_ids_to_transfer.push_back(id3);
     TransferableResourceArray list;
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
     child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                   &list);
     ASSERT_EQ(3u, list.size());
@@ -1005,6 +1011,10 @@ TEST_P(ResourceProviderTest, ReadLockCountStopsReturnToChildOrDelete) {
     // Transfer some resources to the parent.
     ResourceProvider::ResourceIdArray resource_ids_to_transfer;
     resource_ids_to_transfer.push_back(id1);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                   &list);
@@ -1070,6 +1080,10 @@ TEST_P(ResourceProviderTest, ReadLockFenceStopsReturnToChildOrDelete) {
   // Transfer some resources to the parent.
   ResourceProvider::ResourceIdArray resource_ids_to_transfer;
   resource_ids_to_transfer.push_back(id1);
+
+  child_resource_provider_->GenerateSyncTokenForResources(
+      resource_ids_to_transfer);
+
   TransferableResourceArray list;
   child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                 &list);
@@ -1129,6 +1143,10 @@ TEST_P(ResourceProviderTest, ReadLockFenceDestroyChild) {
   ResourceProvider::ResourceIdArray resource_ids_to_transfer;
   resource_ids_to_transfer.push_back(id1);
   resource_ids_to_transfer.push_back(id2);
+
+  child_resource_provider_->GenerateSyncTokenForResources(
+      resource_ids_to_transfer);
+
   TransferableResourceArray list;
   child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                 &list);
@@ -1191,6 +1209,10 @@ TEST_P(ResourceProviderTest, ReadLockFenceContextLost) {
   ResourceProvider::ResourceIdArray resource_ids_to_transfer;
   resource_ids_to_transfer.push_back(id1);
   resource_ids_to_transfer.push_back(id2);
+
+  child_resource_provider_->GenerateSyncTokenForResources(
+      resource_ids_to_transfer);
+
   TransferableResourceArray list;
   child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                 &list);
@@ -1258,6 +1280,10 @@ TEST_P(ResourceProviderTest, TransferSoftwareResources) {
     resource_ids_to_transfer.push_back(id1);
     resource_ids_to_transfer.push_back(id2);
     resource_ids_to_transfer.push_back(id3);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                   &list);
@@ -1305,6 +1331,10 @@ TEST_P(ResourceProviderTest, TransferSoftwareResources) {
     ResourceProvider::ResourceIdArray resource_ids_to_transfer;
     resource_ids_to_transfer.push_back(id1);
     resource_ids_to_transfer.push_back(id2);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                   &list);
@@ -1371,6 +1401,10 @@ TEST_P(ResourceProviderTest, TransferSoftwareResources) {
     resource_ids_to_transfer.push_back(id1);
     resource_ids_to_transfer.push_back(id2);
     resource_ids_to_transfer.push_back(id3);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                   &list);
@@ -1439,6 +1473,7 @@ TEST_P(ResourceProviderTest, TransferGLToSoftware) {
       size, ResourceProvider::TEXTURE_HINT_IMMUTABLE, format);
   uint8_t data1[4] = { 1, 2, 3, 4 };
   child_resource_provider->CopyToResource(id1, data1, size);
+  child_resource_provider->GenerateSyncTokenForResource(id1);
 
   ReturnedResourceArray returned_to_child;
   int child_id =
@@ -1446,6 +1481,7 @@ TEST_P(ResourceProviderTest, TransferGLToSoftware) {
   {
     ResourceProvider::ResourceIdArray resource_ids_to_transfer;
     resource_ids_to_transfer.push_back(id1);
+
     TransferableResourceArray list;
     child_resource_provider->PrepareSendToParent(resource_ids_to_transfer,
                                                  &list);
@@ -1492,6 +1528,10 @@ TEST_P(ResourceProviderTest, TransferInvalidSoftware) {
   {
     ResourceProvider::ResourceIdArray resource_ids_to_transfer;
     resource_ids_to_transfer.push_back(id1);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                   &list);
@@ -1546,6 +1586,10 @@ TEST_P(ResourceProviderTest, DeleteExportedResources) {
     ResourceProvider::ResourceIdArray resource_ids_to_transfer;
     resource_ids_to_transfer.push_back(id1);
     resource_ids_to_transfer.push_back(id2);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                   &list);
@@ -1579,6 +1623,10 @@ TEST_P(ResourceProviderTest, DeleteExportedResources) {
     ResourceProvider::ResourceIdArray resource_ids_to_transfer;
     resource_ids_to_transfer.push_back(mapped_id1);
     resource_ids_to_transfer.push_back(mapped_id2);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     resource_provider_->PrepareSendToParent(resource_ids_to_transfer, &list);
 
@@ -1642,6 +1690,10 @@ TEST_P(ResourceProviderTest, DestroyChildWithExportedResources) {
     ResourceProvider::ResourceIdArray resource_ids_to_transfer;
     resource_ids_to_transfer.push_back(id1);
     resource_ids_to_transfer.push_back(id2);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                   &list);
@@ -1675,6 +1727,10 @@ TEST_P(ResourceProviderTest, DestroyChildWithExportedResources) {
     ResourceProvider::ResourceIdArray resource_ids_to_transfer;
     resource_ids_to_transfer.push_back(mapped_id1);
     resource_ids_to_transfer.push_back(mapped_id2);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     resource_provider_->PrepareSendToParent(resource_ids_to_transfer, &list);
 
@@ -1749,6 +1805,10 @@ TEST_P(ResourceProviderTest, DeleteTransferredResources) {
     // Transfer some resource to the parent.
     ResourceProvider::ResourceIdArray resource_ids_to_transfer;
     resource_ids_to_transfer.push_back(id);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                   &list);
@@ -1802,6 +1862,10 @@ TEST_P(ResourceProviderTest, UnuseTransferredResources) {
     // Transfer some resource to the parent.
     ResourceProvider::ResourceIdArray resource_ids_to_transfer;
     resource_ids_to_transfer.push_back(id);
+
+    child_resource_provider_->GenerateSyncTokenForResources(
+        resource_ids_to_transfer);
+
     TransferableResourceArray list;
     child_resource_provider_->PrepareSendToParent(resource_ids_to_transfer,
                                                   &list);
@@ -1987,6 +2051,9 @@ class ResourceProviderTestTextureFilters : public ResourceProviderTest {
 
       EXPECT_CALL(*child_context,
                   produceTextureDirectCHROMIUM(_, GL_TEXTURE_2D, _));
+
+      child_resource_provider->GenerateSyncTokenForResources(
+          resource_ids_to_transfer);
       child_resource_provider->PrepareSendToParent(resource_ids_to_transfer,
                                                    &list);
       Mock::VerifyAndClearExpectations(child_context);