[Remoting Android] JniGlDisplayHandler calls invalidate() on UI thread

remoting/client/jni/jni_client.h

Index: remoting/client/jni/jni_client.h
diff --git a/remoting/client/jni/jni_client.h b/remoting/client/jni/jni_client.h
index 75e5c539789a39800e57b09515af619375c2e118..d0696c6c867ba61eb94b9d3438588ecfe821c017 100644
--- a/remoting/client/jni/jni_client.h
+++ b/remoting/client/jni/jni_client.h
@@ -10,6 +10,7 @@
 #include "base/android/scoped_java_ref.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "remoting/client/jni/display_updater_factory.h"
 #include "remoting/protocol/connection_to_host.h"
 #include "remoting/protocol/cursor_shape_stub.h"
 
@@ -17,7 +18,6 @@ namespace remoting {
 
 class ChromotingJniRuntime;
 class ChromotingJniInstance;
-class DisplayUpdaterFactory;
 class JniPairingSecretFetcher;
 
 // Houses resources scoped to a session and exposes JNI interface to the
@@ -157,7 +157,8 @@ class JniClient {
   // Reference to the Java client object.
   base::android::ScopedJavaGlobalRef<jobject> java_client_;
 
-  std::unique_ptr<DisplayUpdaterFactory> display_handler_;
+  std::unique_ptr<DisplayUpdaterFactory, DisplayUpdaterFactory::Deleter>

[Sergey Ulanov, 2016/09/19 21:14:56]

I'd prefer to avoid non-default deleters with unique_ptr<>. I haven't seem them
used anywhere else, style guide doesn't explicitly allow them and it seems like
they can be rather error prone. Usually when an object needs to be deleted on a
different thread we wrap it in a thread-proxy wrapper which uses DeleteSoon()
internally.

[Yuwei, 2016/09/19 22:30:37]

I think the issue here is not just about deleting the object on a different
thread. The issue is the Java counterpart must be invalidated on the UI thread
before the object itself is deleted on the display thread. My Destroy() approach
tries to hide the detail of destroying the object on two threads.

Since JniGlDisplayHandler already has an init function, maybe we can just follow
the construct->initiate->invalidate->destruct pattern rather than
construct->initiate->destroy.

[Yuwei, 2016/09/19 22:38:32]

Alternatively we can separate stuffs on the display thread into a core class and
DeleteSoon the core in dtor. I don't have a strong opinion. It will require more
code changes though.

+      display_handler_;
 
   // Deleted on UI thread.
   std::unique_ptr<JniPairingSecretFetcher> secret_fetcher_;