Fixes failing instrumentation tests on Nakasi

Fixes failing instrumentation tests on Nakasi

BUG=593958, 593918, 594407, 592408
Committed: https://crrev.com/83c640830c2f4df4b03ae18609a9dc7bd16b15c3
Cr-Commit-Position: refs/heads/master@{#382495}

[Jinsuk Kim, 2016-03-14 02:13:13]

jinsukkim@chromium.org changed reviewers:
+ sievers@chromium.org

[Jinsuk Kim, 2016-03-14 22:54:23]

jinsukkim@chromium.org changed reviewers:
+ tedchoc@chromium.org

[no sievers, 2016-03-16 00:16:12]

see comments in bug

https://codereview.chromium.org/1797663002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1797663002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:906:
compositor->AttachLayerForReadback(layer_);
Hmm that looks dangerous to attach the layer (potentially) twice.

[Jinsuk Kim, 2016-03-16 06:01:23]

Description was changed from

==========
Fixes failing instrumentation tests on Nakasi

BUG=593958
==========

to

==========
Fixes failing instrumentation tests on Nakasi

BUG=593958, 593918, 594407
==========

[Jinsuk Kim, 2016-03-16 06:01:23]

jinsukkim@chromium.org changed reviewers:
- tedchoc@chromium.org

[Jinsuk Kim, 2016-03-16 06:03:12]

Calling RWH::ViewDestroyed() fixed
BindingManagerIntegrationTest#testRestoreSharedRenderer(). Null-checking in
RWVHA::UnlockCompositingSurface() was to fix the other test
UndoTabModelTest#testOutOfOrder1().

https://codereview.chromium.org/1797663002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1797663002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_android.cc:906:
compositor->AttachLayerForReadback(layer_);
On 2016/03/16 00:16:12, sievers (slow) wrote:
> Hmm that looks dangerous to attach the layer (potentially) twice.

Changed to the comment in the bug.

[no sievers, 2016-03-18 00:42:15]

https://codereview.chromium.org/1797663002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1797663002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:557:
!layer_.get())
I'm worried that this can mess up our |locks_on_frame_count_| and we end up
creating a new |layer_| and the lock count is now nonzero. It's also weird that
there is an early-out here for 'locks_on_frame_count_ == 0' which explicitly
seems to allow unbalanced calls.

