New issue
Advanced search Search tips

Issue 615288 link

Starred by 0 users

Issue metadata

Status: Verified
Owner:
Closed: Jun 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Crash when connecting from Android to any Host

Project Member Reported by ajnolley@chromium.org, May 27 2016

Issue description

Version: 52.0.2743.10
OS: Android

What steps will reproduce the problem?
(1)Attempt to connect to any host using the version listed 
(2)~50% of the time it will crash
(3)logcat is included

This is a regression



 
logcat
11.8 KB View Download
Components: Services>Chromoting
Owner: yuweih@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by yuweih@chromium.org, May 27 2016

I can't repro this on my Nexus 6P anymore (after an Android update, why...) but I can repro this using AJ's tablet....

This can be reproduced by the ToT build, and the log looks like this:

05-27 14:19:19.683: A/chromium(14287): [0527/141919:FATAL:chromoting_jni_instance.cc(96)] Check failed: !view_. 
05-27 14:19:19.683: A/chromium(14287): #00 0xe0e14067 /data/app/org.chromium.chromoting-2/lib/arm/libremoting_client_jni.so+0x00b2a067
05-27 14:19:19.683: A/chromium(14287): #01 0xe0392bf1 /data/app/org.chromium.chromoting-2/lib/arm/libremoting_client_jni.so+0x000a8bf1
05-27 14:19:19.683: A/chromium(14287): #02 0xe0392d9d /data/app/org.chromium.chromoting-2/lib/arm/libremoting_client_jni.so+0x000a8d9d
05-27 14:19:19.683: A/chromium(14287): #03 0xe0394c23 /data/app/org.chromium.chromoting-2/lib/arm/libremoting_client_jni.so+0x000aac23
05-27 14:19:19.683: A/chromium(14287): #04 0xe0394bf7 /data/app/org.chromium.chromoting-2/lib/arm/libremoting_client_jni.so+0x000aabf7
05-27 14:19:19.683: A/chromium(14287): #05 0xe0e02dbd /data/app/org.chromium.chromoting-2/lib/arm/libremoting_client_jni.so+0x00b18dbd
05-27 14:19:19.683: A/chromium(14287): #06 0xe0392423 /data/app/org.chromium.chromoting-2/lib/arm/libremoting_client_jni.so+0x000a8423
05-27 14:19:19.683: A/chromium(14287): #07 0xe0397115 /data/app/org.chromium.chromoting-2/lib/arm/libremoting_client_jni.so+0x000ad115
05-27 14:19:19.683: A/chromium(14287): #08 0xe03964f1 /data/app/org.chromium.chromoting-2/lib/arm/libremoting_client_jni.so+0x000ac4f1
05-27 14:19:19.683: A/chromium(14287): #09 0xe0396455 /data/app/org.chromium.chromoting-2/lib/arm/libremoting_client_jni.so+0x000ac455
05-27 14:19:19.683: A/chromium(14287): #10 0xe1de29b5 /data/app/org.chromium.chromoting-2/oat/arm/base.odex+0x007b99b5
05-27 14:19:19.683: A/libc(14287): Fatal signal 6 (SIGABRT), code -6 in tid 14287 (mium.chromoting)

which is exactly the problem we have in 611893 and should be solved in 614242. The log also suggests that invalidating the weak pointer on the network thread doesn't resolve this problem.

I tried the build with CL patch 2007123003 and the problem goes away.

The thread model in Android client has long been problematic, and became flaky after I committed the CL 1976853002 which made the connectToHost call asynchronous.

There are basically two solutions:

1. Merge 2007403002 and 2007123003 after the latter one is done.
1. Make connectToHost synchronous again, which is bad since it doesn't care whether the auth token is stale or not.

Comment 4 by yuweih@chromium.org, May 28 2016

Looks like making connectToHost synchronous doesn't really solve this problem. Symbolized logs has shown weird behavior of the refcounter.

JniClient holds a reference to ChromotingJniInstance. The network thread holds a reference to the instance for connecting to the host on network inside the ctor of ChromotingJniInstance, and it frees the instance when the task is done, which doesn't make sense since JniClient is still holding a reference to the instance.

