Fix double lock of texture resource in software renderer

Fix double lock of texture resource in software renderer

If in software rendering mode, a copy request
of the layer is not in the root or first render pass,
there will be double locking resource crash.
Now we first release old lock and then try to
obtain lock again.

R=danakj@chromium.org

TEST=SoftwareRendererTest.DoubleLockTextureCrash

Committed: https://crrev.com/dc9a82fb83911f33ab690361d98cafd420e222e3
Cr-Commit-Position: refs/heads/master@{#315155}

[danakj, 2015-02-05 17:42:21]

How come LayerTreeHostReadbackPixelTest.ReadbackSmallNonRootLayer_Software does
not crash?

https://codereview.chromium.org/899183003/diff/1/cc/output/software_renderer_...
File cc/output/software_renderer_unittest.cc (right):

https://codereview.chromium.org/899183003/diff/1/cc/output/software_renderer_...
cc/output/software_renderer_unittest.cc:66: device_viewport_rect).Pass();
you dont need Pass() on a return statement

[Lof, 2015-02-05 18:12:31]

On 2015/02/05 17:42:21, danakj wrote:
> How come LayerTreeHostReadbackPixelTest.ReadbackSmallNonRootLayer_Software
does
> not crash?

Because it requests copy of layer in first render pass.
Problem arises if we request a copy of layer that is not in first or root render
pass.
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/direct_r...
here.
 
>
https://codereview.chromium.org/899183003/diff/1/cc/output/software_renderer_...
> File cc/output/software_renderer_unittest.cc (right):
> 
>
https://codereview.chromium.org/899183003/diff/1/cc/output/software_renderer_...
> cc/output/software_renderer_unittest.cc:66: device_viewport_rect).Pass();
> you dont need Pass() on a return statement
Fixed. Could you please explain why Pass() is not necessary here? We have some
holywars about it :)

[danakj, 2015-02-05 18:15:28]

https://codereview.chromium.org/899183003/diff/1/cc/output/software_renderer_...
File cc/output/software_renderer_unittest.cc (right):

https://codereview.chromium.org/899183003/diff/1/cc/output/software_renderer_...
cc/output/software_renderer_unittest.cc:66: device_viewport_rect).Pass();
On 2015/02/05 17:42:21, danakj wrote:
> you dont need Pass() on a return statement

return makes this an xvalue which allows the move constructor to be used. put
more simply when you return the thing no longer has a name here so it is moved.

[Lof, 2015-02-05 19:58:29]

On 2015/02/05 18:15:28, danakj wrote:
>
https://codereview.chromium.org/899183003/diff/1/cc/output/software_renderer_...
> File cc/output/software_renderer_unittest.cc (right):
> 
>
https://codereview.chromium.org/899183003/diff/1/cc/output/software_renderer_...
> cc/output/software_renderer_unittest.cc:66: device_viewport_rect).Pass();
> On 2015/02/05 17:42:21, danakj wrote:
> > you dont need Pass() on a return statement
> 
> return makes this an xvalue which allows the move constructor to be used. put
> more simply when you return the thing no longer has a name here so it is
moved.

Thank you.
There is no problem with gl_renderer
https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...

[danakj, 2015-02-05 21:19:38]

https://codereview.chromium.org/899183003/diff/20001/cc/output/software_rende...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/899183003/diff/20001/cc/output/software_rende...
cc/output/software_renderer.cc:163: current_framebuffer_lock_ = nullptr;
Can you leave a comment explaining that we are releasing the lock here before
creating a new lock in case the texture is already bound?

IYBN to have a comment in GLRenderer saying such a thing too.

https://codereview.chromium.org/899183003/diff/20001/cc/output/software_rende...
File cc/output/software_renderer_unittest.cc (right):

