Show that ResizeLock is racey with renderer frames in flight.

Show that ResizeLock is racey with renderer frames in flight.

R=piman@chromium.org
BUG=

[danakj, 2017-03-29 23:17:03]

Hello,

wutao@ found that resize lock was flaky even when we got it working, and we
learnt that it was because of NO_PENDING_COMMIT. Here's a patch with a unit test
that fails, but passes if we stop setting NO_PENDING_COMMIT.

I looked that git history and this is an oooold thing. I believe it's intentions
are that if we receive a frame from the renderer and lock, it wants to ensure
that we don't lock forever and that another frame is in flight. The logic I
think goes that since we just received a frame from the renderer and didn't
commit, the renderer will be blocked until we commit it, so we shouldn't lock.

The side effect is that if you insert a copy request and resize, but the
renderer happened to have sent a frame at the same time, then we won't honor the
lock and we'll perform the copy request with the old renderer sized content,
which looks bad.

It'd be helpful wutao, if you can confirm that when not using NO_PENDING_COMMIT
we still get the raciness anyways, which I think would confirm the renderer is
blocking until the timeout.

One possible idea to get rid of NO_PENDING_COMMIT would be to ack the frame as
if ShouldSkipFrame happened. The awkwardness is that we *did* submit the frame
to the display compositor already, so it would draw and double ack which we'd
have to prevent.

Thoughts?

[wutao, 2017-03-30 08:33:37]

On 2017/03/29 23:17:03, danakj wrote:
> Hello,
> 
> wutao@ found that resize lock was flaky even when we got it working, and we
> learnt that it was because of NO_PENDING_COMMIT. Here's a patch with a unit
test
> that fails, but passes if we stop setting NO_PENDING_COMMIT.
> 
> I looked that git history and this is an oooold thing. I believe it's
intentions
> are that if we receive a frame from the renderer and lock, it wants to ensure
> that we don't lock forever and that another frame is in flight. The logic I
> think goes that since we just received a frame from the renderer and didn't
> commit, the renderer will be blocked until we commit it, so we shouldn't lock.
> 
> The side effect is that if you insert a copy request and resize, but the
> renderer happened to have sent a frame at the same time, then we won't honor
the
> lock and we'll perform the copy request with the old renderer sized content,
> which looks bad.
> 
> It'd be helpful wutao, if you can confirm that when not using
NO_PENDING_COMMIT
> we still get the raciness anyways, which I think would confirm the renderer is
> blocking until the timeout.
> 
> One possible idea to get rid of NO_PENDING_COMMIT would be to ack the frame as
> if ShouldSkipFrame happened. The awkwardness is that we *did* submit the frame
> to the display compositor already, so it would draw and double ack which we'd
> have to prevent.
> 
> Thoughts?

I still see wrong size render not using NO_PENDING_COMMIT. 
Cannot lock immediately due to the defer_compositor_lock is true.

After commented this line out, did not see wrong side render in many tries:
can_lock_compositor_ = NO_PENDING_RENDERER_FRAME;
https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...

Is there any logic change for NO_PENDING_RENDERER_FRAME in the past one month?

[danakj, 2017-03-30 14:49:05]

On Thu, Mar 30, 2017 at 4:33 AM, <wutao@chromium.org> wrote:

> On 2017/03/29 23:17:03, danakj wrote:
> > Hello,
> >
> > wutao@ found that resize lock was flaky even when we got it working,
> and we
> > learnt that it was because of NO_PENDING_COMMIT. Here's a patch with a
> unit
> test
> > that fails, but passes if we stop setting NO_PENDING_COMMIT.
> >
> > I looked that git history and this is an oooold thing. I believe it's
> intentions
> > are that if we receive a frame from the renderer and lock, it wants to
> ensure
> > that we don't lock forever and that another frame is in flight. The
> logic I
> > think goes that since we just received a frame from the renderer and
> didn't
> > commit, the renderer will be blocked until we commit it, so we shouldn't
> lock.
> >
> > The side effect is that if you insert a copy request and resize, but the
> > renderer happened to have sent a frame at the same time, then we won't
> honor
> the
> > lock and we'll perform the copy request with the old renderer sized
> content,
> > which looks bad.
> >
> > It'd be helpful wutao, if you can confirm that when not using
> NO_PENDING_COMMIT
> > we still get the raciness anyways, which I think would confirm the
> renderer is
> > blocking until the timeout.
> >
> > One possible idea to get rid of NO_PENDING_COMMIT would be to ack the
> frame as
> > if ShouldSkipFrame happened. The awkwardness is that we *did* submit the
> frame
> > to the display compositor already, so it would draw and double ack which
> we'd
> > have to prevent.
> >
> > Thoughts?
>
> I still see wrong size render not using NO_PENDING_COMMIT.
> Cannot lock immediately due to the defer_compositor_lock is true.
>
> After commented this line out, did not see wrong side render in many tries:
> can_lock_compositor_ = NO_PENDING_RENDERER_FRAME;
> https://cs.chromium.org/chromium/src/content/browser/
> renderer_host/delegated_frame_host.cc?type=cs&q=
> OnCompositingLockStateChanged&l=739
>
> Is there any logic change for NO_PENDING_RENDERER_FRAME in the past one
> month?
>