Lambrous and I have no idea what really happens, but seems like separating the class by threads does solve this problem.

------

Done a continuous connect-and-cancel experiment with CL patch 1998353002.

# connect-and-cancel takes to crash:

ToT: 12, 5, 3, 3, 3, 2, 26, 9

With patch 2007123003: >340, >532, >314

No crash observed. Started getting XMPP error instead...

Comment 5 by yuweih@chromium.org, May 28 2016

Thanks to wez@'s comment on the design doc. I've finally figured out what was going on :)

Refcounted object is initialized to have zero refcount and will be increased to 1 in RefCountedThreadSafe::operator=().
There is a chance that ConnectToHost finishes earlier than operator=(), which drops the refcount to 0 and triggers the d'tor.

Moving the PostTask code out of the ctor should be enough to solve this problem.
Project Member

Comment 6 by bugdroid1@chromium.org, May 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/35c9e62b75248238526386edc44c46794adac095

commit 35c9e62b75248238526386edc44c46794adac095
Author: yuweih <yuweih@chromium.org>
Date: Tue May 31 22:50:52 2016

Fix bug of posting task inside constructor

Refcounted object is initialized to have zero refcount and will be increased
to 1 in RefCountedThreadSafe::operator=(). There is a chance that
ConnectToHost finishes earlier than operator=(), which drops the refcount to
0 and triggers the d'tor.

Adding sleep(1) after PostTask() shows stable repro of this problem.

This CL fixes this problem by pulling the PostTask code out of the ctor.

BUG= 615288 

Review-Url: https://codereview.chromium.org/2023113002
Cr-Commit-Position: refs/heads/master@{#396961}

[modify] https://crrev.com/35c9e62b75248238526386edc44c46794adac095/remoting/client/jni/chromoting_jni_instance.cc
[modify] https://crrev.com/35c9e62b75248238526386edc44c46794adac095/remoting/client/jni/chromoting_jni_instance.h
[modify] https://crrev.com/35c9e62b75248238526386edc44c46794adac095/remoting/client/jni/chromoting_jni_runtime.cc

Labels: Merge-Request-52
Tested on official build 53.0.2754.0. These changes will only affect the Chrome Remote Desktop Android app and will not affect the Chrome browser.

Comment 8 by tin...@google.com, Jun 1 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 1 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c6ab1822890f8eebdf8aca40a26b4715518a4767

commit c6ab1822890f8eebdf8aca40a26b4715518a4767
Author: Lambros Lambrou <lambroslambrou@chromium.org>
Date: Wed Jun 01 17:12:18 2016

Fix bug of posting task inside constructor

Refcounted object is initialized to have zero refcount and will be increased
to 1 in RefCountedThreadSafe::operator=(). There is a chance that
ConnectToHost finishes earlier than operator=(), which drops the refcount to
0 and triggers the d'tor.

Adding sleep(1) after PostTask() shows stable repro of this problem.

This CL fixes this problem by pulling the PostTask code out of the ctor.

BUG= 615288 

Review-Url: https://codereview.chromium.org/2023113002
Cr-Commit-Position: refs/heads/master@{#396961}
(cherry picked from commit 35c9e62b75248238526386edc44c46794adac095)

Review URL: https://codereview.chromium.org/2029103002 .

Cr-Commit-Position: refs/branch-heads/2743@{#165}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/c6ab1822890f8eebdf8aca40a26b4715518a4767/remoting/client/jni/chromoting_jni_instance.cc
[modify] https://crrev.com/c6ab1822890f8eebdf8aca40a26b4715518a4767/remoting/client/jni/chromoting_jni_instance.h
[modify] https://crrev.com/c6ab1822890f8eebdf8aca40a26b4715518a4767/remoting/client/jni/chromoting_jni_runtime.cc

Status: Fixed (was: Assigned)
52.0.2743.23 or later will have the fix.
lambroslambrou@, thanks for merging this for me!
Status: Verified (was: Fixed)
I made ~20 connection attempts per host platform with no hiccups. Verified fixed in 52.0.2743.26

Sign in to add a comment