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

[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
Committed: https://crrev.com/800d69331486e09464761a89d431027dd4d0acff
Cr-Commit-Position: refs/heads/master@{#419841}

[Yuwei, 2016-09-12 20:49:05]

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

[Yuwei, 2016-09-12 20:49:06]

PTAL. Thanks!

https://codereview.chromium.org/2338473002/diff/1/remoting/client/jni/display...
File remoting/client/jni/display_updater_factory.h (right):

https://codereview.chromium.org/2338473002/diff/1/remoting/client/jni/display...
remoting/client/jni/display_updater_factory.h:24: virtual
~DisplayUpdaterFactory() {}
Tried to make this protected but `display_handler_.reset(raw_handler)` requires
the destructor.

BTW we may also remove this interface? Since now we only have one renderer.

[Yuwei, 2016-09-12 21:34:44]

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

https://codereview.chromium.org/2338473002/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_gl_display_handler.cc:68: ui_task_poster_.reset();
Releasing |ui_task_poster_| here may not be very safe for functions like
OnCursorMoved. I'll move this back to the destructor.

[Hzj_jie, 2016-09-13 17:56:28]

zijiehe@chromium.org changed reviewers:
+ zijiehe@chromium.org

[Hzj_jie, 2016-09-13 17:56:29]

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/dis...
File remoting/client/jni/display_updater_factory.h (right):

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/dis...
remoting/client/jni/display_updater_factory.h:24: virtual
~DisplayUpdaterFactory() {}
protected?

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/dis...
remoting/client/jni/display_updater_factory.h:29: virtual void Destroy() = 0;
Add some comments to explain the meaning of this function?

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/jni...
File remoting/client/jni/jni_gl_display_handler.cc (right):

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/jni...
remoting/client/jni/jni_gl_display_handler.cc:117: return;
Suggest to add a PostTaskToDisplayThread(const base::Closure&) function to avoid
these !ui_task_poster_ checks.

[Yuwei, 2016-09-13 18:25:18]

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/dis...
File remoting/client/jni/display_updater_factory.h (right):

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/dis...
remoting/client/jni/display_updater_factory.h:24: virtual
~DisplayUpdaterFactory() {}
On 2016/09/13 17:56:29, Hzj_jie wrote:
> protected?

I commented in previous patch. `display_handler_.reset(raw_handler)` requires a
public destructor so I'm not sure how to bypass this...

[Yuwei, 2016-09-13 19:06:18]

ptal

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

https://codereview.chromium.org/2338473002/diff/20001/remoting/client/jni/jni...
remoting/client/jni/jni_gl_display_handler.cc:68: ui_task_poster_.reset();
On 2016/09/12 21:34:44, Yuwei wrote:
> Releasing |ui_task_poster_| here may not be very safe for functions like
> OnCursorMoved. I'll move this back to the destructor.

I think it's better to just null-check before using it.

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/dis...
File remoting/client/jni/display_updater_factory.h (right):

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/dis...
remoting/client/jni/display_updater_factory.h:24: virtual
~DisplayUpdaterFactory() {}
On 2016/09/13 17:56:29, Hzj_jie wrote:
> protected?

Tried to fix this by providing a deleter that calls Destroy().

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/dis...
remoting/client/jni/display_updater_factory.h:29: virtual void Destroy() = 0;
On 2016/09/13 17:56:29, Hzj_jie wrote:
> Add some comments to explain the meaning of this function?

Done.

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/jni...
File remoting/client/jni/jni_gl_display_handler.cc (right):

https://codereview.chromium.org/2338473002/diff/40001/remoting/client/jni/jni...
remoting/client/jni/jni_gl_display_handler.cc:117: return;
On 2016/09/13 17:56:29, Hzj_jie wrote:
> Suggest to add a PostTaskToDisplayThread(const base::Closure&) function to
avoid
> these !ui_task_poster_ checks.

Done. Slightly different name.

[Hzj_jie, 2016-09-13 19:32:11]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2016-09-13 19:32:12]

lgtm

https://codereview.chromium.org/2338473002/diff/60001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java
(right):

https://codereview.chromium.org/2338473002/diff/60001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:33: private
long mNativeJniGlDisplay;
Now you can move this field down :)

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/dis...
File remoting/client/jni/display_updater_factory.h (right):

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/dis...
remoting/client/jni/display_updater_factory.h:22: class DisplayUpdaterFactory {
Will this class be removed eventually? It looks like JniGlDisplayHandler is the
only consumer.

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/dis...
remoting/client/jni/display_updater_factory.h:26: void
operator()(DisplayUpdaterFactory* factory) {
if (factory) {
  factory->Destroy();
}

I happen to find a doc regarding to the deleter of smart pointers.
http://stackoverflow.com/questions/11164354/does-the-standard-behavior-for-de....
So this deleter may be called even with a nullptr in shared_ptr.

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

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:76: if (display_handler_) {
You do not need to do this check, unique_ptr will do it for you.

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/jni...
File remoting/client/jni/jni_gl_display_handler.h (right):

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/jni...
remoting/client/jni/jni_gl_display_handler.h:89: ~JniGlDisplayHandler()
override;
I suggest to keep this destructor empty, and move the checks to
DeleteOnDisplayThread().

[Hzj_jie, 2016-09-13 19:32:19]

The CQ bit was unchecked by zijiehe@chromium.org

[Yuwei, 2016-09-13 20:49:41]

Thanks!

PTAL

https://codereview.chromium.org/2338473002/diff/60001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java
(right):

https://codereview.chromium.org/2338473002/diff/60001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:33: private
long mNativeJniGlDisplay;
On 2016/09/13 19:32:12, Hzj_jie wrote:
> Now you can move this field down :)

Yep. Moved after merging ToT :P

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/dis...
File remoting/client/jni/display_updater_factory.h (right):

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/dis...
remoting/client/jni/display_updater_factory.h:22: class DisplayUpdaterFactory {
On 2016/09/13 19:32:12, Hzj_jie wrote:
> Will this class be removed eventually? It looks like JniGlDisplayHandler is
the
> only consumer.

I'm thinking about removing this class but just not sure whether there will be a
different RenderStub (VR?) in the future...

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/dis...
remoting/client/jni/display_updater_factory.h:26: void
operator()(DisplayUpdaterFactory* factory) {
On 2016/09/13 19:32:12, Hzj_jie wrote:
> if (factory) {
>   factory->Destroy();
> }
> 
> I happen to find a doc regarding to the deleter of smart pointers.
>
http://stackoverflow.com/questions/11164354/does-the-standard-behavior-for-de....
> So this deleter may be called even with a nullptr in shared_ptr.

Added the null-check.

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

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/jni...
remoting/client/jni/jni_client.cc:76: if (display_handler_) {
On 2016/09/13 19:32:12, Hzj_jie wrote:
> You do not need to do this check, unique_ptr will do it for you.

Oops... Forgot that.

Done.

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/jni...
File remoting/client/jni/jni_gl_display_handler.h (right):

https://codereview.chromium.org/2338473002/diff/60001/remoting/client/jni/jni...
remoting/client/jni/jni_gl_display_handler.h:89: ~JniGlDisplayHandler()
override;
On 2016/09/13 19:32:12, Hzj_jie wrote:
> I suggest to keep this destructor empty, and move the checks to
> DeleteOnDisplayThread().

Done.

[Yuwei, 2016-09-15 23:23:27]

ping!

[Lambros, 2016-09-16 01:44:12]

lgtm

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/di...
File remoting/client/jni/display_updater_factory.h (right):

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/di...
remoting/client/jni/display_updater_factory.h:24: // A deleter that calls
Destroy() to delete the factory.
Does it make sense for this deleter (and Destroy method) to be here in the
interface, instead of in JniGlDisplayHandler?

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
File remoting/client/jni/jni_gl_display_handler.h (right):

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
remoting/client/jni/jni_gl_display_handler.h:91: // Queues a task running on
current call stack. All queued tasks will be
This first sentence is confusing because of the phrase 'call stack'. I'm not
sure how the call stack relates to what this method does.

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
remoting/client/jni/jni_gl_display_handler.h:96: // This avoids listing
DeleteHelper as friend.
I would rephrase this comment without 'DeleteHelper' which I guess(?) is an
implementation detail of TaskRunner?

I would say what the method does first:
"This simply calls the dtor."
Then say why:
"This is to allow TaskRunner::PostTask() to access this private dtor, instead of
using TaskRunner::DeleteSoon() which would require the dtor to be visible".

[Yuwei, 2016-09-16 21:49:45]

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/di...
File remoting/client/jni/display_updater_factory.h (right):

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/di...
remoting/client/jni/display_updater_factory.h:24: // A deleter that calls
Destroy() to delete the factory.
On 2016/09/16 01:44:12, Lambros wrote:
> Does it make sense for this deleter (and Destroy method) to be here in the
> interface, instead of in JniGlDisplayHandler?

I don't have a strong opinion. My justifications are:

1. Anyone who needs to create a reset-able unique_ptr will need such a deleter
2. Currently JniClient references to the DisplayHandler as the interface so
Destroy() probably can't be removed from the interface. Also arguably it may be
good if any future renderer implementations can have a chance to handle the
destroy process by itself.

[Sergey Ulanov, 2016-09-19 21:14:56]

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/di...
File remoting/client/jni/display_updater_factory.h (right):

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/di...
remoting/client/jni/display_updater_factory.h:22: class DisplayUpdaterFactory {
Do we need to keep this interface? I think we needed it to support two
capturers, but I don't think we will need this going forward, so maybe we can
just remove it?

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
File remoting/client/jni/jni_client.h (right):

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
remoting/client/jni/jni_client.h:160: std::unique_ptr<DisplayUpdaterFactory,
DisplayUpdaterFactory::Deleter>
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]

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
File remoting/client/jni/jni_client.h (right):

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
remoting/client/jni/jni_client.h:160: std::unique_ptr<DisplayUpdaterFactory,
DisplayUpdaterFactory::Deleter>
On 2016/09/19 21:14:56, Sergey Ulanov wrote:
> 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.

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]

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
File remoting/client/jni/jni_client.h (right):

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
remoting/client/jni/jni_client.h:160: std::unique_ptr<DisplayUpdaterFactory,
DisplayUpdaterFactory::Deleter>
On 2016/09/19 22:30:37, Yuwei wrote:
> On 2016/09/19 21:14:56, Sergey Ulanov wrote:
> > 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.
> 
> 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.

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.

[Yuwei, 2016-09-19 23:52:11]

PTAL. Thanks!

Changes:
* Removed the DisplayUpdaterFactory interface
* Defined the lifetime as ctor->initialize->invalidate->dtor similar to
ChromotingJniInstance

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/di...
File remoting/client/jni/display_updater_factory.h (right):

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/di...
remoting/client/jni/display_updater_factory.h:22: class DisplayUpdaterFactory {
On 2016/09/19 21:14:56, Sergey Ulanov wrote:
> Do we need to keep this interface? I think we needed it to support two
> capturers, but I don't think we will need this going forward, so maybe we can
> just remove it?

That's what I have considered. Removed.

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
File remoting/client/jni/jni_gl_display_handler.h (right):

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
remoting/client/jni/jni_gl_display_handler.h:91: // Queues a task running on
current call stack. All queued tasks will be
On 2016/09/16 01:44:12, Lambros wrote:
> This first sentence is confusing because of the phrase 'call stack'. I'm not
> sure how the call stack relates to what this method does.

Removed.

https://codereview.chromium.org/2338473002/diff/120001/remoting/client/jni/jn...
remoting/client/jni/jni_gl_display_handler.h:96: // This avoids listing
DeleteHelper as friend.
On 2016/09/16 01:44:12, Lambros wrote:
> I would rephrase this comment without 'DeleteHelper' which I guess(?) is an
> implementation detail of TaskRunner?
> 
> I would say what the method does first:
> "This simply calls the dtor."
> Then say why:
> "This is to allow TaskRunner::PostTask() to access this private dtor, instead
of
> using TaskRunner::DeleteSoon() which would require the dtor to be visible".

Obsolete.

[Sergey Ulanov, 2016-09-20 18:09:34]

LGTM.
I think it would be best to split JniGlDisplayHandler into two classes: one for
UI thread and one for display thread.
The one for display can be nested in JniGlDisplayHandler (e.g. call it
JniGlDisplayHandler::Core). That would make separation between the threads very
clear and will allow to avoid Invalidate(). This can be done in a separate CL,
but maybe add TODO for now.

https://codereview.chromium.org/2338473002/diff/160001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java
(right):

https://codereview.chromium.org/2338473002/diff/160001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:26: * 
Callbacks by native counterpart are called on UI.
s/UI/the UI thread/
Or maybe rephrase the whole comment, e.g.:
  All functions, including callbacks from native code are called only on
  UI thread.

https://codereview.chromium.org/2338473002/diff/160001/remoting/client/jni/jn...
File remoting/client/jni/jni_gl_display_handler.cc (right):

https://codereview.chromium.org/2338473002/diff/160001/remoting/client/jni/jn...
remoting/client/jni/jni_gl_display_handler.cc:151: void
JniGlDisplayHandler::QueueTaskOnDisplayThread(const base::Closure& task) {
this functions posts tasks to the UI thread, so the name is incorrect

https://codereview.chromium.org/2338473002/diff/160001/remoting/client/jni/jn...
remoting/client/jni/jni_gl_display_handler.cc:154: }
indentation

[Yuwei, 2016-09-20 19:20:38]

Thanks!

> I think it would be best to split JniGlDisplayHandler into two classes: one
for
> UI thread and one for display thread.

Acknowledged.

The one for display can be nested in JniGlDisplayHandler (e.g. call it
JniGlDisplayHandler::Core). That would make separation between the threads very
clear and will allow to avoid Invalidate(). This can be done in a separate CL,
but maybe add TODO for now.

Added a TODO

https://codereview.chromium.org/2338473002/diff/160001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java
(right):

https://codereview.chromium.org/2338473002/diff/160001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:26: * 
Callbacks by native counterpart are called on UI.
On 2016/09/20 18:09:34, Sergey Ulanov wrote:
> s/UI/the UI thread/
> Or maybe rephrase the whole comment, e.g.:
>   All functions, including callbacks from native code are called only on
>   UI thread.

Done.

https://codereview.chromium.org/2338473002/diff/160001/remoting/client/jni/jn...
File remoting/client/jni/jni_gl_display_handler.cc (right):

https://codereview.chromium.org/2338473002/diff/160001/remoting/client/jni/jn...
remoting/client/jni/jni_gl_display_handler.cc:151: void
JniGlDisplayHandler::QueueTaskOnDisplayThread(const base::Closure& task) {
On 2016/09/20 18:09:34, Sergey Ulanov wrote:
> this functions posts tasks to the UI thread, so the name is incorrect

Changed to `PostSequentialTaskOnDisplayThread`.

https://codereview.chromium.org/2338473002/diff/160001/remoting/client/jni/jn...
remoting/client/jni/jni_gl_display_handler.cc:154: }
On 2016/09/20 18:09:34, Sergey Ulanov wrote:
> indentation

Done.

[Yuwei, 2016-09-20 19:21:19]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2016-09-20 19:21:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
zijiehe@chromium.org, lambroslambrou@chromium.org, sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2338473002/#ps180001
(title: "Reviewer's Feedback")

[commit-bot: I haz the power, 2016-09-20 19:22:01]

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, 2016-09-20 19:33:50]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-20 19:37:54]

Description was changed from

==========
[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
==========

to

==========
[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

Committed: https://crrev.com/800d69331486e09464761a89d431027dd4d0acff
Cr-Commit-Position: refs/heads/master@{#419841}
==========

[commit-bot: I haz the power, 2016-09-20 19:37:56]

Patchset 10 (id:??) landed as
https://crrev.com/800d69331486e09464761a89d431027dd4d0acff
Cr-Commit-Position: refs/heads/master@{#419841}