Correctly destroy returned front buffer after resize

Correctly destroy returned front buffer after resize

When returning a front buffer with ReturnFrontBuffer, if the front buffer was
resized in the mean time, we were considering the returned mailbox as lost, and
never freeing it, causing a large GPU memory leak.
This patch fixes it.

BUG=692074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2779873002
Cr-Commit-Position: refs/heads/master@{#459969}
Committed: https://chromium.googlesource.com/chromium/src/+/15176a8d134862de21fe2de9f9fd1d7390ac9003

[piman, 2017-03-28 00:17:21]

Description was changed from

==========
Correctly destroy returned front buffer after resize

When returning a front buffer with ReturnFrontBuffer, if the front buffer was
resized in the mean time, we were considering the returned mailbox as lost, and
never freeing it, causing a large GPU memory leak.
This patch fixes it.

BUG=692074
==========

to

==========
Correctly destroy returned front buffer after resize

When returning a front buffer with ReturnFrontBuffer, if the front buffer was
resized in the mean time, we were considering the returned mailbox as lost, and
never freeing it, causing a large GPU memory leak.
This patch fixes it.

BUG=692074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2017-03-28 00:17:25]

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

[commit-bot: I haz the power, 2017-03-28 00:17:46]

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

[piman, 2017-03-28 00:19:18]

piman@chromium.org changed reviewers:
+ erikchen@chromium.org

[piman, 2017-03-28 00:19:18]

FYI, I wouldn't be surprised if this was related to your "10GB of maybe
IOSurfaces leaked" issue.

[erikchen, 2017-03-28 00:45:45]

On 2017/03/28 00:19:18, piman wrote:
> FYI, I wouldn't be surprised if this was related to your "10GB of maybe
> IOSurfaces leaked" issue.

lgtm.

good catch. What can we do to add tests to catch this type of problem? Can we
add some state to textures so that if we don't destroy [when we know we could
have destroyed], we throw an error? [or dcheck or uma stat]

[piman, 2017-03-28 00:52:09]

On 2017/03/28 00:45:45, erikchen wrote:
> On 2017/03/28 00:19:18, piman wrote:
> > FYI, I wouldn't be surprised if this was related to your "10GB of maybe
> > IOSurfaces leaked" issue.
> 
> lgtm.
> 
> good catch. What can we do to add tests to catch this type of problem?

I started looking into tests, but we need more infrastructure because regular
unit tests don't create offscreen contexts (that are needed to exercise this
API), and integration tests (GLManager) don't provide visibility into the
context calls (i.e. we can't differentiate calling glDeleteTextures vs not), so
I wanted to send this first (we may want to merge).

> Can we
> add some state to textures so that if we don't destroy [when we know we could
> have destroyed], we throw an error? [or dcheck or uma stat]

Well, it's hard to really define this at a low level, because we were explicitly
calling the API to leak the texture, which is the right thing to do in some
cases. At a high level we do track GPU memory, and it clearly showed the leak
(e.g. task manager), so we may be able to write a directed integration test for
this - I'll think more about it. Again, needs a bunch of infrastructure, so not
a low hanging fruit.

[piman, 2017-03-28 00:52:22]

The CQ bit was unchecked by piman@chromium.org

[piman, 2017-03-28 00:52:28]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2017-03-28 00:52:51]

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, 2017-03-28 01:27:24]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1490662348310340, "parent_rev":
"70eb969b7713def48c0a3c865ea3dd0ba8ee1169", "commit_rev":
"15176a8d134862de21fe2de9f9fd1d7390ac9003"}

[commit-bot: I haz the power, 2017-03-28 01:28:09]

Description was changed from

==========
Correctly destroy returned front buffer after resize

When returning a front buffer with ReturnFrontBuffer, if the front buffer was
resized in the mean time, we were considering the returned mailbox as lost, and
never freeing it, causing a large GPU memory leak.
This patch fixes it.

BUG=692074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Correctly destroy returned front buffer after resize

When returning a front buffer with ReturnFrontBuffer, if the front buffer was
resized in the mean time, we were considering the returned mailbox as lost, and
never freeing it, causing a large GPU memory leak.
This patch fixes it.

BUG=692074
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2779873002
Cr-Commit-Position: refs/heads/master@{#459969}
Committed:
https://chromium.googlesource.com/chromium/src/+/15176a8d134862de21fe2de9f9fd...
==========

[commit-bot: I haz the power, 2017-03-28 01:28:10]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/15176a8d134862de21fe2de9f9fd...