drm_hwcomposer: Add check to vsync routine to avoid crash on callback

Vsync could be disabled during routine being running and this could
potentially lead to crash on callback invocation. Crash happens if
VSyncControl(false) was called when Routine has cached callback and
unlocked mutex but haven't callback yet. At this point we can't be
sure that callback is still valid so invoking it is incorrect
behaviour.

Second check if vsync is enabled drastically shortens window when we
could go into invalid state, from the whole vblank invocation to
several machine instructions between check and invocation.

Please note that we can't check against cached value in this case,
therefore operations on this flag should be atomic instead.

Signed-off-by: Roman Kovalivskyi <roman.kovalivskyi@globallogic.com>

2 files changed, 21 insertions, 5 deletions

diff --git a/drm/vsyncworker.cpp b/drm/vsyncworker.cpp
index d022887..08ab301 100644
--- a/drm/vsyncworker.cpp
+++ b/drm/vsyncworker.cpp
@@ -126,14 +126,10 @@ void VSyncWorker::Routine() {
     }
   }
 
-  bool enabled = enabled_;
   int display = display_;
   std::shared_ptr<VsyncCallback> callback(callback_);
   Unlock();
 
-  if (!enabled)
-    return;
-
   DrmCrtc *crtc = drm_->GetCrtcForDisplay(display);
   if (!crtc) {
     ALOGE("Failed to get crtc for display");
@@ -161,6 +157,26 @@ void VSyncWorker::Routine() {
   }
 
   /*
+   * VSync could be disabled during routine execution so it could potentially
+   * lead to crash since callback's inner hook could be invalid anymore. We have
+   * no control over lifetime of this hook, therefore we can't rely that it'll
+   * be valid after vsync disabling.
+   *
+   * Blocking VSyncControl to wait until routine
+   * will finish execution is logically correct way to fix this issue, but it
+   * creates visible lags and stutters, so we have to resort to other ways of
+   * mitigating this issue.
+   *
+   * Doing check before attempt to invoke callback drastically shortens the
+   * window when such situation could happen and that allows us to practically
+   * avoid this issue.
+   *
+   * Please note that issue described below is different one and it is related
+   * to RegisterCallback, not to disabling vsync via VSyncControl.
+   */
+  if (!enabled_)
+    return;
+  /*
    * There's a race here where a change in callback_ will not take effect until
    * the next subsequent requested vsync. This is unavoidable since we can't
    * call the vsync hook while holding the thread lock.
diff --git a/include/vsyncworker.h b/include/vsyncworker.h
index b2bca9d..96f7432 100644
--- a/include/vsyncworker.h
+++ b/include/vsyncworker.h
@@ -60,7 +60,7 @@ class VSyncWorker : public Worker {
   std::shared_ptr<VsyncCallback> callback_ = NULL;
 
   int display_;
-  bool enabled_;
+  std::atomic_bool enabled_;
   int64_t last_timestamp_;
 };
 }  // namespace android