gpu: fix gpu process crashed on win_blink_rel due to dcheck in CopyTextureCHROMIUM.

gpu: fix gpu process crashed on win_blink_rel due to dcheck in CopyTextureCHROMIUM.

When gpu context is lost, CopyTextureCHROMIUMResourceManager can be deleted
without releasing resources like GLES2DecoderImpl.

BUG=438950
Committed: https://crrev.com/4decc8740001718826b165f64428829e4b4abfec
Cr-Commit-Position: refs/heads/master@{#307075}

[dshwang, 2014-12-05 14:35:42]

dongseong.hwang@intel.com changed reviewers:
+ kbr@chromium.org, sievers@chromium.org

[dshwang, 2014-12-05 14:35:43]

@sievers, could you review this P1 bug?

GLES2DecoderImpl::Destroy() doesn't release resources when real gl context is
lost.
It means dtor of CopyTextureCHROMIUMResourceManager can be called without
calling CopyTextureCHROMIUMResourceManager::Destroy().
So I remove the invalid DCHECK.

[dsinclair, 2014-12-05 17:43:44]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org, piman@chromium.org

[no sievers, 2014-12-05 18:21:04]

lgtm

[no sievers, 2014-12-05 18:21:05]

lgtm

[Ken Russell (switch to Gerrit), 2014-12-05 18:59:28]

https://codereview.chromium.org/780293002/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (left):

https://codereview.chromium.org/780293002/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:263:
DCHECK(!buffer_id_);
Please add the comment here from the CL description about why this manager might
be deleted without releasing resources. Otherwise it's going to be difficult to
reconstruct from the version history why this was removed.

[dshwang, 2014-12-05 19:54:49]

On 2014/12/05 18:21:05, sievers wrote:
> lgtm

Thank you for review.

On 2014/12/05 18:59:28, Ken Russell wrote:
>
https://codereview.chromium.org/780293002/diff/1/gpu/command_buffer/service/g...
> File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (left):
> 
>
https://codereview.chromium.org/780293002/diff/1/gpu/command_buffer/service/g...
> gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:263:
> DCHECK(!buffer_id_);
> Please add the comment here from the CL description about why this manager
might
> be deleted without releasing resources. Otherwise it's going to be difficult
to
> reconstruct from the version history why this was removed.

Yes, done.

[dshwang, 2014-12-05 19:54:54]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-12-05 19:55:25]

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

[piman, 2014-12-05 20:34:12]

https://codereview.chromium.org/780293002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/780293002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:265: //
GLES2DecoderImpl.
nit: is there a way to check that the GL resources were cleared if the context
was not lost? I wouldn't want a new field introduced that we forgot to clear and
cause a leak. Since we don't track GL-level objects, it can be very hard to
detect and track down this kind of leak.

[no sievers, 2014-12-05 20:38:15]

On 2014/12/05 20:34:12, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/780293002/diff/20001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):
> 
>
https://codereview.chromium.org/780293002/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:265: //
> GLES2DecoderImpl.
> nit: is there a way to check that the GL resources were cleared if the context
> was not lost? I wouldn't want a new field introduced that we forgot to clear
and
> cause a leak. Since we don't track GL-level objects, it can be very hard to
> detect and track down this kind of leak.

There must be expectations in the decoder unittests for creating and destroying
the resources.
(Although this kind of testing is really a bit of writing the same code twice in
different ways :)

[no sievers, 2014-12-05 20:45:17]

On 2014/12/05 20:38:15, sievers wrote:
> On 2014/12/05 20:34:12, piman (Very slow to review) wrote:
> >
>
https://codereview.chromium.org/780293002/diff/20001/gpu/command_buffer/servi...
> > File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):
> > 
> >
>
https://codereview.chromium.org/780293002/diff/20001/gpu/command_buffer/servi...
> > gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:265: //
> > GLES2DecoderImpl.
> > nit: is there a way to check that the GL resources were cleared if the
context
> > was not lost? I wouldn't want a new field introduced that we forgot to clear
> and
> > cause a leak. Since we don't track GL-level objects, it can be very hard to
> > detect and track down this kind of leak.
> 
> There must be expectations in the decoder unittests for creating and
destroying
> the resources.
> (Although this kind of testing is really a bit of writing the same code twice
in
> different ways :)

Nope, that was removed in https://chromiumcodereview.appspot.com/10450030.
(Although the ids, ::kServiceCopyTextureChromium* are left behind.)

[no sievers, 2014-12-05 20:46:46]

On 2014/12/05 20:45:17, sievers wrote:
> On 2014/12/05 20:38:15, sievers wrote:
> > On 2014/12/05 20:34:12, piman (Very slow to review) wrote:
> > >
> >
>
https://codereview.chromium.org/780293002/diff/20001/gpu/command_buffer/servi...
> > > File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/780293002/diff/20001/gpu/command_buffer/servi...
> > > gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:265: //
> > > GLES2DecoderImpl.
> > > nit: is there a way to check that the GL resources were cleared if the
> context
> > > was not lost? I wouldn't want a new field introduced that we forgot to
clear
> > and
> > > cause a leak. Since we don't track GL-level objects, it can be very hard
to
> > > detect and track down this kind of leak.
> > 
> > There must be expectations in the decoder unittests for creating and
> destroying
> > the resources.
> > (Although this kind of testing is really a bit of writing the same code
twice
> in
> > different ways :)
> 
> Nope, that was removed in https://chromiumcodereview.appspot.com/10450030.
> (Although the ids, ::kServiceCopyTextureChromium* are left behind.)

Can we just make Destroy(bool have_context) explicit and DCHECK(was_destroyed)
in the destructor?

Btw, I introduced the same bug in gl_copy_texture_CHROMIUM_unittest.cc which
doesn't handle context lost.

[no sievers, 2014-12-05 20:47:08]

On 2014/12/05 20:46:46, sievers wrote:
> On 2014/12/05 20:45:17, sievers wrote:
> > On 2014/12/05 20:38:15, sievers wrote:
> > > On 2014/12/05 20:34:12, piman (Very slow to review) wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/780293002/diff/20001/gpu/command_buffer/servi...
> > > > File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/780293002/diff/20001/gpu/command_buffer/servi...
> > > > gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:265: //
> > > > GLES2DecoderImpl.
> > > > nit: is there a way to check that the GL resources were cleared if the
> > context
> > > > was not lost? I wouldn't want a new field introduced that we forgot to
> clear
> > > and
> > > > cause a leak. Since we don't track GL-level objects, it can be very hard
> to
> > > > detect and track down this kind of leak.
> > > 
> > > There must be expectations in the decoder unittests for creating and
> > destroying
> > > the resources.
> > > (Although this kind of testing is really a bit of writing the same code
> twice
> > in
> > > different ways :)
> > 
> > Nope, that was removed in https://chromiumcodereview.appspot.com/10450030.
> > (Although the ids, ::kServiceCopyTextureChromium* are left behind.)
> 
> Can we just make Destroy(bool have_context) explicit and DCHECK(was_destroyed)
> in the destructor?
> 
> Btw, I introduced the same bug in gl_copy_texture_CHROMIUM_unittest.cc which
> doesn't handle context lost.

Err gles2_cmd_clear_framebuffer.cc I meant.

[commit-bot: I haz the power, 2014-12-05 20:47:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-05 20:48:37]

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