aw: Fix race between first frame draw and view detach causing deadlock.

aw: Fix race between first frame draw and view detach causing deadlock.

If a AwContents.onDetachedFromWindow occurs at simulaneously with
RenderThreadManager::DrawGL, then the following sequence of events can
occur:

* RenderThreadManager::DrawGL creates hardware_renderer_ because a frame
  is ready to be committed to the HardwareRenderer.
* BrowserViewRenderer::OnDetachedFromWindow calls the uncommitted frame
  to be returned to the child compositor.
* RenderThreadManager tries to commit the frame that is just returned,
  leaving it in a state where neither the RenderThreadManager, nor its
  HardwareRenderer has a frame.

Then subsequently when deleting native objects, DeleteHardwareRendererOnUI
does no work, meaning that hardware_renderer_ remains set at the time
that the RenderThreadManager destructor is called, leading to a DCHECK,
or deadlock.

In order to avoid this, we consider RenderThreadManager to have a
frame (and thus need to run the synchronous RenderThread cleanup in
DeleteHardwareRendererOnUI) if it has ever received a frame (stored in
has_received_frame_), rather than if it or hardware_renderer_ currently
has a frame.

In order that BrowserViewRenderer receives the right signal post
deletion of hardware_renderer_, we reset has_received_frame_ when
RenderThreadManager deletes hardware_renderer_, if RenderThreadManager
does not currently have a frame to commit to a new HardwareRenderer
instance.

