Query objects should not be shared between contexts

Query objects should not be shared between contexts

glQuery objects are not sharable, so managing IDs for each client
locally in GLES2Implementation.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282292

[hj.r.chung, 2014-07-08 12:26:34]

PTAL!

[piman, 2014-07-08 17:37:05]

+sievers -piman

[no sievers, 2014-07-08 22:54:04]

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:4063: args['alloc_code'] =
alloc_code
what's this line needed for?

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:18: #include <utility>
needed?

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:327: return
share_group_->GetIdHandler(namespace_id);
How about DCHECK(namespace_id != id_namespaces::kQueries)?

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:335: return NULL;
NOTREACHED?

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:3349:
query_id_allocator_->FreeID(queries[ii]);
You are removing the check for whether these IDs were allocated with GenQueries.
Maybe leave that in and still set the GL error in that case?

[hj.r.chung, 2014-07-09 05:26:46]

updated patch and left a couple of comments!

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/build_gle...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/build_gle...
gpu/command_buffer/build_gles2_cmd_buffer.py:4063: args['alloc_code'] =
alloc_code
On 2014/07/08 22:54:04, sievers wrote:
> what's this line needed for?
I added 'alloc_code' because only that portion of the code
needs to be different when objects aren't shared,
and didn't want to split up the file.Write or duplicate code

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:18: #include <utility>
On 2014/07/08 22:54:04, sievers wrote:
> needed?

Done.

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:327: return
share_group_->GetIdHandler(namespace_id);
On 2014/07/08 22:54:04, sievers wrote:
> How about DCHECK(namespace_id != id_namespaces::kQueries)?

Done.

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:335: return NULL;
On 2014/07/08 22:54:04, sievers wrote:
> NOTREACHED?

Done.

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:3349:
query_id_allocator_->FreeID(queries[ii]);
On 2014/07/08 22:54:04, sievers wrote:
> You are removing the check for whether these IDs were allocated with
GenQueries.
> Maybe leave that in and still set the GL error in that case?

For now IdHandlerInterface::FreeIds always returns true, and glDeleteQueries
give errors only when id is negative.
Should I add the check for negative ids?

[no sievers, 2014-07-09 17:50:49]

lgtm

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:327: return
share_group_->GetIdHandler(namespace_id);
On 2014/07/09 05:26:46, hj.r.chung wrote:
> On 2014/07/08 22:54:04, sievers wrote:
> > How about DCHECK(namespace_id != id_namespaces::kQueries)?
> 
> Done.

I don't see it :)

https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
gpu/command_buffer/client/gles2_implementation.cc:3349:
query_id_allocator_->FreeID(queries[ii]);
On 2014/07/09 05:26:46, hj.r.chung wrote:
> On 2014/07/08 22:54:04, sievers wrote:
> > You are removing the check for whether these IDs were allocated with
> GenQueries.
> > Maybe leave that in and still set the GL error in that case?
> 
> For now IdHandlerInterface::FreeIds always returns true, and glDeleteQueries
> give errors only when id is negative.
> Should I add the check for negative ids?

Ok, I missed that the old code always returned true, so that's fine then.

https://codereview.chromium.org/371363002/diff/20001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/371363002/diff/20001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:331: DCHECK(namespace_id ==
id_namespaces::kQueries);
nit: Probably don't need both the DCHECK() *and* NOTREACHED() below.

[hj.r.chung, 2014-07-10 01:19:04]

On 2014/07/09 17:50:49, sievers wrote:
> lgtm
> 
>
https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
> 
>
https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
> gpu/command_buffer/client/gles2_implementation.cc:327: return
> share_group_->GetIdHandler(namespace_id);
> On 2014/07/09 05:26:46, hj.r.chung wrote:
> > On 2014/07/08 22:54:04, sievers wrote:
> > > How about DCHECK(namespace_id != id_namespaces::kQueries)?
> > 
> > Done.
> 
> I don't see it :)
> 
>
https://codereview.chromium.org/371363002/diff/1/gpu/command_buffer/client/gl...
> gpu/command_buffer/client/gles2_implementation.cc:3349:
> query_id_allocator_->FreeID(queries[ii]);
> On 2014/07/09 05:26:46, hj.r.chung wrote:
> > On 2014/07/08 22:54:04, sievers wrote:
> > > You are removing the check for whether these IDs were allocated with
> > GenQueries.
> > > Maybe leave that in and still set the GL error in that case?
> > 
> > For now IdHandlerInterface::FreeIds always returns true, and glDeleteQueries
> > give errors only when id is negative.
> > Should I add the check for negative ids?
> 
> Ok, I missed that the old code always returned true, so that's fine then.
> 
>
https://codereview.chromium.org/371363002/diff/20001/gpu/command_buffer/clien...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
> 
>
https://codereview.chromium.org/371363002/diff/20001/gpu/command_buffer/clien...
> gpu/command_buffer/client/gles2_implementation.cc:331: DCHECK(namespace_id ==
> id_namespaces::kQueries);
> nit: Probably don't need both the DCHECK() *and* NOTREACHED() below.

thank you for the review!

[hj.r.chung, 2014-07-10 01:19:28]

https://codereview.chromium.org/371363002/diff/20001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/371363002/diff/20001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:331: DCHECK(namespace_id ==
id_namespaces::kQueries);
On 2014/07/09 17:50:48, sievers wrote:
> nit: Probably don't need both the DCHECK() *and* NOTREACHED() below.

Done.

[hj.r.chung, 2014-07-10 02:46:14]

The CQ bit was checked by heejin.r.chung@samsung.com

[commit-bot: I haz the power, 2014-07-10 02:48:04]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/heejin.r.chung@samsung.com/371363002/4...

[commit-bot: I haz the power, 2014-07-10 03:33:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26844)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/31283)

[commit-bot: I haz the power, 2014-07-10 03:38:51]

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

[commit-bot: I haz the power, 2014-07-10 03:38:52]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[hj.r.chung, 2014-07-10 04:21:06]

The CQ bit was checked by heejin.r.chung@samsung.com

[commit-bot: I haz the power, 2014-07-10 04:23:24]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/heejin.r.chung@samsung.com/371363002/6...

[hj.r.chung, 2014-07-10 04:44:27]

The CQ bit was checked by heejin.r.chung@samsung.com

[commit-bot: I haz the power, 2014-07-10 04:44:47]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/heejin.r.chung@samsung.com/371363002/8...

[commit-bot: I haz the power, 2014-07-10 08:26:15]

Change committed as 282292

[ccameron, 2014-07-10 19:09:15]

On 2014/07/10 08:26:15, I haz the power (commit-bot) wrote:
> Change committed as 282292

I've been seeing lots of failures of the WebGL conformance test
conformance_context_context_release_with_workers since this change. The failures
are intermittent, so it's not clear that it is this change, but it's the only
recent GPU/WebGL related change. I may revert to see if it fixes the problem.