Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18612018:
Restructure chromoting_jni_instance handling of Java strings (Closed)
|
Created:
7 years, 5 months ago by solb Modified:
7 years, 5 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, weitaosu+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRestructure chromoting_jni_instance handling of Java strings
This also adds some comments to explain how/why things are being done the way they are.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211510
Patch Set 1 #Patch Set 2 : Rebase on top of issue 18477010 #
Total comments: 67
Patch Set 3 : Connection/disconnection state machine and moar comments #
Total comments: 22
Patch Set 4 : Ensure connect and disconnect are called on the same thread #
Total comments: 18
Patch Set 5 : Fix comment style and clear members on correct threads #Patch Set 6 : Rebase atop trunk #
Total comments: 20
Patch Set 7 : Documentation and guards against using variables on the wrong thread #
Total comments: 2
Messages
Total messages: 16 (0 generated)
Takes care of Wez's comments, as discussed at https://codereview.chromium.org/18856012/#msg11. (This patchset is constructed to apply on top of that one.)
This should once again be in sync to land, but it's still awaiting review.
https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:28: // The base and networks stacks must be registered with the JNI in order to nit: the JNI -> JNI https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:29: // work on Android. A (required) AtExitManager cleans this up at world's end. nit: why is (required) needed here? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:34: // On Android, the UI thread is managed by the Java side of things, so we nit: by the Java side of things -> by Java https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:60: // TODO(solb) detach all threads from JVM Why the TODO? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:67: const char* host_pubkey) { nit: DCHECK() that you're called on the expected thread. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:81: // All our work must be done on the network thread. ConnectToHost seems to store the strings while on the calling, Java thread, so this comment seems wrong? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:83: // We're a singleton, so Unretained is safe here. Why does being a singleton make it safe? Who deletes the singleton? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:94: chat_.reset(); // This object must outlive client_. nit: Suggest putting a comment before |client_| "|client_| must be torn down before |signalling_|." instead. And please put these sorts of comments before the code they refer to, rather than at the end of the line. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:95: chat_config_.reset(); // TODO(solb) Restructure to reuse between sessions. Either remove this TODO or elaborate on what you intend to re-use. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:107: // All our work must be done on the UI thread. Previous method says network thread, not UI thread. Which is it? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:128: // All our work must be done on the UI thread. Here too https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:147: // We ignore this message, since OnConnectionState() tells us the same thing. nit: Move this into the body of the method https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:148: void ChromotingJNIInstance::OnConnectionReady(bool ready) {} Why do both exist, then? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:150: // We ignore this message because we don't deal with capabilities. You don't really need this comment; it's clear from the method name that we just ignore capabilities. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:153: // We ignore this message because OnConnectionState() reveals the same info. nit: Move this comment into the body of the method https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:168: // We don't support third party authentication, so we return a null pointer. nit: Move this inside the method, and reword e.g. "Return null to indicate that third party authentication is not supported." https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:194: // We're a singleton, so Unretained is safe here. nit: Blank line before this comment. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:41: // loops and task runners) that must outlive any use of the network stack. Why is the ClientUserInterface maintaining message loops needed by network code? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:46: // a DisconnectFromHost() call is followed by a ConnectToHost() one. See style guide re comment style: e.g. Returns the singleton JNI interface. Re-used across connections. Why is this re-used across connections? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:49: void ConnectToHost( Add comments to explain the semantics of these three methods, e.g. what happens if I can this twice w/out disconnect, etc. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:61: // Gets notified if the user needs to enter a PIN, and notifies Java in turn. Suggest: Calls to Java to prompt the user for an authentication PIN. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:90: scoped_ptr<base::AtExitManager> collector_; nit: Add a comment to explain this. Does it really need to be a scoped_ptr<>? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:101: scoped_refptr<FrameConsumerProxy> frames_; frame_consumer_ https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:103: // The below variables are specific to each connection. Why are per-session variables mixed in with global ones? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:108: scoped_ptr<XmppSignalStrategy::XmppServerConfig> chat_config_; chat_config_ -> signalling_config_ https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:109: scoped_ptr<XmppSignalStrategy> chat_; // must outlive client_ chat_ -> signalling_ https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:109: scoped_ptr<XmppSignalStrategy> chat_; // must outlive client_ If this must out-live |client_| then why does it appear after |client_| in the member order - it'll be torn down first if it appears last. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:110: scoped_ptr<NetworkSettings> netset_; network_settings_ https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:111: protocol::SecretFetchedCallback announce_secret_; Why is this called |announce_secret_|?
How does this look? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:28: // The base and networks stacks must be registered with the JNI in order to On 2013/07/11 22:32:08, Wez wrote: > nit: the JNI -> JNI Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:29: // work on Android. A (required) AtExitManager cleans this up at world's end. On 2013/07/11 22:32:08, Wez wrote: > nit: why is (required) needed here? Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:34: // On Android, the UI thread is managed by the Java side of things, so we On 2013/07/11 22:32:08, Wez wrote: > nit: by the Java side of things -> by Java Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:60: // TODO(solb) detach all threads from JVM On 2013/07/11 22:32:08, Wez wrote: > Why the TODO? As I mentioned in a prior CL, it's something I plan to figure out a proper solution to, but I just haven't had a chance yet. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:67: const char* host_pubkey) { On 2013/07/11 22:32:08, Wez wrote: > nit: DCHECK() that you're called on the expected thread. The threading of this method has become less important as the code has developed. I'll DCHECK() on whether we're currently "connected" instead. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:81: // All our work must be done on the network thread. On 2013/07/11 22:32:08, Wez wrote: > ConnectToHost seems to store the strings while on the calling, Java thread, so > this comment seems wrong? Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:83: // We're a singleton, so Unretained is safe here. On 2013/07/11 22:32:08, Wez wrote: > Why does being a singleton make it safe? Who deletes the singleton? The singleton is deleted automatically at exit, which is after DisconnectFromHost has been invoked. Thus, all callbacks will have stopped before destruction. I'm adding a DCHECK() and comment on the destructor to this effect. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:94: chat_.reset(); // This object must outlive client_. On 2013/07/11 22:32:08, Wez wrote: > nit: Suggest putting a comment before |client_| "|client_| must be torn down > before |signalling_|." instead. And please put these sorts of comments before > the code they refer to, rather than at the end of the line. Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:95: chat_config_.reset(); // TODO(solb) Restructure to reuse between sessions. On 2013/07/11 22:32:08, Wez wrote: > Either remove this TODO or elaborate on what you intend to re-use. Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:107: // All our work must be done on the UI thread. On 2013/07/11 22:32:08, Wez wrote: > Previous method says network thread, not UI thread. Which is it? I believe it's correct as written. This method is invoked by the NegotiatingClientAuthenticator on the network thread. It trampolines the call to the UI thread and makes a JNI call into Java to prompt the user for her PIN. Upon entry of this information, Java makes a JNI call back to AuthenticateWithPin(), which invokes the callback (the one that was received as an argument to this method) on the network thread. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:128: // All our work must be done on the UI thread. On 2013/07/11 22:32:08, Wez wrote: > Here too Once again, we're invoked on the network thread but must operate on the UI one. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:147: // We ignore this message, since OnConnectionState() tells us the same thing. On 2013/07/11 22:32:08, Wez wrote: > nit: Move this into the body of the method Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:148: void ChromotingJNIInstance::OnConnectionReady(bool ready) {} On 2013/07/11 22:32:08, Wez wrote: > Why do both exist, then? Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:150: // We ignore this message because we don't deal with capabilities. On 2013/07/11 22:32:08, Wez wrote: > You don't really need this comment; it's clear from the method name that we just > ignore capabilities. Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:153: // We ignore this message because OnConnectionState() reveals the same info. On 2013/07/11 22:32:08, Wez wrote: > nit: Move this comment into the body of the method Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:168: // We don't support third party authentication, so we return a null pointer. On 2013/07/11 22:32:08, Wez wrote: > nit: Move this inside the method, and reword e.g. "Return null to indicate that > third party authentication is not supported." Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:194: // We're a singleton, so Unretained is safe here. On 2013/07/11 22:32:08, Wez wrote: > nit: Blank line before this comment. Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:41: // loops and task runners) that must outlive any use of the network stack. On 2013/07/11 22:32:08, Wez wrote: > Why is the ClientUserInterface maintaining message loops needed by network code? Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:46: // a DisconnectFromHost() call is followed by a ConnectToHost() one. On 2013/07/11 22:32:08, Wez wrote: > See style guide re comment style: e.g. Returns the singleton JNI interface. > Re-used across connections. > > Why is this re-used across connections? This will be addressed in a separate CL that separates out the ClientUserInterface components. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:49: void ConnectToHost( On 2013/07/11 22:32:08, Wez wrote: > Add comments to explain the semantics of these three methods, e.g. what happens > if I can this twice w/out disconnect, etc. Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:61: // Gets notified if the user needs to enter a PIN, and notifies Java in turn. On 2013/07/11 22:32:08, Wez wrote: > Suggest: Calls to Java to prompt the user for an authentication PIN. Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:90: scoped_ptr<base::AtExitManager> collector_; On 2013/07/11 22:32:08, Wez wrote: > nit: Add a comment to explain this. > > Does it really need to be a scoped_ptr<>? Done. It's a scoped_ptr to avoid keeping a bare pointer that might get leaked. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:101: scoped_refptr<FrameConsumerProxy> frames_; On 2013/07/11 22:32:08, Wez wrote: > frame_consumer_ Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:103: // The below variables are specific to each connection. On 2013/07/11 22:32:08, Wez wrote: > Why are per-session variables mixed in with global ones? This is for that other CL I referenced above. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:108: scoped_ptr<XmppSignalStrategy::XmppServerConfig> chat_config_; On 2013/07/11 22:32:08, Wez wrote: > chat_config_ -> signalling_config_ Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:109: scoped_ptr<XmppSignalStrategy> chat_; // must outlive client_ On 2013/07/11 22:32:08, Wez wrote: > chat_ -> signalling_ Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:109: scoped_ptr<XmppSignalStrategy> chat_; // must outlive client_ On 2013/07/11 22:32:08, Wez wrote: > If this must out-live |client_| then why does it appear after |client_| in the > member order - it'll be torn down first if it appears last. It's actually manually reset in DisconnectFromHost(). These variables are declared in the same order that they're instantiated, but the destruction order is different. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:110: scoped_ptr<NetworkSettings> netset_; On 2013/07/11 22:32:08, Wez wrote: > network_settings_ Done. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:111: protocol::SecretFetchedCallback announce_secret_; On 2013/07/11 22:32:08, Wez wrote: > Why is this called |announce_secret_|? Done.
https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:60: // TODO(solb) detach all threads from JVM On 2013/07/11 23:59:09, solb wrote: > On 2013/07/11 22:32:08, Wez wrote: > > Why the TODO? > > As I mentioned in a prior CL, it's something I plan to figure out a proper > solution to, but I just haven't had a chance yet. OK, please file a bug for that work and refer to it in the comment. Also, prefer the form: "TODO: crbug.com/<bug#> Detach threads from JVM here." https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:67: const char* host_pubkey) { On 2013/07/11 23:59:09, solb wrote: > On 2013/07/11 22:32:08, Wez wrote: > > nit: DCHECK() that you're called on the expected thread. > > The threading of this method has become less important as the code has > developed. I'll DCHECK() on whether we're currently "connected" instead. Don't other threads read these values, though? Are you sure that they can never do that while the object is disconnected? https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:83: // We're a singleton, so Unretained is safe here. On 2013/07/11 23:59:09, solb wrote: > On 2013/07/11 22:32:08, Wez wrote: > > Why does being a singleton make it safe? Who deletes the singleton? > > The singleton is deleted automatically at exit, which is after > DisconnectFromHost has been invoked. Thus, all callbacks will have stopped > before destruction. I'm adding a DCHECK() and comment on the destructor to this > effect. Ah, OK, so the guarantee is that the object will only be torn down on exit, by which time this object will be gone. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:128: // All our work must be done on the UI thread. On 2013/07/11 23:59:09, solb wrote: > On 2013/07/11 22:32:08, Wez wrote: > > Here too > > Once again, we're invoked on the network thread but must operate on the UI one. See comment above; no need for this comment at all, then. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:46: // a DisconnectFromHost() call is followed by a ConnectToHost() one. On 2013/07/11 23:59:09, solb wrote: > On 2013/07/11 22:32:08, Wez wrote: > > See style guide re comment style: e.g. Returns the singleton JNI interface. > > Re-used across connections. > > > > Why is this re-used across connections? > > This will be addressed in a separate CL that separates out the > ClientUserInterface components. OK, but please address the comment style issue. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:49: void ConnectToHost( On 2013/07/11 22:32:08, Wez wrote: > Add comments to explain the semantics of these three methods, e.g. what happens > if I can this twice w/out disconnect, etc. Chromium comments on methods are things like: "Starts a connection to |host_jid|, using the |username| and |auth_token| to authentication to the Talk network. This object can be re-used for multiple connections by calling DisconnectFromHost() in-between." https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.h:90: scoped_ptr<base::AtExitManager> collector_; On 2013/07/11 23:59:09, solb wrote: > On 2013/07/11 22:32:08, Wez wrote: > > nit: Add a comment to explain this. > > > > Does it really need to be a scoped_ptr<>? > > Done. It's a scoped_ptr to avoid keeping a bare pointer that might get leaked. Why is it not just a member, since it has the same lifetime as the containing object? https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:43: base::MessageLoop::QuitClosure()); AutoThreads don't exit until there are no more references to them, but you're holding a reference to them from a global singleton that won't be torn down until the main thread exits, so haven't you created a cyclical dependency here? https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:94: pin_ = ""; This is racy if the caller calls ConnectToHost and then immediately DisconnectFromHost then this will crash it, since you're writing these values on one thread but reading from the other. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:110: // Although we're invoked on the network thread, we must work on the UI one. This comment is superfluous, since it's clear that that's what we're doing. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:112: // We're a singleton, so Unretained is safe here. Based on the discussion around the singleton on other comments, I don't think this comment is necessary either, just put a comment e.g. in the destructor to indicate that since we're only torn down on exit, we needn't be retained. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:158: // We ignore this message because OnConnectionState() reveals the same info. This comment actually seems wrong; we just don't support pairing, right? We won't see this called, since we don't ask for pairing, so NOTIMPLEMENTED instead of the comment. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:206: protocol::AuthenticationMethod::FromString("spake2_pair")); This object doesn't implement pairing, so don't ask for spake2_pair https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:41: // loops and task runners) that must outlive any use of the rest of Chromoting. This still doesn't explain why; IIUC this object simply contains global resources such as the TaskRunners on which Chromoting components run, right? https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:57: // Must only be called during a successful or failed connection. See above. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:60: void AuthenticateWithPin(const char* pin); Comment to explain this, please! https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:114: // we're currently connected, or the connection has failed somehow. Chromium comment style would be something like: "True if ConnectToHost() has been called without a subsequent DisconnectFromHost(). i.e. |connected_| is true while connecting, or after the connection has failed, until explicitly cleared via DisconnectFromHost()."
https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:43: base::MessageLoop::QuitClosure()); On 2013/07/12 00:24:48, Wez wrote: > AutoThreads don't exit until there are no more references to them, but you're > holding a reference to them from a global singleton that won't be torn down > until the main thread exits, so haven't you created a cyclical dependency here? I don't believe so. Unlike the threads that are created here, the main (UI) thread is managed; thus, the singleton gets torn down as the shared library is unloaded, which is before the managed thread could exit. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:94: pin_ = ""; On 2013/07/12 00:24:48, Wez wrote: > This is racy if the caller calls ConnectToHost and then immediately > DisconnectFromHost then this will crash it, since you're writing these values on > one thread but reading from the other. This is always called from the same thread as ConnectToHost(), but I'll DCHECK() that now that you mention it. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:110: // Although we're invoked on the network thread, we must work on the UI one. On 2013/07/12 00:24:48, Wez wrote: > This comment is superfluous, since it's clear that that's what we're doing. Done. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:112: // We're a singleton, so Unretained is safe here. On 2013/07/12 00:24:48, Wez wrote: > Based on the discussion around the singleton on other comments, I don't think > this comment is necessary either, just put a comment e.g. in the destructor to > indicate that since we're only torn down on exit, we needn't be retained. Done. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:158: // We ignore this message because OnConnectionState() reveals the same info. On 2013/07/12 00:24:48, Wez wrote: > This comment actually seems wrong; we just don't support pairing, right? We > won't see this called, since we don't ask for pairing, so NOTIMPLEMENTED instead > of the comment. Sorry; I guess I didn't pay much attention to what this was for. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:206: protocol::AuthenticationMethod::FromString("spake2_pair")); On 2013/07/12 00:24:48, Wez wrote: > This object doesn't implement pairing, so don't ask for spake2_pair Done. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:41: // loops and task runners) that must outlive any use of the rest of Chromoting. On 2013/07/12 00:24:48, Wez wrote: > This still doesn't explain why; IIUC this object simply contains global > resources such as the TaskRunners on which Chromoting components run, right? Right. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:57: // Must only be called during a successful or failed connection. On 2013/07/12 00:24:48, Wez wrote: > See above. As covered in another comment, it's not racy because it's invoked on the same thread as ConnectToHost(). https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:60: void AuthenticateWithPin(const char* pin); On 2013/07/12 00:24:48, Wez wrote: > Comment to explain this, please! Done. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:114: // we're currently connected, or the connection has failed somehow. On 2013/07/12 00:24:48, Wez wrote: > Chromium comment style would be something like: > "True if ConnectToHost() has been called without a subsequent > DisconnectFromHost(). i.e. |connected_| is true while connecting, or after the > connection has failed, until explicitly cleared via DisconnectFromHost()." Done.
https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:60: // TODO(solb) detach all threads from JVM On 2013/07/12 00:24:48, Wez wrote: > On 2013/07/11 23:59:09, solb wrote: > > On 2013/07/11 22:32:08, Wez wrote: > > > Why the TODO? > > > > As I mentioned in a prior CL, it's something I plan to figure out a proper > > solution to, but I just haven't had a chance yet. > > OK, please file a bug for that work and refer to it in the comment. Also, prefer > the form: > > "TODO: crbug.com/<bug#> Detach threads from JVM here." Ping. https://codereview.chromium.org/18612018/diff/5629499534213120/remoting/clien... remoting/client/jni/chromoting_jni_instance.cc:83: // We're a singleton, so Unretained is safe here. On 2013/07/12 00:24:48, Wez wrote: > On 2013/07/11 23:59:09, solb wrote: > > On 2013/07/11 22:32:08, Wez wrote: > > > Why does being a singleton make it safe? Who deletes the singleton? > > > > The singleton is deleted automatically at exit, which is after > > DisconnectFromHost has been invoked. Thus, all callbacks will have stopped > > before destruction. I'm adding a DCHECK() and comment on the destructor to > this > > effect. > > Ah, OK, so the guarantee is that the object will only be torn down on exit, by > which time this object will be gone. Blah... by which time there can be no further pending tasks, by definition. https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:43: base::MessageLoop::QuitClosure()); On 2013/07/12 01:14:22, solb wrote: > On 2013/07/12 00:24:48, Wez wrote: > > AutoThreads don't exit until there are no more references to them, but you're > > holding a reference to them from a global singleton that won't be torn down > > until the main thread exits, so haven't you created a cyclical dependency > here? > > I don't believe so. Unlike the threads that are created here, the main (UI) > thread is managed; thus, the singleton gets torn down as the shared library is > unloaded, which is before the managed thread could exit. But you are wrapping the main MessageLoopForUI in an AutoThreadTaskRunner, aren't you? Is that not controlling that MessageLoop's lifetime? https://codereview.chromium.org/18612018/diff/14001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:94: pin_ = ""; On 2013/07/12 01:14:22, solb wrote: > On 2013/07/12 00:24:48, Wez wrote: > > This is racy if the caller calls ConnectToHost and then immediately > > DisconnectFromHost then this will crash it, since you're writing these values > on > > one thread but reading from the other. > > This is always called from the same thread as ConnectToHost(), but I'll DCHECK() > that now that you mention it. That's not the point - ConnectToHost and DisconnectFromHost write to these fields but e.g. ConnectToHostOnNetworkThread _reads_ them. https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:70: DCHECK(ui_task_runner_->BelongsToCurrentThread() && !connected_); DCHECK() preconditions separately, so you can see which one failed in error output. https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:152: // Pairing is unsupported. nit: No need for the comment given the NOTIMPLEMENTED :) https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:40: // contains global resources (e.g. message loops and task runners) on which the nit: Either move the bracket to the end of the line, or change the wording to "... that the Chromoting components use." https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:41: // Chromium components run. nit: Chromium -> Chromoting https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:49: // Should only be called while disconnected. Must be invoked on UI thread. Please see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:58: // invoked on UI thread. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:61: // To be called once the user has provided a PIN to the user interface. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:64: // Called by client authenticator. Calls to Java to prompt for the user's PIN. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/jni_i... File remoting/client/jni/jni_interface.cc (right): https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/jni_i... remoting/client/jni/jni_interface.cc:35: remoting::ChromotingJNIInstance::GetInstance(); // Initialize threads now. Move this comment to its own line.
Why did you ping on the crbug thing? It should be all set... https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:70: DCHECK(ui_task_runner_->BelongsToCurrentThread() && !connected_); On 2013/07/12 01:25:44, Wez wrote: > DCHECK() preconditions separately, so you can see which one failed in error > output. Done. https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:152: // Pairing is unsupported. On 2013/07/12 01:25:44, Wez wrote: > nit: No need for the comment given the NOTIMPLEMENTED :) Done. https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:40: // contains global resources (e.g. message loops and task runners) on which the On 2013/07/12 01:25:44, Wez wrote: > nit: Either move the bracket to the end of the line, or change the wording to > "... that the Chromoting components use." Done. https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:41: // Chromium components run. On 2013/07/12 01:25:44, Wez wrote: > nit: Chromium -> Chromoting Done. https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:49: // Should only be called while disconnected. Must be invoked on UI thread. On 2013/07/12 01:25:44, Wez wrote: > Please see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > Done. https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:58: // invoked on UI thread. On 2013/07/12 01:25:44, Wez wrote: > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done. https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:61: // To be called once the user has provided a PIN to the user interface. On 2013/07/12 01:25:44, Wez wrote: > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done. https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:64: // Called by client authenticator. Calls to Java to prompt for the user's PIN. On 2013/07/12 01:25:44, Wez wrote: > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done. https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/jni_i... File remoting/client/jni/jni_interface.cc (right): https://codereview.chromium.org/18612018/diff/23001/remoting/client/jni/jni_i... remoting/client/jni/jni_interface.cc:35: remoting::ChromotingJNIInstance::GetInstance(); // Initialize threads now. On 2013/07/12 01:25:44, Wez wrote: > Move this comment to its own line. Done.
Ping?
Looking good. Almost there. :) https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:42: // TODO(solb) We shouldn't really control the managed UI thread's lifetime. nit: We _don't_ really control its lifetime, I think? i.e. the problem here is that we pretent to AutoThread that we control it, but really we don't? https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:57: ChromotingJNIInstance::~ChromotingJNIInstance() { nit: check which thread this is torn down on? https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:86: DCHECK(ui_task_runner_->BelongsToCurrentThread() && connected_); Check thread & connected_ state separately, as above https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:94: void ChromotingJNIInstance::AuthenticateWithPin(const char* pin) { Do you need to check which thread this is called on as well? https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:95: pin_ = pin; Why do you store the PIN at all? You never seem to use it, except here, when invoking the pin_callback_ https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:64: void AuthenticateWithPin(const char* pin); IIUC correctly, this API should be called by Java only if the Java has received a FetchSecret request for the user's PIN? So this method should use the same terminology, a.g. ProvideSecret(), and the comment should make clear when it may be called. https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:67: // current authentication attempt is put on hold until |callback| is invoked. When you say _is_ put on hold, do you mean _should be_ put on hold, ie. the caller needs to pause processing auth stuff? https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:69: const protocol::SecretFetchedCallback& callback); This method seems to be an internal implementation detail - why is it public? https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:123: bool connected_; For this and the rest of the members below, you should clarify which thread they are used on in the comments. https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/jni_i... File remoting/client/jni/jni_interface.cc (right): https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/jni_i... remoting/client/jni/jni_interface.cc:43: // Initialize threads now. nit: Suggest: "Create the JNI singleton to set up Chromoting threads."
https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:42: // TODO(solb) We shouldn't really control the managed UI thread's lifetime. On 2013/07/12 20:13:41, Wez wrote: > nit: We _don't_ really control its lifetime, I think? > > i.e. the problem here is that we pretent to AutoThread that we control it, but > really we don't? Done. https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:57: ChromotingJNIInstance::~ChromotingJNIInstance() { On 2013/07/12 20:13:41, Wez wrote: > nit: check which thread this is torn down on? Done. https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:86: DCHECK(ui_task_runner_->BelongsToCurrentThread() && connected_); On 2013/07/12 20:13:41, Wez wrote: > Check thread & connected_ state separately, as above Sorry; I'm not sure how that slipped through. https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:94: void ChromotingJNIInstance::AuthenticateWithPin(const char* pin) { On 2013/07/12 20:13:41, Wez wrote: > Do you need to check which thread this is called on as well? Done. https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:95: pin_ = pin; On 2013/07/12 20:13:41, Wez wrote: > Why do you store the PIN at all? You never seem to use it, except here, when > invoking the pin_callback_ Good point. I think this is a bit of a leftover, but it was still functioning to ensure the copy happened before the asynchronous call. I just replaced it with a direct string constructor invocation. https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.h (right): https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:64: void AuthenticateWithPin(const char* pin); On 2013/07/12 20:13:41, Wez wrote: > IIUC correctly, this API should be called by Java only if the Java has received > a FetchSecret request for the user's PIN? So this method should use the same > terminology, a.g. ProvideSecret(), and the comment should make clear when it may > be called. Done. https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:67: // current authentication attempt is put on hold until |callback| is invoked. On 2013/07/12 20:13:41, Wez wrote: > When you say _is_ put on hold, do you mean _should be_ put on hold, ie. the > caller needs to pause processing auth stuff? No; I mean that the Chromoting authentication code won't do anything else until someone invokes its |callback|. https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:69: const protocol::SecretFetchedCallback& callback); On 2013/07/12 20:13:41, Wez wrote: > This method seems to be an internal implementation detail - why is it public? Done. https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.h:123: bool connected_; On 2013/07/12 20:13:41, Wez wrote: > For this and the rest of the members below, you should clarify which thread they > are used on in the comments. Done. https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/jni_i... File remoting/client/jni/jni_interface.cc (right): https://codereview.chromium.org/18612018/diff/33001/remoting/client/jni/jni_i... remoting/client/jni/jni_interface.cc:43: // Initialize threads now. On 2013/07/12 20:13:41, Wez wrote: > nit: Suggest: "Create the JNI singleton to set up Chromoting threads." Done.
LGTM aside from the issue below. Gary, do you want to take one last look? https://codereview.chromium.org/18612018/diff/43001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/43001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:101: // asynchronous run, since Java might want it back as soon as we return. No need for that; base::Bind() has to copy it anyway.
lgtm
https://codereview.chromium.org/18612018/diff/43001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_instance.cc (right): https://codereview.chromium.org/18612018/diff/43001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_instance.cc:101: // asynchronous run, since Java might want it back as soon as we return. On 2013/07/12 22:48:22, Wez wrote: > No need for that; base::Bind() has to copy it anyway. I'll do this in the next CL so that this lands today. Thanks, Wez!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/solb@chromium.org/18612018/43001
Message was sent while issue was closed.
Change committed as 211510 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