BUG=668692
Committed: https://crrev.com/ff08fc807243b2317ef1c8b42f3f423496d12a76
Cr-Commit-Position: refs/heads/master@{#436672}

[Tobias Sargeant, 2016-12-06 15:50:52]

tobiasjs@chromium.org changed reviewers:
+ boliu@chromium.org

[Tobias Sargeant, 2016-12-06 15:50:52]

I had another thought on how we could fix this, which would be to add a
HardwareRenderer constructor that took a queue of initial frames to commit, and
then in DrawGL where we construct hardware_renderer_ we swap child_frame_ while
holding the lock, and if we are left holding frames, we construct
hardware_renderer_ with them directly.

[Tobias Sargeant, 2016-12-06 15:51:01]

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

[commit-bot: I haz the power, 2016-12-06 15:51:16]

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

[boliu, 2016-12-06 16:15:35]

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
File android_webview/browser/render_thread_manager.cc (right):

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
android_webview/browser/render_thread_manager.cc:187: has_received_frame_ =
true;
maybe add a TODO here, this doesn't work if we switch to full async draw, ie we
only get a future here and don't actually know if the frame is empty or not,
maybe update TODO in BVR.h as well to point to this issue

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
android_webview/browser/render_thread_manager.cc:342: has_received_frame_ =
HasFrameForHardwareRendererOnRT();
I don't understand this

also this means has_received_frame_ is accessed on RT as well

[commit-bot: I haz the power, 2016-12-06 16:16:37]

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

[commit-bot: I haz the power, 2016-12-06 16:16:38]

Dry run: This issue passed the CQ dry run.

[Tobias Sargeant, 2016-12-06 16:29:27]

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
File android_webview/browser/render_thread_manager.cc (right):

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
android_webview/browser/render_thread_manager.cc:342: has_received_frame_ =
HasFrameForHardwareRendererOnRT();
On 2016/12/06 16:15:35, boliu wrote:
> I don't understand this
> 
> also this means has_received_frame_ is accessed on RT as well

Good point about has_received_frame_. The thing I wanted to avoid was this
sequence:

* has_received_frame_ is true
* detach causes hr_ destroy but has_received_frame_ is not reset.
* first frame after attach (in sync mode) is empty, causing BVR::OnDrawHardware
to return true, even though there's nothing to draw.

[boliu, 2016-12-06 16:34:10]

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
File android_webview/browser/render_thread_manager.cc (right):

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
android_webview/browser/render_thread_manager.cc:342: has_received_frame_ =
HasFrameForHardwareRendererOnRT();
On 2016/12/06 16:29:27, Tobias Sargeant wrote:
> On 2016/12/06 16:15:35, boliu wrote:
> > I don't understand this
> > 
> > also this means has_received_frame_ is accessed on RT as well
> 
> Good point about has_received_frame_. The thing I wanted to avoid was this
> sequence:
> 
> * has_received_frame_ is true
> * detach causes hr_ destroy but has_received_frame_ is not reset.

can you unset has_received_frame_ at end of DeleteHardwareRendererOnUI, on UI
thread?

> * first frame after attach (in sync mode) is empty, causing
BVR::OnDrawHardware
> to return true, even though there's nothing to draw.

[Tobias Sargeant, 2016-12-06 16:36:38]

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
File android_webview/browser/render_thread_manager.cc (right):

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
android_webview/browser/render_thread_manager.cc:342: has_received_frame_ =
HasFrameForHardwareRendererOnRT();
On 2016/12/06 16:34:10, boliu wrote:
> On 2016/12/06 16:29:27, Tobias Sargeant wrote:
> > On 2016/12/06 16:15:35, boliu wrote:
> > > I don't understand this
> > > 
> > > also this means has_received_frame_ is accessed on RT as well
> > 
> > Good point about has_received_frame_. The thing I wanted to avoid was this
> > sequence:
> > 
> > * has_received_frame_ is true
> > * detach causes hr_ destroy but has_received_frame_ is not reset.
> 
> can you unset has_received_frame_ at end of DeleteHardwareRendererOnUI, on UI
> thread?
> 
> > * first frame after attach (in sync mode) is empty, causing
> BVR::OnDrawHardware
> > to return true, even though there's nothing to draw.
> 

Yes. That's a much better solution.

[Tobias Sargeant, 2016-12-06 18:11:26]

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
File android_webview/browser/render_thread_manager.cc (right):

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
android_webview/browser/render_thread_manager.cc:187: has_received_frame_ =
true;
On 2016/12/06 16:15:35, boliu wrote:
> maybe add a TODO here, this doesn't work if we switch to full async draw, ie
we
> only get a future here and don't actually know if the frame is empty or not,
> maybe update TODO in BVR.h as well to point to this issue

Done.

https://codereview.chromium.org/2560463002/diff/1/android_webview/browser/ren...
android_webview/browser/render_thread_manager.cc:342: has_received_frame_ =
HasFrameForHardwareRendererOnRT();
On 2016/12/06 16:36:38, Tobias Sargeant wrote:
> On 2016/12/06 16:34:10, boliu wrote:
> > On 2016/12/06 16:29:27, Tobias Sargeant wrote:
> > > On 2016/12/06 16:15:35, boliu wrote:
> > > > I don't understand this
> > > > 
> > > > also this means has_received_frame_ is accessed on RT as well
> > > 
> > > Good point about has_received_frame_. The thing I wanted to avoid was this
> > > sequence:
> > > 
> > > * has_received_frame_ is true
> > > * detach causes hr_ destroy but has_received_frame_ is not reset.
> > 
> > can you unset has_received_frame_ at end of DeleteHardwareRendererOnUI, on
UI
> > thread?
> > 
> > > * first frame after attach (in sync mode) is empty, causing
> > BVR::OnDrawHardware
> > > to return true, even though there's nothing to draw.
> > 
> 
> Yes. That's a much better solution.

Done.

[boliu, 2016-12-06 18:15:03]

https://codereview.chromium.org/2560463002/diff/20001/android_webview/browser...
File android_webview/browser/render_thread_manager.cc (right):

https://codereview.chromium.org/2560463002/diff/20001/android_webview/browser...
android_webview/browser/render_thread_manager.cc:392: has_received_frame_ =
!child_frames_.empty();
the RequestInvokeGL below has nothing to do with HR. HR should definitely be
deleted at this point

so I don't get this either. just set has_received_frame_ to false at end of this
method?

[Tobias Sargeant, 2016-12-06 18:22:55]

https://codereview.chromium.org/2560463002/diff/20001/android_webview/browser...
File android_webview/browser/render_thread_manager.cc (right):

https://codereview.chromium.org/2560463002/diff/20001/android_webview/browser...
android_webview/browser/render_thread_manager.cc:392: has_received_frame_ =
!child_frames_.empty();
On 2016/12/06 18:15:03, boliu wrote:
> the RequestInvokeGL below has nothing to do with HR. HR should definitely be
> deleted at this point
> 
> so I don't get this either. just set has_received_frame_ to false at end of
this
> method?

Done.

[boliu, 2016-12-06 18:25:32]

lgtm

[Tobias Sargeant, 2016-12-06 18:29:42]

The CQ bit was checked by tobiasjs@chromium.org

[commit-bot: I haz the power, 2016-12-06 18:30:24]

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-12-06 19:05:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481048982042360,
"parent_rev": "ce05270699a9b693a7067481f93cb5841b232b09", "commit_rev":
"89951a5b36fcdbb3298345ae396567a4e1aad634"}