I don't think it has changed in many years, at least intentionally.

NO_PENDING_RENDERER_FRAME has the following comment when it is set:
  // A compositor lock that is part of a resize lock timed out. We

  // should display a renderer frame.


This suggests its intent is to ensure forward progress when the lock times
out, in case a resize would happen just as the lock times out, preventing
frames indefinitely. How this works is when the lock times out,
OnCompositoringLockStateChanged will fire, which will change us to
NO_PENDING_RENDERER_FRAME. At that point we will accept the next frame sent
from the renderer regardless of size and allow it to gutter. Whe we accept
the new frame we move from NO_PENDING_RENDERER_FRAME to NO_PENDING_COMMIT,
then once the commit happens that will try to lock again, which was
explicitly built to fail on the 2nd lock attempt. That leaves us in
NO_PENDING_COMMIT so we continue to accept frames of any size.

The other case is when CheckResizeLock() happens, we unlock the compositor
but don't release the resize lock or change from YES_DID_LOCK. When
OnCompositingLockStateChanged happens,it sends us into
NO_PENDING_RENDERER_FRAME, but then we'd move to NO_PENDING_COMMIT at the
end of processing the frame. At that point we have a resize lock which we
can't re-lock leaving us in NO_PENDING_COMMIT. However since this was a
non-guttering frame, as long as the size of the view hasn't changed in the
meantime, once the commit happens, it would match size so we'd drop the
resize lock. If the view size in the meantime, we'd keep the resize lock
and stay in NO_PENDING_COMMIT, accepting frames that gutter.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wutao, 2017-03-30 16:37:43]

