New issue
Advanced search Search tips

Issue 646116 link

Starred by 1 user
Status: Fixed
Owner:
yuweih@chromium.org
Closed: Oct 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

[Remoting Android] GlDisplay.invalidate should be called on UI thread

Project Member Reported by yuweih@chromium.org, Sep 12 2016 
Currently the native pointer in GlDisplay is used on UI thread and null'ed on display thread like this:

Using:

if (mNativeJniGlDisplay != 0) {
  call native function...
}

Invalidating:

mNativeJniGlDisplay = 0;

And there is no synchronization (except for just marking the native pointer as volatile). If invalidation happens between the check and actually calling the native function, then the app will crash.

Currently no crash caused by this has been repro'ed since it's very unlikely to happen, but we should still fix this to prevent flakiness.
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 20 2016 

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

commit 800d69331486e09464761a89d431027dd4d0acff
Author: yuweih <yuweih@chromium.org>
Date: Tue Sep 20 19:33:27 2016

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

Currently the native pointer in GlDisplay is used on UI thread and null'ed
on display thread, and we check whether the pointer is null before using it.
If invalidate() is called between the check and actually calling the native
function then the app may crash.

This CL fixes this by making JniGlDisplayHandler call invalidate() on UI
thread.

BUG= 646116 

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

[modify] https://crrev.com/800d69331486e09464761a89d431027dd4d0acff/remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java
[modify] https://crrev.com/800d69331486e09464761a89d431027dd4d0acff/remoting/client/jni/chromoting_jni_instance.cc
[delete] https://crrev.com/1f85ba7bedef44c5de50c55b6f636f4173c7dbd6/remoting/client/jni/display_updater_factory.h
[modify] https://crrev.com/800d69331486e09464761a89d431027dd4d0acff/remoting/client/jni/jni_client.cc
[modify] https://crrev.com/800d69331486e09464761a89d431027dd4d0acff/remoting/client/jni/jni_client.h
[modify] https://crrev.com/800d69331486e09464761a89d431027dd4d0acff/remoting/client/jni/jni_gl_display_handler.cc
[modify] https://crrev.com/800d69331486e09464761a89d431027dd4d0acff/remoting/client/jni/jni_gl_display_handler.h
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 4 2016 

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

commit b78ae9e328113495720d85810d08bd8dd9bf53da
Author: yuweih <yuweih@chromium.org>
Date: Tue Oct 04 23:12:52 2016

[Remoting Android] Separate the display core from JniGlDisplayHandler

This CL separates the core that lives entirely on the display thread from
JniGlDisplayHandler so that the thread model can be clearer.

BUG= 646116 

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

[modify] https://crrev.com/b78ae9e328113495720d85810d08bd8dd9bf53da/remoting/client/jni/jni_client.cc
[modify] https://crrev.com/b78ae9e328113495720d85810d08bd8dd9bf53da/remoting/client/jni/jni_gl_display_handler.cc
[modify] https://crrev.com/b78ae9e328113495720d85810d08bd8dd9bf53da/remoting/client/jni/jni_gl_display_handler.h

Comment 3 by yuweih@chromium.org, Oct 5 2016

Status: Fixed (was: Assigned)
►

Sign in to add a comment