[Remoting Android] Refactor ClientTelemetryLogger

[Remoting Android] Refactor ClientTelemetryLogger

This CL refactors ClientTelemetryLogger such that it is bound to one session
to avoid vulnerability of leaking session state if the logger has any bug.
TelemetryLogWriter will still have the same lifetime as the app.

Also made a ConnectToHostInfo struct to avoid passing 17 arguments to
ChromotingJniInstance.

BUG=680752
Review-Url: https://codereview.chromium.org/2643483003
Cr-Commit-Position: refs/heads/master@{#445320}
Committed: https://chromium.googlesource.com/chromium/src/+/72b3d43cd95b330a4056e06371399af57b07bacd

[Yuwei, 2017-01-17 23:11:19]

The CQ bit was checked by yuweih@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 23:12:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 23:14:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 23:14:54]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuwei, 2017-01-18 00:45:34]

The CQ bit was checked by yuweih@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 00:46:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 01:37:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 01:37:14]

Dry run: This issue passed the CQ dry run.

[Yuwei, 2017-01-18 20:58:17]

Description was changed from

==========
[Remoting Android] Refactor ClientTelemetryLogger

This CL refactors ClientTelemetryLogger such that it is bound to one session
to avoid vulnerability of leaking session state if the logger has any bug.
TelemetryLogWriter will still have the same lifetime as the app.

BUG=680752
==========

to

==========
[Remoting Android] Refactor ClientTelemetryLogger

This CL refactors ClientTelemetryLogger such that it is bound to one session
to avoid vulnerability of leaking session state if the logger has any bug.
TelemetryLogWriter will still have the same lifetime as the app.

Also made a ConnectToHostInfo struct to avoid passing 17 arguments to
ChromotingJniInstance.

BUG=680752
==========

[Yuwei, 2017-01-18 20:59:33]

yuweih@chromium.org changed reviewers:
+ lambroslambrou@chromium.org, sergeyu@chromium.org

[Yuwei, 2017-01-18 20:59:34]

yuweih@chromium.org changed required reviewers:
+ sergeyu@chromium.org

[Yuwei, 2017-01-18 20:59:34]

PTAL. I won't check this in until M57 is branched just to be safe.

[Yuwei, 2017-01-18 21:09:30]

On 2017/01/18 20:59:34, Yuwei wrote:
> PTAL. I won't check this in until M57 is branched just to be safe.

Talked to Joe offline. Since we already need to do a throughout test pass on M57
due to the existing refactoring on the OpenGL stuff, I think it's okay to check
this in for M57.

[Sergey Ulanov, 2017-01-19 00:10:34]

On 2017/01/18 21:09:30, Yuwei wrote:
> On 2017/01/18 20:59:34, Yuwei wrote:
> > PTAL. I won't check this in until M57 is branched just to be safe.
> 
> Talked to Joe offline. Since we already need to do a throughout test pass on
M57
> due to the existing refactoring on the OpenGL stuff, I think it's okay to
check
> this in for M57.

This looks like rather big change for merge and we usually don't merge
refactoring changes. If it fixes something that needs to be fixed ASAP, then it
might be better to extract the fix to a simpler change to be merged to the M57
branch.

