Check if there is error on waiting for GL Fence.

ui/gl/gl_fence_arb.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gl/gl_fence_arb.h"
 
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_context.h"
 
 namespace gfx {
@@ skipping 10 matching lines @@
 
 bool GLFenceARB::HasCompleted() {
   // Handle the case where FenceSync failed.
   if (!sync_)
     return true;
 
   DCHECK_EQ(GL_TRUE, glIsSync(sync_));
   // We could potentially use glGetSynciv here, but it doesn't work
   // on OSX 10.7 (always says the fence is not signaled yet).
   // glClientWaitSync works better, so let's use that instead.
-  return glClientWaitSync(sync_, 0, 0) != GL_TIMEOUT_EXPIRED;
+  GLenum result = glClientWaitSync(sync_, 0, 0);
+  if (result == GL_WAIT_FAILED) {
+    NOTREACHED();

[no sievers, 2014/11/03 19:05:02]

How about LOG(FATAL) instead (and removing NOTREACHED)?

I'm not sure if it's better to return |true| or |false| from this function if
there was an error. Above we return |true| if the fence creation failed, but the
current code does not signal the fence as complete if there was an error. So
maybe LOG(FATAL) would help so that we could find out about drivers that have
issues and then blacklist the extension since it will break functionality either
way (we either skip synchronization or never signal the fence).

[dshwang, 2014/11/04 13:55:08]

Yes, I used FATAL, but I'm afraid of it can crash release build.

[no sievers, 2014/11/04 19:12:56]

That's good in a way, because we will get crash reports from which we will be
able to form a blacklist entry (based on vendor and driver version or so).
Better to not use the extension if it doesn't work.

+    LOG(ERROR) << "Fail to wait for legal GLFence. error code:" << glGetError();

[no sievers, 2014/11/03 19:05:03]

You have to loop over it until it stops returning errors, since glGetError() is
not guaranteed to return the accumulated errors in any specific order. I.e.

LOG(ERROR) << "Failed to wait for GLFence. Clearing errors:";
GLenum error;
while ((error = glGetError()) != GL_NO_ERROR) {
  LOG(ERROR) << "GLES2Util::GetStringEnum(error)";
}

[dshwang, 2014/11/04 13:55:08]

Most existing code don't do loop-checking. I think this code should clean up its
own error only, not leakage of others. If it doesn't clean up all accumulated
errors, it's not mistake of this. It's mistake of code leaking the error.

In addition, I could not use GetStringEnum because it's defined in gpu/

[no sievers, 2014/11/04 19:12:56]

The problem is that you cannot clear the previous error only. glGetError()
returns all error flags set "in unspecified order".

So you'd have to clear all errors before making a GL call here, if you only
wanted to call glGetError() once. But that seems worse.

+  }
+  return result != GL_TIMEOUT_EXPIRED;

[no sievers, 2014/11/03 19:05:02]

If you return explicitly above, you can DCHECK_NE(result, GL_TIMEOUT_EXPIRED)
here, since we don't set a timeout.

[dshwang, 2014/11/04 13:55:08]

even if we don't set a timeout, GL_TIMEOUT_EXPIRED can be fired, if the fence is
not signaled.

 }
 
 void GLFenceARB::ClientWait() {
   DCHECK_EQ(GL_TRUE, glIsSync(sync_));
   if (!flush_event_.get() || flush_event_->IsSignaled()) {
     glClientWaitSync(sync_, GL_SYNC_FLUSH_COMMANDS_BIT, GL_TIMEOUT_IGNORED);

[dshwang, 2014/11/04 13:55:08]

Added DCHECK_NE(result, GL_TIMEOUT_EXPIRED) here

   } else {
     LOG(ERROR) << "Trying to wait for uncommitted fence. Skipping...";
   }
 }
 
 void GLFenceARB::ServerWait() {
   DCHECK_EQ(GL_TRUE, glIsSync(sync_));
   if (!flush_event_.get() || flush_event_->IsSignaled()) {
     glWaitSync(sync_, 0, GL_TIMEOUT_IGNORED);
   } else {
     LOG(ERROR) << "Trying to wait for uncommitted fence. Skipping...";
   }
 }
 
 GLFenceARB::~GLFenceARB() {
   DCHECK_EQ(GL_TRUE, glIsSync(sync_));
   glDeleteSync(sync_);
 }
 
 }  // namespace gfx