Is the problem maybe this sequence?
void RenderWidgetHostViewAndroid::Destroy() {
  RemoveLayers();
  SetContentViewCore(NULL); -> can call ReleaseLocksOnSurface()

Note that there is another call to RemoveLayers() at the top of
SetContentViewCore().

It think it makes sense to ensure that we release locks on the 'compositing
surface' (which really is the layer) before we delete the layer.

[Jinsuk Kim, 2016-03-21 00:13:16]

On 2016/03/18 00:42:15, sievers wrote:
>
https://codereview.chromium.org/1797663002/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/1797663002/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_android.cc:557:
> !layer_.get())
> I'm worried that this can mess up our |locks_on_frame_count_| and we end up
> creating a new |layer_| and the lock count is now nonzero. It's also weird
that
> there is an early-out here for 'locks_on_frame_count_ == 0' which explicitly
> seems to allow unbalanced calls.
> 
> Is the problem maybe this sequence?
> void RenderWidgetHostViewAndroid::Destroy() {
>   RemoveLayers();
>   SetContentViewCore(NULL); -> can call ReleaseLocksOnSurface()
> 
> Note that there is another call to RemoveLayers() at the top of
> SetContentViewCore().
> 
> It think it makes sense to ensure that we release locks on the 'compositing
> surface' (which really is the layer) before we delete the layer.

RemoveLayers() won't be necessary because it is also called in
SetContentViewCore(NULL) as you said, assuming |content_view_core_| is not
already NULL (I think it will be true).

Checked the flow after RWVH::Destroy(), and found that it was already on the way
to releasing locks when this happened. SetContentViewCore(NULL) ->
ReleaseLocksOnSurface() -> UnlockCompositingSurface() ->
FrameEvictor::UnlockFrame() ->.. -> EvictDelegatedFrame() ->
DestroyDelegatedContent() where |layer_| gets nulled out, which triggers the
above callback mechanism that calls ReleaseLocksOnSurface() again, which fails
because |layer_| is already deleted.

To stop the flow before it reaches the DCHECK(HasValidFrame()), checking
|layers_.get()| may be okay..? because it would not happen in other
circumstances but only in this destruction process. The other way I can think of
is simply swap the following two lines in UnlockCompositingSurface():


frame_evictor_->UnlockFrame();
locks_on_frame_count_--;

->

locks_on_frame_count_--;
frame_evictor_->UnlockFrame();

This decreases the lock count before the flow above gets triggered - so the
first if() can detect the frame count being zero and return immediately.

Let me check if there's other (hopefully) better way. Feel free to make
suggestions.

[Jinsuk Kim, 2016-03-21 00:40:47]

On 2016/03/21 00:13:16, Jinsuk wrote:
> On 2016/03/18 00:42:15, sievers wrote:
> >
>
https://codereview.chromium.org/1797663002/diff/20001/content/browser/rendere...
> > File content/browser/renderer_host/render_widget_host_view_android.cc
(right):
> > 
> >
>
https://codereview.chromium.org/1797663002/diff/20001/content/browser/rendere...
> > content/browser/renderer_host/render_widget_host_view_android.cc:557:
> > !layer_.get())
> > I'm worried that this can mess up our |locks_on_frame_count_| and we end up
> > creating a new |layer_| and the lock count is now nonzero. It's also weird
> that
> > there is an early-out here for 'locks_on_frame_count_ == 0' which explicitly
> > seems to allow unbalanced calls.
> > 
> > Is the problem maybe this sequence?
> > void RenderWidgetHostViewAndroid::Destroy() {
> >   RemoveLayers();
> >   SetContentViewCore(NULL); -> can call ReleaseLocksOnSurface()
> > 
> > Note that there is another call to RemoveLayers() at the top of
> > SetContentViewCore().
> > 
> > It think it makes sense to ensure that we release locks on the 'compositing
> > surface' (which really is the layer) before we delete the layer.
> 
> RemoveLayers() won't be necessary because it is also called in
> SetContentViewCore(NULL) as you said, assuming |content_view_core_| is not
> already NULL (I think it will be true).
> 
> Checked the flow after RWVH::Destroy(), and found that it was already on the
way
> to releasing locks when this happened. SetContentViewCore(NULL) ->
> ReleaseLocksOnSurface() -> UnlockCompositingSurface() ->
> FrameEvictor::UnlockFrame() ->.. -> EvictDelegatedFrame() ->
> DestroyDelegatedContent() where |layer_| gets nulled out, which triggers the
> above callback mechanism that calls ReleaseLocksOnSurface() again, which fails
> because |layer_| is already deleted.
> 
> To stop the flow before it reaches the DCHECK(HasValidFrame()), checking
> |layers_.get()| may be okay..? because it would not happen in other
> circumstances but only in this destruction process. The other way I can think
of
> is simply swap the following two lines in UnlockCompositingSurface():
> 
> 
> frame_evictor_->UnlockFrame();
> locks_on_frame_count_--;
> 
> ->
> 
> locks_on_frame_count_--;
> frame_evictor_->UnlockFrame();
> 
> This decreases the lock count before the flow above gets triggered - so the
> first if() can detect the frame count being zero and return immediately.
> 
> Let me check if there's other (hopefully) better way. Feel free to make
> suggestions.

Never mind what I said about |RemoveLayers()|... it is always called in
Destroy(), regardless of the value of |content_view_core_|.

And what I meant to say in:
"above callback mechanism that calls ReleaseLocksOnSurface() again, ... "
is actually:
above callback mechanism that calls UnlockCompositingSurface() again, ... "

[Jinsuk Kim, 2016-03-21 04:52:36]

https://codereview.chromium.org/1797663002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1797663002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:557:
!layer_.get())
On 2016/03/18 00:42:15, sievers wrote:
> I'm worried that this can mess up our |locks_on_frame_count_| and we end up
> creating a new |layer_| and the lock count is now nonzero. It's also weird
that
> there is an early-out here for 'locks_on_frame_count_ == 0' which explicitly
> seems to allow unbalanced calls.
> 
> Is the problem maybe this sequence?
> void RenderWidgetHostViewAndroid::Destroy() {
>   RemoveLayers();
>   SetContentViewCore(NULL); -> can call ReleaseLocksOnSurface()
> 
> Note that there is another call to RemoveLayers() at the top of
> SetContentViewCore().
> 
> It think it makes sense to ensure that we release locks on the 'compositing
> surface' (which really is the layer) before we delete the layer.

locks_on_frame_count_ == 0 can be removed and replaced with DCHECK_EQ as done in
ReleaseLocksOnSurface(). For that, called frame_evictor_->DiscardedFrame()
before DestroyDelegatedContent() (please see  EvictDelegatedFrame()) to have
frame_evictor_->HasFrame() return false.

[no sievers, 2016-03-21 21:42:44]

lgtm, that's good to fix the issues.

I'm wondering now though - but maybe we can look at that separately in a new bug
or patch - if the existing logic still has corner case issues, for example when
we call DestroyDelegatedContent() from CheckOutputSurfaceChanged() (renderer
context was lost). Maybe the more robust behavior would be that dropping the
|layer_| simply always implies resetting all locks, and then we can just early
out in Unlock() if there is no |layer_| like you originally suggested.

https://codereview.chromium.org/1797663002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1797663002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1504: void
RenderWidgetHostViewAndroid::EvictDelegatedFrame() {
you can DCHECK_EQ(locks_on_frame_count_, 0u) here also.

[Jinsuk Kim, 2016-03-21 22:28:20]

Thanks for reviewing. Let me do that in the next patch.

https://codereview.chromium.org/1797663002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1797663002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1504: void
RenderWidgetHostViewAndroid::EvictDelegatedFrame() {
On 2016/03/21 21:42:44, sievers wrote:
> you can DCHECK_EQ(locks_on_frame_count_, 0u) here also.

Done.

[Jinsuk Kim, 2016-03-21 22:30:39]

Description was changed from

==========
Fixes failing instrumentation tests on Nakasi

BUG=593958, 593918, 594407
==========

to

==========
Fixes failing instrumentation tests on Nakasi

BUG=593958, 593918, 594407, 592408
==========

[Jinsuk Kim, 2016-03-22 02:37:42]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2016-03-22 02:37:42]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1797663002/#ps60001
(title: "addressed a comment")

[commit-bot: I haz the power, 2016-03-22 02:37:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1797663002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1797663002/60001

[commit-bot: I haz the power, 2016-03-22 03:20:39]

Description was changed from

==========
Fixes failing instrumentation tests on Nakasi

BUG=593958, 593918, 594407, 592408
==========

to

==========
Fixes failing instrumentation tests on Nakasi

BUG=593958, 593918, 594407, 592408
==========

[commit-bot: I haz the power, 2016-03-22 03:20:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-22 03:25:54]

Description was changed from

==========
Fixes failing instrumentation tests on Nakasi

BUG=593958, 593918, 594407, 592408
==========

to

==========
Fixes failing instrumentation tests on Nakasi

BUG=593958, 593918, 594407, 592408

Committed: https://crrev.com/83c640830c2f4df4b03ae18609a9dc7bd16b15c3
Cr-Commit-Position: refs/heads/master@{#382495}
==========

[commit-bot: I haz the power, 2016-03-22 03:25:55]

Patchset 4 (id:??) landed as
https://crrev.com/83c640830c2f4df4b03ae18609a9dc7bd16b15c3
Cr-Commit-Position: refs/heads/master@{#382495}

[Jinsuk Kim, 2016-03-23 02:00:44]

On 2016/03/22 03:25:55, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/83c640830c2f4df4b03ae18609a9dc7bd16b15c3
> Cr-Commit-Position: refs/heads/master@{#382495}

I suppose this needs to be cherry-picked to M50 branch. Daniel, could you do me
a favor of doing it? I don't have the permission for that. I think I can soon
become a a committer and handle it myself but not now...

[Jinsuk Kim, 2016-03-23 02:23:46]

On 2016/03/23 02:00:44, Jinsuk wrote:
> On 2016/03/22 03:25:55, commit-bot: I haz the power wrote:
> > Patchset 4 (id:??) landed as
> > https://crrev.com/83c640830c2f4df4b03ae18609a9dc7bd16b15c3
> > Cr-Commit-Position: refs/heads/master@{#382495}
> 
> I suppose this needs to be cherry-picked to M50 branch. Daniel, could you do
me
> a favor of doing it? I don't have the permission for that. I think I can soon
> become a a committer and handle it myself but not now...

Set Merge-Request-50 on crbug.com/592408