On 2017/03/30 14:49:05, danakj wrote:
> On Thu, Mar 30, 2017 at 4:33 AM, <mailto:wutao@chromium.org> wrote:
> 
> > On 2017/03/29 23:17:03, danakj wrote:
> > > Hello,
> > >
> > > wutao@ found that resize lock was flaky even when we got it working,
> > and we
> > > learnt that it was because of NO_PENDING_COMMIT. Here's a patch with a
> > unit
> > test
> > > that fails, but passes if we stop setting NO_PENDING_COMMIT.
> > >
> > > I looked that git history and this is an oooold thing. I believe it's
> > intentions
> > > are that if we receive a frame from the renderer and lock, it wants to
> > ensure
> > > that we don't lock forever and that another frame is in flight. The
> > logic I
> > > think goes that since we just received a frame from the renderer and
> > didn't
> > > commit, the renderer will be blocked until we commit it, so we shouldn't
> > lock.
> > >
> > > The side effect is that if you insert a copy request and resize, but the
> > > renderer happened to have sent a frame at the same time, then we won't
> > honor
> > the
> > > lock and we'll perform the copy request with the old renderer sized
> > content,
> > > which looks bad.
> > >
> > > It'd be helpful wutao, if you can confirm that when not using
> > NO_PENDING_COMMIT
> > > we still get the raciness anyways, which I think would confirm the
> > renderer is
> > > blocking until the timeout.
> > >
> > > One possible idea to get rid of NO_PENDING_COMMIT would be to ack the
> > frame as
> > > if ShouldSkipFrame happened. The awkwardness is that we *did* submit the
> > frame
> > > to the display compositor already, so it would draw and double ack which
> > we'd
> > > have to prevent.
> > >
> > > Thoughts?
> >
> > I still see wrong size render not using NO_PENDING_COMMIT.
> > Cannot lock immediately due to the defer_compositor_lock is true.
> >
> > After commented this line out, did not see wrong side render in many tries:
> > can_lock_compositor_ = NO_PENDING_RENDERER_FRAME;
> > https://cs.chromium.org/chromium/src/content/browser/
> > renderer_host/delegated_frame_host.cc?type=cs&q=
> > OnCompositingLockStateChanged&l=739
> >
> > Is there any logic change for NO_PENDING_RENDERER_FRAME in the past one
> > month?
> >
> 
> I don't think it has changed in many years, at least intentionally.
> 
> NO_PENDING_RENDERER_FRAME has the following comment when it is set:
>   // A compositor lock that is part of a resize lock timed out. We
> 
>   // should display a renderer frame.
> 
> 
> This suggests its intent is to ensure forward progress when the lock times
> out, in case a resize would happen just as the lock times out, preventing
> frames indefinitely. How this works is when the lock times out,
> OnCompositoringLockStateChanged will fire, which will change us to
> NO_PENDING_RENDERER_FRAME. At that point we will accept the next frame sent
> from the renderer regardless of size and allow it to gutter. Whe we accept
> the new frame we move from NO_PENDING_RENDERER_FRAME to NO_PENDING_COMMIT,
> then once the commit happens that will try to lock again, which was
> explicitly built to fail on the 2nd lock attempt. That leaves us in
> NO_PENDING_COMMIT so we continue to accept frames of any size.
> 
> The other case is when CheckResizeLock() happens, we unlock the compositor
> but don't release the resize lock or change from YES_DID_LOCK. When
> OnCompositingLockStateChanged happens,it sends us into
> NO_PENDING_RENDERER_FRAME, but then we'd move to NO_PENDING_COMMIT at the
> end of processing the frame. At that point we have a resize lock which we
> can't re-lock leaving us in NO_PENDING_COMMIT. However since this was a
> non-guttering frame, as long as the size of the view hasn't changed in the
> meantime, once the commit happens, it would match size so we'd drop the
> resize lock. If the view size in the meantime, we'd keep the resize lock
> and stay in NO_PENDING_COMMIT, accepting frames that gutter.
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I realized in my local branch if I make change at this line [713]: if
(can_lock_compositor_ == NO_PENDING_COMMIT) to if (can_lock_compositor_ ==
NO_PENDING_RENDERER_FRAME)
, then I do not need to comment out this at line [739]: can_lock_compositor_ =
NO_PENDING_RENDERER_FRAME;

Because we are not using NO_PENDING_COMMIT, so the old 713 line will always
false. So can_lock_compositor_ will be NO_PENDING_RENDERER_FRAME forever after
it was set?

[713]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...
[739]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...

[danakj, 2017-03-30 17:38:23]

On Thu, Mar 30, 2017 at 12:37 PM, <wutao@chromium.org> wrote:

