Fix a race in WebGL non-blocking readback implementation

Due to a race condition between OnSignalAck and CheckResultsAvailable,
getSyncParameter or clientWaitSync can report that a
WebGLSync has passed before the readback results are actually available
to getBufferSubData. This results in getBufferSubData falling back to
the slower path sometimes.

To remedy this, this change switches from using SignalQuery to a custom
callback mechanism managed by the individual QueryTracker Query object.
This callback is called immediately when the Query discovers that it
has completed.

Bug: 873424
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I2136e3ea4d55dc4b5c2392d24791b787500a30af
Reviewed-on: https://chromium-review.googlesource.com/1171908Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582478}

gpu/command_buffer/client/gles2_implementation.cc

@@ -5688,13 +5688,12 @@ void GLES2Implementation::EndQueryEXT(GLenum target) {
   if (target == GL_READBACK_SHADOW_COPIES_UPDATED_CHROMIUM) {
     DCHECK(capabilities_.chromium_nonblocking_readback);
     DCHECK(query);
-    GLuint query_id = query->id();
     auto serial = readback_buffer_shadow_tracker_->buffer_shadow_serial();
     readback_buffer_shadow_tracker_->IncrementSerial();
     auto buffers = readback_buffer_shadow_tracker_->TakeUnfencedBufferList();
-    SignalQuery(query_id, base::BindOnce(
-                              &GLES2Implementation::BufferShadowWrittenCallback,
-                              std::move(buffers), serial));
+    query->SetCompletedCallback(
+        base::BindOnce(&GLES2Implementation::BufferShadowWrittenCallback,
+                       std::move(buffers), serial));
   }
 }
 

gpu/command_buffer/client/query_tracker.cc

@@ -149,6 +149,8 @@ QueryTracker::Query::Query(GLuint id,
       client_begin_time_us_(0),
       result_(0) {}
 
+QueryTracker::Query::~Query() {}
+
 void QueryTracker::Query::Begin(QueryTrackerClient* client) {
   // init memory, inc count
   MarkAsActive();
@@ -232,6 +234,9 @@ bool QueryTracker::Query::CheckResultsAvailable(
           result_ = info_.sync->result;
           break;
       }
+      if (on_completed_callback_) {
+        std::move(on_completed_callback_.value()).Run();
+      }
       state_ = kComplete;
     } else {
       if ((helper->flush_generation() - flush_count_ - 1) >= 0x80000000) {
@@ -250,6 +255,12 @@ uint64_t QueryTracker::Query::GetResult() const {
   return result_;
 }
 
+void QueryTracker::Query::SetCompletedCallback(base::OnceClosure callback) {
+  DCHECK(!on_completed_callback_);
+  DCHECK(state_ == kPending);
+  on_completed_callback_ = std::move(callback);
+}
+
 QueryTracker::QueryTracker(MappedMemoryManager* manager)
     : query_sync_manager_(manager),
       mapped_memory_(manager),

gpu/command_buffer/client/query_tracker.h

@@ -121,6 +121,7 @@ class GLES2_IMPL_EXPORT QueryTracker {
     };
 
     Query(GLuint id, GLenum target, const QuerySyncManager::QueryInfo& info);
+    ~Query();
 
     GLenum target() const {
       return target_;
@@ -173,6 +174,8 @@ class GLES2_IMPL_EXPORT QueryTracker {
 
     uint64_t GetResult() const;
 
+    void SetCompletedCallback(base::OnceClosure callback);
+
    private:
     friend class QueryTracker;
     friend class QueryTrackerTest;
@@ -189,6 +192,8 @@ class GLES2_IMPL_EXPORT QueryTracker {
     uint32_t flush_count_;
     uint64_t client_begin_time_us_;  // Only used for latency query target.
     uint64_t result_;
+
+    base::Optional<base::OnceClosure> on_completed_callback_;
   };
 
   explicit QueryTracker(MappedMemoryManager* manager);