Remove a lame assertion from one_copy_tile_task_worker_pool.cc.

Remove a lame assertion from one_copy_tile_task_worker_pool.cc.

This assertion was crashing in practice during renderer shutdown,
when SiteDetailsBrowserTest was run under --site-per-process. The
crash occurs if the allocation fails.

There seems to be an identical assertion a couple lines below, that
is correctly guarded against a null pointer.

BUG=None
TEST=browser_tests
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5510c5b383ec6ffb22ae30a9a0fc4ead5a106a4f
Cr-Commit-Position: refs/heads/master@{#373954}

[ncarter (slow), 2016-02-05 21:42:16]

Description was changed from

==========
Remove a lame assertion from one_copy_tile_task_worker_pool.cc.

This assertion was crashing in practice during renderer shutdown,
when SiteDetailsBrowserTest was run under --site-per-process. The
crash occurs if the allocation fails.

There seems to be an identical assertion a couple lines below, that
is correctly guarded against a null pointer.

BUG=None
TEST=browser_tests
==========

to

==========
Remove a lame assertion from one_copy_tile_task_worker_pool.cc.

This assertion was crashing in practice during renderer shutdown,
when SiteDetailsBrowserTest was run under --site-per-process. The
crash occurs if the allocation fails.

There seems to be an identical assertion a couple lines below, that
is correctly guarded against a null pointer.

BUG=None
TEST=browser_tests
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ncarter (slow), 2016-02-05 22:05:28]

nick@chromium.org changed reviewers:
+ danakj@chromium.org

[ncarter (slow), 2016-02-05 22:05:29]

danakj: Please review

https://codereview.chromium.org/1674843002/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1674843002/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:356: DCHECK_EQ(1u,
gfx::NumberOfPlanesForBufferFormat(buffer->GetFormat()));
This assertion is the same as the one I'm removing, except that it doesn't crash
when AllocateGpuMemoryBuffer fails.

[danakj, 2016-02-05 22:06:36]

danakj@chromium.org changed reviewers:
+ ericrk@chromium.org

[danakj, 2016-02-05 22:06:37]

=>ericrk

[danakj, 2016-02-05 22:06:57]

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

[commit-bot: I haz the power, 2016-02-05 22:07:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1674843002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1674843002/1

[ericrk, 2016-02-05 23:30:42]

lgtm

Your change looks good to me. CCing Reveman in case he has thoughts on why we do
so much work after failing to create the GMB.

https://codereview.chromium.org/1674843002/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1674843002/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:356: DCHECK_EQ(1u,
gfx::NumberOfPlanesForBufferFormat(buffer->GetFormat()));
On 2016/02/05 22:05:28, ncarter wrote:
> This assertion is the same as the one I'm removing, except that it doesn't
crash
> when AllocateGpuMemoryBuffer fails.

I agree that the above was redundant... I'm a bit curious as to why we proceed
with as much work as we do when this operation fails - it seems like we're
pretty safe in guarding against direct failures, but we do end up issuing a
bunch of GL commands that hopefully do nothing, but it's hard to know for
sure...

Given that you're just trying to solve the simpler problem of the bad DCECK,
LGTM.

[ericrk, 2016-02-05 23:30:45]

lgtm

Your change looks good to me. CCing Reveman in case he has thoughts on why we do
so much work after failing to create the GMB.

[commit-bot: I haz the power, 2016-02-05 23:37:18]

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

[commit-bot: I haz the power, 2016-02-05 23:37:21]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-02-05 23:38:23]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-02-05 23:41:00]

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

[commit-bot: I haz the power, 2016-02-05 23:51:45]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-02-05 23:53:54]

Description was changed from

==========
Remove a lame assertion from one_copy_tile_task_worker_pool.cc.

This assertion was crashing in practice during renderer shutdown,
when SiteDetailsBrowserTest was run under --site-per-process. The
crash occurs if the allocation fails.

There seems to be an identical assertion a couple lines below, that
is correctly guarded against a null pointer.

BUG=None
TEST=browser_tests
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Remove a lame assertion from one_copy_tile_task_worker_pool.cc.

This assertion was crashing in practice during renderer shutdown,
when SiteDetailsBrowserTest was run under --site-per-process. The
crash occurs if the allocation fails.

There seems to be an identical assertion a couple lines below, that
is correctly guarded against a null pointer.

BUG=None
TEST=browser_tests
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5510c5b383ec6ffb22ae30a9a0fc4ead5a106a4f
Cr-Commit-Position: refs/heads/master@{#373954}
==========

[commit-bot: I haz the power, 2016-02-05 23:53:55]

Patchset 1 (id:??) landed as
https://crrev.com/5510c5b383ec6ffb22ae30a9a0fc4ead5a106a4f
Cr-Commit-Position: refs/heads/master@{#373954}

[reveman, 2016-02-06 00:11:45]

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

[reveman, 2016-02-06 00:11:45]

lgtm

but can you open a bug with exact details for how you were able to reproduce
this issue? Allocations used to be able to fail when the gpu process crashed but
that has changed and allocations should never fail anymore. I'd like to make GMB
allocation failure fatal and being able to reproduce this shutdown issue will
help enormously.