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

remoting/client/jni/jni_client.h

 // Copyright 2015 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.
 
 #ifndef REMOTING_CLIENT_JNI_JNI_CLIENT_H_
 #define REMOTING_CLIENT_JNI_JNI_CLIENT_H_
 
 #include <jni.h>
 
 #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"
 
 namespace remoting {
 
 class ChromotingJniRuntime;
 class ChromotingJniInstance;
-class DisplayUpdaterFactory;
 class JniPairingSecretFetcher;
 
 // Houses resources scoped to a session and exposes JNI interface to the
 // Java client during a session. All its methods should be invoked exclusively
 // from the UI thread unless otherwise noted.
 class JniClient {
  public:
   JniClient(ChromotingJniRuntime* runtime,
             base::android::ScopedJavaGlobalRef<jobject> java_client);
   virtual ~JniClient();
@@ skipping 119 matching lines @@
   // TODO(yuweih): Instead of exposing a weak pointer in the public API,
   //               consider handling task posting inside the client.
   base::WeakPtr<JniClient> GetWeakPtr();
 
  private:
   ChromotingJniRuntime* runtime_;
 
   // 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_;
 
   // Deleted on Network thread.
   std::unique_ptr<ChromotingJniInstance> session_;
 
   // Holds pointer for the UI thread.
   base::WeakPtr<JniClient> weak_ptr_;
   base::WeakPtrFactory<JniClient> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(JniClient);
 };
 
 }  // namespace remoting
 
 #endif  // REMOTING_CLIENT_JNI_JNI_CLIENT_H_