[Sergey Ulanov, 2017-01-19 00:50:39]

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
File remoting/base/telemetry_log_writer.cc (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
remoting/base/telemetry_log_writer.cc:23: weak_ptr_ =
weak_factory_.GetWeakPtr();
this can be initialized in the initializer list above (if you move weak_ptr_
after weak_factory_), but weak_ptr can be removed completely if GetWeakPtr()
doesn't need to be thread-safe.

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
File remoting/base/telemetry_log_writer.h (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
remoting/base/telemetry_log_writer.h:46: // Can be called on any thread.
Do we actually need to allow it to be called on any thread, given that it's
usable only on one thread?

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
remoting/base/telemetry_log_writer.h:72:
DISALLOW_COPY_AND_ASSIGN(TelemetryLogWriter);
nit: add empty line here

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/client_...
File remoting/client/client_telemetry_logger.cc (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/client_...
remoting/client/client_telemetry_logger.cc:29:
base::WeakPtr<ChromotingEventLogWriter> log_writer,
ChromotingJniRuntime is guaranteed to outlive ChromotingJniInstance, so
log_writer will always outlive ClientTelemetryLogger. So do we really need to
use weak pointers here instead of raw pointers?

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/client_...
remoting/client/client_telemetry_logger.cc:52: log_writer_->Log(event);
Since log_writer_ is a weak pointer now, you need to verify that it still exists
before using. Same everywhere else log_writer_ is used in this class.
Or alternatively make log_writer_ a raw pointer and require that it outlives
this class.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_instance.cc:492: // |client_| must be torn
down before |signaling_|.
best to keep this comment before client_.reset() call.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
File remoting/client/jni/chromoting_jni_runtime.h (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_runtime.h:54: // The runtime itself will not
send out any logs. Used on the network thread.
Please explain in the comment how this is used.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_runtime.h:55:
base::WeakPtr<TelemetryLogWriter> log_writer() {
This should be called GetLogWriter(). Also please move to .cc file.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_runtime.h:57: DCHECK(log_writer_);
don't need this DCHECK. operartor->() in unique_ptr<> asserts that the pointer
is not nullptr.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_client.cc (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:49: // TODO(BUG 680752): Create
ClientTelemetryLogger here. No need to pass host
Do we still need this TODO?

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:56: info));
git cl format

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:153: const
base::android::JavaParamRef<jstring>& pairSecret,
please cleanup args names while you are here. these should be host_jid, host_id,
etc.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:158: const
base::android::JavaParamRef<jstring>& host_os_version) {
This list of parameters could be cleaned up on the java side as well, so then
this function would take one argument that references Java class. Add a TODO?

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_client.h (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.h:13: #include
"remoting/client/jni/connect_to_host_info.h"
ConnectToHostInfo can be forward-declared.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.h:33: // TODO(yuweih): Put these arguments into a
struct.
remove this TODO

[Yuwei, 2017-01-19 22:04:09]

Patchset #3 (id:40001) has been deleted

[Yuwei, 2017-01-19 22:58:16]

Patchset #3 (id:60001) has been deleted

[Yuwei, 2017-01-19 23:00:01]

PTAL

Removed GetWeakPtr from TelemetryLogWriter and just use raw pointer since the
log writer is mostly a singleton.

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
File remoting/base/telemetry_log_writer.cc (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
remoting/base/telemetry_log_writer.cc:23: weak_ptr_ =
weak_factory_.GetWeakPtr();
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> this can be initialized in the initializer list above (if you move weak_ptr_
> after weak_factory_), but weak_ptr can be removed completely if GetWeakPtr()
> doesn't need to be thread-safe.

Obsolete.

BTW I don't think you can move weak_ptr_ after weak_factory_ since weak_factory_
is supposed to be the last member to initialize so that it can get destroyed
first.

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
File remoting/base/telemetry_log_writer.h (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
remoting/base/telemetry_log_writer.h:46: // Can be called on any thread.
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> Do we actually need to allow it to be called on any thread, given that it's
> usable only on one thread?

Obsolete.

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
remoting/base/telemetry_log_writer.h:72:
DISALLOW_COPY_AND_ASSIGN(TelemetryLogWriter);
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> nit: add empty line here

Obsolete.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/client_...
File remoting/client/client_telemetry_logger.cc (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/client_...
remoting/client/client_telemetry_logger.cc:29:
base::WeakPtr<ChromotingEventLogWriter> log_writer,
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> ChromotingJniRuntime is guaranteed to outlive ChromotingJniInstance, so
> log_writer will always outlive ClientTelemetryLogger. So do we really need to
> use weak pointers here instead of raw pointers?

Done. You are right. It should be safe enough to use raw pointer.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/client_...
remoting/client/client_telemetry_logger.cc:52: log_writer_->Log(event);
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> Since log_writer_ is a weak pointer now, you need to verify that it still
exists
> before using. Same everywhere else log_writer_ is used in this class.
> Or alternatively make log_writer_ a raw pointer and require that it outlives
> this class.

Just required that log_writer_ outlives the class.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_instance.cc:492: // |client_| must be torn
down before |signaling_|.
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> best to keep this comment before client_.reset() call.

Done.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
File remoting/client/jni/chromoting_jni_runtime.h (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_runtime.h:54: // The runtime itself will not
send out any logs. Used on the network thread.
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> Please explain in the comment how this is used.

Done.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_runtime.h:55:
base::WeakPtr<TelemetryLogWriter> log_writer() {
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> This should be called GetLogWriter(). Also please move to .cc file.

Done.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/chr...
remoting/client/jni/chromoting_jni_runtime.h:57: DCHECK(log_writer_);
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> don't need this DCHECK. operartor->() in unique_ptr<> asserts that the pointer
> is not nullptr.

Done.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_client.cc (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:49: // TODO(BUG 680752): Create
ClientTelemetryLogger here. No need to pass host
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> Do we still need this TODO?

No. Removed. This is not feasible unless we allow ClientTelemetryLogger to be
created on a different thread.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:56: info));
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> git cl format

Done.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:153: const
base::android::JavaParamRef<jstring>& pairSecret,
On 2017/01/19 00:50:39, Sergey Ulanov wrote:
> please cleanup args names while you are here. these should be host_jid,
host_id,
> etc.

Done.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:158: const
base::android::JavaParamRef<jstring>& host_os_version) {
On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> This list of parameters could be cleaned up on the java side as well, so then
> this function would take one argument that references Java class. Add a TODO?

Done.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
File remoting/client/jni/jni_client.h (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.h:13: #include
"remoting/client/jni/connect_to_host_info.h"
On 2017/01/19 00:50:39, Sergey Ulanov wrote:
> ConnectToHostInfo can be forward-declared.
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

https://codereview.chromium.org/2643483003/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_client.h:33: // TODO(yuweih): Put these arguments into a
struct.
On 2017/01/19 00:50:39, Sergey Ulanov wrote:
> remove this TODO

Done.

[Sergey Ulanov, 2017-01-23 00:36:49]

lgtm

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
File remoting/base/telemetry_log_writer.cc (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
remoting/base/telemetry_log_writer.cc:23: weak_ptr_ =
weak_factory_.GetWeakPtr();
On 2017/01/19 23:00:00, Yuwei wrote:
> On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> > this can be initialized in the initializer list above (if you move weak_ptr_
> > after weak_factory_), but weak_ptr can be removed completely if GetWeakPtr()
> > doesn't need to be thread-safe.
> 
> Obsolete.
> 
> BTW I don't think you can move weak_ptr_ after weak_factory_ since
weak_factory_
> is supposed to be the last member to initialize so that it can get destroyed
> first.

Yes, it's better to keep WeakFactory last, but in practice it makes any
difference very rarely. In this case it certainly doesn't matter - it's safe to
destroy weak_ptr_ before WeakFactory.

[Yuwei, 2017-01-23 02:19:47]

Thanks!

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
File remoting/base/telemetry_log_writer.cc (right):

https://codereview.chromium.org/2643483003/diff/20001/remoting/base/telemetry...
remoting/base/telemetry_log_writer.cc:23: weak_ptr_ =
weak_factory_.GetWeakPtr();
On 2017/01/23 00:36:49, Sergey Ulanov wrote:
> On 2017/01/19 23:00:00, Yuwei wrote:
> > On 2017/01/19 00:50:38, Sergey Ulanov wrote:
> > > this can be initialized in the initializer list above (if you move
weak_ptr_
> > > after weak_factory_), but weak_ptr can be removed completely if
GetWeakPtr()
> > > doesn't need to be thread-safe.
> > 
> > Obsolete.
> > 
> > BTW I don't think you can move weak_ptr_ after weak_factory_ since
> weak_factory_
> > is supposed to be the last member to initialize so that it can get destroyed
> > first.
> 
> Yes, it's better to keep WeakFactory last, but in practice it makes any
> difference very rarely. In this case it certainly doesn't matter - it's safe
to
> destroy weak_ptr_ before WeakFactory.

Acknowledged.

[Yuwei, 2017-01-23 02:19:52]

The CQ bit was checked by yuweih@chromium.org

[commit-bot: I haz the power, 2017-01-23 02:20:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 04:19:17]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485137992035050,
"parent_rev": "08fdd54b4722bdb8f3bafdf68f7377f1214dcd44", "commit_rev":
"72b3d43cd95b330a4056e06371399af57b07bacd"}

[commit-bot: I haz the power, 2017-01-23 04:20:19]

Description was changed from

==========
[Remoting Android] Refactor ClientTelemetryLogger

This CL refactors ClientTelemetryLogger such that it is bound to one session
to avoid vulnerability of leaking session state if the logger has any bug.
TelemetryLogWriter will still have the same lifetime as the app.

Also made a ConnectToHostInfo struct to avoid passing 17 arguments to
ChromotingJniInstance.

BUG=680752
==========

to

==========
[Remoting Android] Refactor ClientTelemetryLogger

This CL refactors ClientTelemetryLogger such that it is bound to one session
to avoid vulnerability of leaking session state if the logger has any bug.
TelemetryLogWriter will still have the same lifetime as the app.

Also made a ConnectToHostInfo struct to avoid passing 17 arguments to
ChromotingJniInstance.

BUG=680752

Review-Url: https://codereview.chromium.org/2643483003
Cr-Commit-Position: refs/heads/master@{#445320}
Committed:
https://chromium.googlesource.com/chromium/src/+/72b3d43cd95b330a4056e0637139...
==========

[commit-bot: I haz the power, 2017-01-23 04:20:20]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/72b3d43cd95b330a4056e0637139...