[commit-bot: I haz the power, 2016-12-06 19:05:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-06 19:07:13]

Description was changed from

==========
aw: Fix race between first frame draw and view detach causing deadlock.

If a AwContents.onDetachedFromWindow occurs at simulaneously with
RenderThreadManager::DrawGL, then the following sequence of events can
occur:

* RenderThreadManager::DrawGL creates hardware_renderer_ because a frame
  is ready to be committed to the HardwareRenderer.
* BrowserViewRenderer::OnDetachedFromWindow calls the uncommitted frame
  to be returned to the child compositor.
* RenderThreadManager tries to commit the frame that is just returned,
  leaving it in a state where neither the RenderThreadManager, nor its
  HardwareRenderer has a frame.

Then subsequently when deleting native objects, DeleteHardwareRendererOnUI
does no work, meaning that hardware_renderer_ remains set at the time
that the RenderThreadManager destructor is called, leading to a DCHECK,
or deadlock.

In order to avoid this, we consider RenderThreadManager to have a
frame (and thus need to run the synchronous RenderThread cleanup in
DeleteHardwareRendererOnUI) if it has ever received a frame (stored in
has_received_frame_), rather than if it or hardware_renderer_ currently
has a frame.

In order that BrowserViewRenderer receives the right signal post
deletion of hardware_renderer_, we reset has_received_frame_ when
RenderThreadManager deletes hardware_renderer_, if RenderThreadManager
does not currently have a frame to commit to a new HardwareRenderer
instance.

BUG=668692
==========

to

==========
aw: Fix race between first frame draw and view detach causing deadlock.

If a AwContents.onDetachedFromWindow occurs at simulaneously with
RenderThreadManager::DrawGL, then the following sequence of events can
occur:

* RenderThreadManager::DrawGL creates hardware_renderer_ because a frame
  is ready to be committed to the HardwareRenderer.
* BrowserViewRenderer::OnDetachedFromWindow calls the uncommitted frame
  to be returned to the child compositor.
* RenderThreadManager tries to commit the frame that is just returned,
  leaving it in a state where neither the RenderThreadManager, nor its
  HardwareRenderer has a frame.

Then subsequently when deleting native objects, DeleteHardwareRendererOnUI
does no work, meaning that hardware_renderer_ remains set at the time
that the RenderThreadManager destructor is called, leading to a DCHECK,
or deadlock.

In order to avoid this, we consider RenderThreadManager to have a
frame (and thus need to run the synchronous RenderThread cleanup in
DeleteHardwareRendererOnUI) if it has ever received a frame (stored in
has_received_frame_), rather than if it or hardware_renderer_ currently
has a frame.

In order that BrowserViewRenderer receives the right signal post
deletion of hardware_renderer_, we reset has_received_frame_ when
RenderThreadManager deletes hardware_renderer_, if RenderThreadManager
does not currently have a frame to commit to a new HardwareRenderer
instance.

BUG=668692

Committed: https://crrev.com/ff08fc807243b2317ef1c8b42f3f423496d12a76
Cr-Commit-Position: refs/heads/master@{#436672}
==========

[commit-bot: I haz the power, 2016-12-06 19:07:14]

Patchset 3 (id:??) landed as
https://crrev.com/ff08fc807243b2317ef1c8b42f3f423496d12a76
Cr-Commit-Position: refs/heads/master@{#436672}