> On 2017/03/30 14:49:05, danakj wrote:
>
> > On Thu, Mar 30, 2017 at 4:33 AM, <mailto:wutao@chromium.org> wrote:
> >
> > > On 2017/03/29 23:17:03, danakj wrote:
> > > > Hello,
> > > >
> > > > wutao@ found that resize lock was flaky even when we got it working,
> > > and we
> > > > learnt that it was because of NO_PENDING_COMMIT. Here's a patch with
> a
> > > unit
> > > test
> > > > that fails, but passes if we stop setting NO_PENDING_COMMIT.
> > > >
> > > > I looked that git history and this is an oooold thing. I believe it's
> > > intentions
> > > > are that if we receive a frame from the renderer and lock, it wants
> to
> > > ensure
> > > > that we don't lock forever and that another frame is in flight. The
> > > logic I
> > > > think goes that since we just received a frame from the renderer and
> > > didn't
> > > > commit, the renderer will be blocked until we commit it, so we
> shouldn't
> > > lock.
> > > >
> > > > The side effect is that if you insert a copy request and resize, but
> the
> > > > renderer happened to have sent a frame at the same time, then we
> won't
> > > honor
> > > the
> > > > lock and we'll perform the copy request with the old renderer sized
> > > content,
> > > > which looks bad.
> > > >
> > > > It'd be helpful wutao, if you can confirm that when not using
> > > NO_PENDING_COMMIT
> > > > we still get the raciness anyways, which I think would confirm the
> > > renderer is
> > > > blocking until the timeout.
> > > >
> > > > One possible idea to get rid of NO_PENDING_COMMIT would be to ack the
> > > frame as
> > > > if ShouldSkipFrame happened. The awkwardness is that we *did* submit
> the
> > > frame
> > > > to the display compositor already, so it would draw and double ack
> which
> > > we'd
> > > > have to prevent.
> > > >
> > > > Thoughts?
> > >
> > > I still see wrong size render not using NO_PENDING_COMMIT.
> > > Cannot lock immediately due to the defer_compositor_lock is true.
> > >
> > > After commented this line out, did not see wrong side render in many
> tries:
> > > can_lock_compositor_ = NO_PENDING_RENDERER_FRAME;
> > > https://cs.chromium.org/chromium/src/content/browser/
> > > renderer_host/delegated_frame_host.cc?type=cs&q=
> > > OnCompositingLockStateChanged&l=739
> > >
> > > Is there any logic change for NO_PENDING_RENDERER_FRAME in the past one
> > > month?
> > >
> >
> > I don't think it has changed in many years, at least intentionally.
> >
> > NO_PENDING_RENDERER_FRAME has the following comment when it is set:
> > // A compositor lock that is part of a resize lock timed out. We
> >
> > // should display a renderer frame.
> >
> >
> > This suggests its intent is to ensure forward progress when the lock
> times
> > out, in case a resize would happen just as the lock times out, preventing
> > frames indefinitely. How this works is when the lock times out,
> > OnCompositoringLockStateChanged will fire, which will change us to
> > NO_PENDING_RENDERER_FRAME. At that point we will accept the next frame
> sent
> > from the renderer regardless of size and allow it to gutter. Whe we
> accept
> > the new frame we move from NO_PENDING_RENDERER_FRAME to
> NO_PENDING_COMMIT,
> > then once the commit happens that will try to lock again, which was
> > explicitly built to fail on the 2nd lock attempt. That leaves us in
> > NO_PENDING_COMMIT so we continue to accept frames of any size.
> >
> > The other case is when CheckResizeLock() happens, we unlock the
> compositor
> > but don't release the resize lock or change from YES_DID_LOCK. When
> > OnCompositingLockStateChanged happens,it sends us into
> > NO_PENDING_RENDERER_FRAME, but then we'd move to NO_PENDING_COMMIT at the
> > end of processing the frame. At that point we have a resize lock which we
> > can't re-lock leaving us in NO_PENDING_COMMIT. However since this was a
> > non-guttering frame, as long as the size of the view hasn't changed in
> the
> > meantime, once the commit happens, it would match size so we'd drop the
> > resize lock. If the view size in the meantime, we'd keep the resize lock
> > and stay in NO_PENDING_COMMIT, accepting frames that gutter.
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
> I realized in my local branch if I make change at this line [713]: if
> (can_lock_compositor_ == NO_PENDING_COMMIT) to if (can_lock_compositor_ ==
> NO_PENDING_RENDERER_FRAME)
> , then I do not need to comment out this at line [739]:
> can_lock_compositor_ =
> NO_PENDING_RENDERER_FRAME;
>
> Because we are not using NO_PENDING_COMMIT, so the old 713 line will always
> false. So can_lock_compositor_ will be NO_PENDING_RENDERER_FRAME forever
> after
> it was set?
>

Ya I think you're right, if you stop setting NO_PENDING_COMMIT it loses the
ability to leave NO_PENDING_RENDERER_FRAME without that change. So if I
understand, if you:

- Stop setting NO_PENDING_COMMIT.
- Change DelegatedFrameHost
<https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...>
::OnCompositingDidCommit
<https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...>
to
check for NO_PENDING_RENDERER_FRAME instead.

Then you stop seeing the lock fail at all? I would expect it to still race
sometimes, but probably less (since it would recover without flaking most
times).


>
> [713]
> https://cs.chromium.org/chromium/src/content/browser/
> renderer_host/delegated_frame_host.cc?type=cs&l=713
> [739]
> https://cs.chromium.org/chromium/src/content/browser/
> renderer_host/delegated_frame_host.cc?type=cs&l=739
>
> https://codereview.chromium.org/2782063006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-03-30 18:00:02]

