Prevent crash when using getExtension while a PBO is bound.

Prevent crash when using getExtension while a PBO is bound.

BUG=641643
CQ_INCLUDE_TRYBOTS=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

Committed: https://crrev.com/803bcd7befd6a191578a8752ff82cd2bcddd7e1f
Cr-Commit-Position: refs/heads/master@{#415547}

[Kai Ninomiya, 2016-08-29 18:05:49]

Description was changed from

==========
Prevent crash when using getExtension while a PBO is bound.

BUG=641643
==========

to

==========
Prevent crash when using getExtension while a PBO is bound.

BUG=641643
CQ_INCLUDE_TRYBOTS=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
==========

[Kai Ninomiya, 2016-08-29 18:32:32]

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

[Kai Ninomiya, 2016-08-29 18:33:17]

kainino@chromium.org changed reviewers:
+ kbr@chromium.org, zmo@chromium.org

[Kai Ninomiya, 2016-08-29 18:33:18]

PTAL

[commit-bot: I haz the power, 2016-08-29 18:59:55]

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

[Zhenyao Mo, 2016-08-29 19:15:25]

On 2016/08/29 18:33:18, Kai Ninomiya wrote:
> PTAL

The code looks fine, but you will need to update the test expectations.

[Zhenyao Mo, 2016-08-29 19:18:48]

https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.cc:92:
glBindBuffer(GL_PIXEL_UNPACK_BUFFER, binding_override);
A minor optimization: if original_bindings_ == binding_override, then no
BindBuffer() calls.

This should also simplify the test expectations change.

[Ken Russell (switch to Gerrit), 2016-08-29 19:36:17]

https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.cc:92:
glBindBuffer(GL_PIXEL_UNPACK_BUFFER, binding_override);
On 2016/08/29 19:18:48, Zhenyao Mo wrote:
> A minor optimization: if original_bindings_ == binding_override, then no
> BindBuffer() calls.
> 
> This should also simplify the test expectations change.

Good point Mo. See also below.

https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.cc:97: if (original_binding_) {
Similar test could be done here to avoid restoring the binding if it was
unchanged.

https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.cc:779: ScopedPixelUnpackBufferOverride
scoped_pbo_override(context_type_, 0);
I think you should move this up to the top of this method,
FeatureInfo::InitializeFeatures. Otherwise, the same bug is likely to crop up
again if someone inserts any extension checking code which calls TexImage2D.

[commit-bot: I haz the power, 2016-08-30 03:42:52]

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

[commit-bot: I haz the power, 2016-08-30 03:42:53]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Kai Ninomiya, 2016-08-30 23:45:26]

Patchset #2 (id:20001) has been deleted

[Kai Ninomiya, 2016-08-30 23:45:51]

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

[commit-bot: I haz the power, 2016-08-30 23:46:24]

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

[Kai Ninomiya, 2016-08-30 23:49:25]

Updated; PTAL.


On 2016/08/29 19:36:17, Ken Russell wrote:
>
https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
> File gpu/command_buffer/service/feature_info.cc (right):
> 
>
https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
> gpu/command_buffer/service/feature_info.cc:92:
> glBindBuffer(GL_PIXEL_UNPACK_BUFFER, binding_override);
> On 2016/08/29 19:18:48, Zhenyao Mo wrote:
> > A minor optimization: if original_bindings_ == binding_override, then no
> > BindBuffer() calls.
> > 
> > This should also simplify the test expectations change.
> 
> Good point Mo. See also below.
> 
>
https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
> gpu/command_buffer/service/feature_info.cc:97: if (original_binding_) {
> Similar test could be done here to avoid restoring the binding if it was
> unchanged.
> 
>
https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
> gpu/command_buffer/service/feature_info.cc:779:
ScopedPixelUnpackBufferOverride
> scoped_pbo_override(context_type_, 0);
> I think you should move this up to the top of this method,
> FeatureInfo::InitializeFeatures. Otherwise, the same bug is likely to crop up
> again if someone inserts any extension checking code which calls TexImage2D.

[Kai Ninomiya, 2016-08-30 23:51:02]

https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.cc:92:
glBindBuffer(GL_PIXEL_UNPACK_BUFFER, binding_override);
On 2016/08/29 19:18:48, Zhenyao Mo wrote:
> A minor optimization: if original_bindings_ == binding_override, then no
> BindBuffer() calls.
> 
> This should also simplify the test expectations change.

Done.

https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.cc:97: if (original_binding_) {
On 2016/08/29 19:36:17, Ken Russell wrote:
> Similar test could be done here to avoid restoring the binding if it was
> unchanged.

Done.

https://codereview.chromium.org/2291753002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/feature_info.cc:779: ScopedPixelUnpackBufferOverride
scoped_pbo_override(context_type_, 0);
On 2016/08/29 19:36:17, Ken Russell wrote:
> I think you should move this up to the top of this method,
> FeatureInfo::InitializeFeatures. Otherwise, the same bug is likely to crop up
> again if someone inserts any extension checking code which calls TexImage2D.

Done.

[Zhenyao Mo, 2016-08-31 00:15:20]

Great job!

Mostly looks good with two minor suggestions.

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager_unittest.cc:74: if (feature_info) {
nit: You can move if (!feature_info) feature_info = new FeatureInfo() before
this.  Then context_type can just be feature_info->context_type(), hiding the
assumption default context_type is OPENGLES2.

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:342: context_type, gl_info.is_es3);
gl_info.is_es3 looks incorrect.  It indicates whether the underlying driver is
es3, not whether es3 is enabled in command buffer. I think you need to add an
extra parameter to SetupContextGroupInitExpectations() to indicate that.

[Kai Ninomiya, 2016-08-31 00:47:59]

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager_unittest.cc:74: if (feature_info) {
On 2016/08/31 00:15:19, Zhenyao Mo wrote:
> nit: You can move if (!feature_info) feature_info = new FeatureInfo() before
> this.  Then context_type can just be feature_info->context_type(), hiding the
> assumption default context_type is OPENGLES2.

Done (not yet uploaded).

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:342: context_type, gl_info.is_es3);
On 2016/08/31 00:15:19, Zhenyao Mo wrote:
> gl_info.is_es3 looks incorrect.  It indicates whether the underlying driver is
> es3, not whether es3 is enabled in command buffer. I think you need to add an
> extra parameter to SetupContextGroupInitExpectations() to indicate that.

Hm... okay. I can pass in that extra parameter easily for the 2 tests which call
SetupContextGroupInitExpectations. But it's also called from
GLES2DecoderTestBase::InitDecoderFromCommandLine
(https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec...).

Despite the `GLES2*` name, it seems that I can't assume `enable_es3` should
always be false here. Should I propagate this change all the way up to the
individual tests? Or can I compute `enable_es3` from, like, `(init.context_type
== CONTEXT_TYPE_WEBGL2 || init.context_type == CONTEXT_TYPE_OPENGLES3)`?

[Zhenyao Mo, 2016-08-31 00:51:41]

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:342: context_type, gl_info.is_es3);
On 2016/08/31 00:47:59, Kai Ninomiya wrote:
> On 2016/08/31 00:15:19, Zhenyao Mo wrote:
> > gl_info.is_es3 looks incorrect.  It indicates whether the underlying driver
is
> > es3, not whether es3 is enabled in command buffer. I think you need to add
an
> > extra parameter to SetupContextGroupInitExpectations() to indicate that.
> 
> Hm... okay. I can pass in that extra parameter easily for the 2 tests which
call
> SetupContextGroupInitExpectations. But it's also called from
> GLES2DecoderTestBase::InitDecoderFromCommandLine
>
(https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec...).
> 
> Despite the `GLES2*` name, it seems that I can't assume `enable_es3` should
> always be false here. Should I propagate this change all the way up to the
> individual tests? Or can I compute `enable_es3` from, like,
`(init.context_type
> == CONTEXT_TYPE_WEBGL2 || init.context_type == CONTEXT_TYPE_OPENGLES3)`?

Yeah that should be fine. In testing, if we specify the context type, it is
always obeyed.

[Kai Ninomiya, 2016-08-31 01:11:17]

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:342: context_type, gl_info.is_es3);
On 2016/08/31 00:51:40, Zhenyao Mo wrote:
> On 2016/08/31 00:47:59, Kai Ninomiya wrote:
> > On 2016/08/31 00:15:19, Zhenyao Mo wrote:
> > > gl_info.is_es3 looks incorrect.  It indicates whether the underlying
driver
> is
> > > es3, not whether es3 is enabled in command buffer. I think you need to add
> an
> > > extra parameter to SetupContextGroupInitExpectations() to indicate that.
> > 
> > Hm... okay. I can pass in that extra parameter easily for the 2 tests which
> call
> > SetupContextGroupInitExpectations. But it's also called from
> > GLES2DecoderTestBase::InitDecoderFromCommandLine
> >
>
(https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec...).
> > 
> > Despite the `GLES2*` name, it seems that I can't assume `enable_es3` should
> > always be false here. Should I propagate this change all the way up to the
> > individual tests? Or can I compute `enable_es3` from, like,
> `(init.context_type
> > == CONTEXT_TYPE_WEBGL2 || init.context_type == CONTEXT_TYPE_OPENGLES3)`?
> 
> Yeah that should be fine. In testing, if we specify the context type, it is
> always obeyed.

Okay. I also quickly simulated removing `enable_es3` entirely (i.e., replacing
it with context_type). But that does not work: `context_type` is GLES2 and
`enable_es3` is true at one point in TextureManagerTest::SetupFeatureInfo
(https://cs.chromium.org/chromium/src/gpu/command_buffer/service/texture_manag...).
Is that okay?

[Zhenyao Mo, 2016-08-31 01:18:30]

lgtm

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/test_helper.cc (right):

https://codereview.chromium.org/2291753002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/test_helper.cc:342: context_type, gl_info.is_es3);
On 2016/08/31 01:11:17, Kai Ninomiya wrote:
> On 2016/08/31 00:51:40, Zhenyao Mo wrote:
> > On 2016/08/31 00:47:59, Kai Ninomiya wrote:
> > > On 2016/08/31 00:15:19, Zhenyao Mo wrote:
> > > > gl_info.is_es3 looks incorrect.  It indicates whether the underlying
> driver
> > is
> > > > es3, not whether es3 is enabled in command buffer. I think you need to
add
> > an
> > > > extra parameter to SetupContextGroupInitExpectations() to indicate that.
> > > 
> > > Hm... okay. I can pass in that extra parameter easily for the 2 tests
which
> > call
> > > SetupContextGroupInitExpectations. But it's also called from
> > > GLES2DecoderTestBase::InitDecoderFromCommandLine
> > >
> >
>
(https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec...).
> > > 
> > > Despite the `GLES2*` name, it seems that I can't assume `enable_es3`
should
> > > always be false here. Should I propagate this change all the way up to the
> > > individual tests? Or can I compute `enable_es3` from, like,
> > `(init.context_type
> > > == CONTEXT_TYPE_WEBGL2 || init.context_type == CONTEXT_TYPE_OPENGLES3)`?
> > 
> > Yeah that should be fine. In testing, if we specify the context type, it is
> > always obeyed.
> 
> Okay. I also quickly simulated removing `enable_es3` entirely (i.e., replacing
> it with context_type). But that does not work: `context_type` is GLES2 and
> `enable_es3` is true at one point in TextureManagerTest::SetupFeatureInfo
>
(https://cs.chromium.org/chromium/src/gpu/command_buffer/service/texture_manag...).
> Is that okay?

I am sorry you stepped on these issues.  Such inconsistencies are definitely
poor designs - originally the command buffer was only designed for ES2.  So to
force it to support ES3 without a major re-design, this is what we ended up
with.

[Kai Ninomiya, 2016-08-31 01:23:41]

The CQ bit was checked by kainino@chromium.org

[commit-bot: I haz the power, 2016-08-31 01:24:28]

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, 2016-08-31 02:09:21]

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

[commit-bot: I haz the power, 2016-08-31 02:09:22]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Zhenyao Mo, 2016-08-31 02:11:45]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2016-08-31 02:12:03]

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, 2016-08-31 02:52:54]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-31 02:54:09]

Description was changed from

==========
Prevent crash when using getExtension while a PBO is bound.

BUG=641643
CQ_INCLUDE_TRYBOTS=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

==========
Prevent crash when using getExtension while a PBO is bound.

BUG=641643
CQ_INCLUDE_TRYBOTS=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

Committed: https://crrev.com/803bcd7befd6a191578a8752ff82cd2bcddd7e1f
Cr-Commit-Position: refs/heads/master@{#415547}
==========

[commit-bot: I haz the power, 2016-08-31 02:54:11]

Patchset 3 (id:??) landed as
https://crrev.com/803bcd7befd6a191578a8752ff82cd2bcddd7e1f
Cr-Commit-Position: refs/heads/master@{#415547}