https://codereview.chromium.org/899183003/diff/20001/cc/output/software_rende...
cc/output/software_renderer_unittest.cc:453: TEST_F(SoftwareRendererTest,
DoubleLockTextureCrash) {
I'm glad GL renderer doesn't have this problem, but do we have any test guarding
it from regressing?

Can we make a test that will work for both renderers?

[Lof, 2015-02-06 10:32:07]

All done.

[danakj, 2015-02-06 18:57:27]

https://codereview.chromium.org/899183003/diff/40001/cc/output/software_rende...
File cc/output/software_renderer_unittest.cc (right):

https://codereview.chromium.org/899183003/diff/40001/cc/output/software_rende...
cc/output/software_renderer_unittest.cc:453: TEST_F(SoftwareRendererTest,
DoubleLockTextureCrash) {
I dont think we need this test too anymore

https://codereview.chromium.org/899183003/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_readback.cc (right):

https://codereview.chromium.org/899183003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1151:
TEST_P(LayerTreeHostReadbackCrashPixelTest,
LTHReadbackNonFirstNonRootRenderPassTest?

https://codereview.chromium.org/899183003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1153:
scoped_refptr<SolidColorLayer> background =
Can you add comment explaining this test has 3 render passes with the copy
request on the renderpass in the middle. Doing a copy request can be destructive
of state, so for renderpasses after the first drawn the code path is different.
This verifies the non-first and non-root path. And reference
http://crbug.com/99393.

https://codereview.chromium.org/899183003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1159:
blue->SetForceRenderSurface(true);
can you also put a copy request on here (see line 630 for an example) so that
the other one is the 2nd copy request instead of just the 2nd renderpass? that
should make this test more futureproof.

[Lof, 2015-02-06 22:57:59]

I fixed remarks.

[danakj, 2015-02-06 23:11:43]

Thanks! Looks good, a few small comments

https://codereview.chromium.org/899183003/diff/60001/cc/output/software_rende...
File cc/output/software_renderer_unittest.cc (right):

https://codereview.chromium.org/899183003/diff/60001/cc/output/software_rende...
cc/output/software_renderer_unittest.cc:65: CHECK(!list->empty());
I don't think we need this, back() will crash instead, seems fine?

https://codereview.chromium.org/899183003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_readback.cc (right):

https://codereview.chromium.org/899183003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1147: class
LTHReadbackNonFirstNonRootRenderPassTest
Sorry LTH I meant LayerTreeHost, but was saving typing heh :)

https://codereview.chromium.org/899183003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1153: // This test has 3 render
passes with the copy request
Wrap this comment at 80 columns instead of at 60 or so?

https://codereview.chromium.org/899183003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1154: // on the renderpass in the
middle. Doing a copy request
"RenderPass" or "render pass", not "renderpass"

https://codereview.chromium.org/899183003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1155: // can be destructive of
state, so for renderpasses after
ditto

https://codereview.chromium.org/899183003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1157: // This verifies the non -
first and non - root path.
no spaces around -

https://codereview.chromium.org/899183003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1161:
background->SetForceRenderSurface(true);
do these SetForceRS calls actually do anything for this test? layers with a copy
request get a render surface. If you remove them does the test fail without the
change and pass with it still?

https://codereview.chromium.org/899183003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1165:
blue->SetForceRenderSurface(true);
same here?

[Lof, 2015-02-07 00:05:39]

Fixed it.
By the way, what does "IYBN" mean?

[danakj, 2015-02-07 00:10:10]

On Fri, Feb 6, 2015 at 4:05 PM, <lof84@yandex-team.ru> wrote:

> Fixed it.
> By the way, what does "IYBN" mean?
>

Er, I meant IWBN (It would be nice)


>
> https://codereview.chromium.org/899183003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-02-07 00:12:57]

LGTM

https://codereview.chromium.org/899183003/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_pixeltest_readback.cc (right):

https://codereview.chromium.org/899183003/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_pixeltest_readback.cc:1147: class
LayerTreeHostReadbackNonFirstNonRootRenderPassTest
Sorry, last thing. It should be ....NonRootRenderPassPixelTest. We suffix all
pixel tests with that.

[Lof, 2015-02-07 00:18:35]

New patchsets have been uploaded after l-g-t-m from danakj@chromium.org

[Lof, 2015-02-07 00:19:16]

Done!

[danakj, 2015-02-07 00:20:53]

Thanks LGTM

[danakj, 2015-02-07 00:31:51]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-02-07 00:32:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/899183003/100001

[commit-bot: I haz the power, 2015-02-07 01:32:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-07 01:33:40]

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