I'm converting the RWHVA unittests into more independent DelegatedFrameHost
tests that rely on less things so we can test more scenarios. In the
process I hooked it up to an actual compositor instead of the test
pretending to do commits. This pointed out to me that NO_PENDING_COMMIT is
more horrible than I thought.

Because the UI compositor used to be the thing that presented renderer
frames, we wait for a commit after submitting a renderer frame. But if
there's no resize involved that submit doesn't involve the UI compositor at
all, so no commit happens, so we stay in NO_PENDING_COMMIT even tho no
commit is coming or needed. That prevents the compositor from locking once
a resize does happen,until after the first commit of resize I guess.

On Thu, Mar 30, 2017 at 1:37 PM, <danakj+owner@chromium.org> wrote:

> On Thu, Mar 30, 2017 at 12:37 PM, <wutao@chromium.org> wrote:
>
>> On 2017/03/30 14:49:05, danakj wrote:
>>
>> > On Thu, Mar 30, 2017 at 4:33 AM, <mailto:wutao@chromium.org> wrote:
>> >
>> > > On 2017/03/29 23:17:03, danakj wrote:
>> > > > Hello,
>> > > >
>> > > > wutao@ found that resize lock was flaky even when we got it
>> working,
>> > > and we
>> > > > learnt that it was because of NO_PENDING_COMMIT. Here's a patch
>> with a
>> > > unit
>> > > test
>> > > > that fails, but passes if we stop setting NO_PENDING_COMMIT.
>> > > >
>> > > > I looked that git history and this is an oooold thing. I believe
>> it's
>> > > intentions
>> > > > are that if we receive a frame from the renderer and lock, it wants
>> to
>> > > ensure
>> > > > that we don't lock forever and that another frame is in flight. The
>> > > logic I
>> > > > think goes that since we just received a frame from the renderer and
>> > > didn't
>> > > > commit, the renderer will be blocked until we commit it, so we
>> shouldn't
>> > > lock.
>> > > >
>> > > > The side effect is that if you insert a copy request and resize,
>> but the
>> > > > renderer happened to have sent a frame at the same time, then we
>> won't
>> > > honor
>> > > the
>> > > > lock and we'll perform the copy request with the old renderer sized
>> > > content,
>> > > > which looks bad.
>> > > >
>> > > > It'd be helpful wutao, if you can confirm that when not using
>> > > NO_PENDING_COMMIT
>> > > > we still get the raciness anyways, which I think would confirm the
>> > > renderer is
>> > > > blocking until the timeout.
>> > > >
>> > > > One possible idea to get rid of NO_PENDING_COMMIT would be to ack
>> the
>> > > frame as
>> > > > if ShouldSkipFrame happened. The awkwardness is that we *did*
>> submit the
>> > > frame
>> > > > to the display compositor already, so it would draw and double ack
>> which
>> > > we'd
>> > > > have to prevent.
>> > > >
>> > > > Thoughts?
>> > >
>> > > I still see wrong size render not using NO_PENDING_COMMIT.
>> > > Cannot lock immediately due to the defer_compositor_lock is true.
>> > >
>> > > After commented this line out, did not see wrong side render in many
>> tries:
>> > > can_lock_compositor_ = NO_PENDING_RENDERER_FRAME;
>> > > https://cs.chromium.org/chromium/src/content/browser/
>> > > renderer_host/delegated_frame_host.cc?type=cs&q=
>> > > OnCompositingLockStateChanged&l=739
>> > >
>> > > Is there any logic change for NO_PENDING_RENDERER_FRAME in the past
>> one
>> > > month?
>> > >
>> >
>> > I don't think it has changed in many years, at least intentionally.
>> >
>> > NO_PENDING_RENDERER_FRAME has the following comment when it is set:
>> > // A compositor lock that is part of a resize lock timed out. We
>> >
>> > // should display a renderer frame.
>> >
>> >
>> > This suggests its intent is to ensure forward progress when the lock
>> times
>> > out, in case a resize would happen just as the lock times out,
>> preventing
>> > frames indefinitely. How this works is when the lock times out,
>> > OnCompositoringLockStateChanged will fire, which will change us to
>> > NO_PENDING_RENDERER_FRAME. At that point we will accept the next frame
>> sent
>> > from the renderer regardless of size and allow it to gutter. Whe we
>> accept
>> > the new frame we move from NO_PENDING_RENDERER_FRAME to
>> NO_PENDING_COMMIT,
>> > then once the commit happens that will try to lock again, which was
>> > explicitly built to fail on the 2nd lock attempt. That leaves us in
>> > NO_PENDING_COMMIT so we continue to accept frames of any size.
>> >
>> > The other case is when CheckResizeLock() happens, we unlock the
>> compositor
>> > but don't release the resize lock or change from YES_DID_LOCK. When
>> > OnCompositingLockStateChanged happens,it sends us into
>> > NO_PENDING_RENDERER_FRAME, but then we'd move to NO_PENDING_COMMIT at
>> the
>> > end of processing the frame. At that point we have a resize lock which
>> we
>> > can't re-lock leaving us in NO_PENDING_COMMIT. However since this was a
>> > non-guttering frame, as long as the size of the view hasn't changed in
>> the
>> > meantime, once the commit happens, it would match size so we'd drop the
>> > resize lock. If the view size in the meantime, we'd keep the resize lock
>> > and stay in NO_PENDING_COMMIT, accepting frames that gutter.
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>> Groups
>> > "Chromium-reviews" group.
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>> I realized in my local branch if I make change at this line [713]: if
>> (can_lock_compositor_ == NO_PENDING_COMMIT) to if (can_lock_compositor_ ==
>> NO_PENDING_RENDERER_FRAME)
>> , then I do not need to comment out this at line [739]:
>> can_lock_compositor_ =
>> NO_PENDING_RENDERER_FRAME;
>>
>> Because we are not using NO_PENDING_COMMIT, so the old 713 line will
>> always
>> false. So can_lock_compositor_ will be NO_PENDING_RENDERER_FRAME forever
>> after
>> it was set?
>>
>
> Ya I think you're right, if you stop setting NO_PENDING_COMMIT it loses
> the ability to leave NO_PENDING_RENDERER_FRAME without that change. So if I
> understand, if you:
>
> - Stop setting NO_PENDING_COMMIT.
> - Change DelegatedFrameHost
>
<https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...>
> ::OnCompositingDidCommit
>
<https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...>
to
> check for NO_PENDING_RENDERER_FRAME instead.
>
> Then you stop seeing the lock fail at all? I would expect it to still race
> sometimes, but probably less (since it would recover without flaking most
> times).
>
>
>>
>> [713]
>> https://cs.chromium.org/chromium/src/content/browser/rendere
>> r_host/delegated_frame_host.cc?type=cs&l=713
>> [739]
>> https://cs.chromium.org/chromium/src/content/browser/rendere
>> r_host/delegated_frame_host.cc?type=cs&l=739
>>
>> https://codereview.chromium.org/2782063006/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wutao, 2017-03-30 18:46:54]

On 2017/03/30 17:38:23, danakj wrote:

> 
> Ya I think you're right, if you stop setting NO_PENDING_COMMIT it loses the
> ability to leave NO_PENDING_RENDERER_FRAME without that change. So if I
> understand, if you:
> 
> - Stop setting NO_PENDING_COMMIT.
> - Change DelegatedFrameHost
>
<https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...>
> ::OnCompositingDidCommit
>
<https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_...>
> to
> check for NO_PENDING_RENDERER_FRAME instead.
> 
> Then you stop seeing the lock fail at all? I would expect it to still race
> sometimes, but probably less (since it would recover without flaking most
> times).

Right. That is what I did.
I do find some race case on some ChromeOS device (kevin), on the login UI ->
click "Add person", then rotation the screen. It will have wrong size rendering.

Increasing the timeout to 133ms preventing the race case (hundreds times of
trying)
There is some time causing crash on the loginUI, and Polymer Frame, when I press
the hot key to rotate the screen very fast. I will investigate if it is other
issues about the login webui.

[danakj, 2017-03-31 14:33:40]

I think https://codereview.chromium.org/2790623003/ fixes this case as well as
other issues.

[danakj, 2017-03-31 14:33:46]

Description was changed from

==========
Show that ResizeLock is racey with renderer frames in flight.

R=piman@chromium.org
BUG=
==========

to

==========
Show that ResizeLock is racey with renderer frames in flight.

R=piman@chromium.org
BUG=
==========