Fix a bug in shader/program relationship.

Fix a bug in shader/program relationship.

A program can have two shaders attached in ES2/3. However, a linked program's
shaders may not be the current attached shaders to that program. Consider the
following sequence.

1) attach Shader1 and Shader2 to Program
2) link Program, succeed
3) detach Shader1 and Shader2 from Program
4) attach Shader3 and Shader4 to program

So after step 3, no shaders are attached to program. After step 4, Shader3 and
Shader4 are attached to Program, not Shader1 and Shader2. However, if we want
to use shaders to access information from Program, we still need to use Shader1
and Shader2.

BUG=735784
TEST=conformance2/uniforms/query-uniform-blocks-after-shader-detach.html
R=kbr@chromium.org
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2958763003
Cr-Commit-Position: refs/heads/master@{#482477}
Committed: https://chromium.googlesource.com/chromium/src/+/8e4a133d94d88ad3fcf2705fb45f16ab72b58092

[Zhenyao Mo, 2017-06-26 19:43:48]

Description was changed from

==========
Fix a bug in shader/program relationship.

A program can have two shaders attached in ES2/3. However, a linked program's
shaders may not be the current attached shaders to that program. Consider the
following sequence.

1) attach Shader1 and Shader2 to Program
2) link Program, succeed
3) detach Shader1 and Shader2 from Program
4) attach Shader3 and Shader4 to program

So after step 3, no shaders are attached to program. After step 4, Shader3 and
Shader4 are attached to Program, not Shader1 and Shader2. However, if we want
to use shaders to access information from Program, we still need to use Shader1
and Shader2.

BUG=735784
TEST=conformance2/uniforms/query-uniform-blocks-after-shader-detach.html
R=kbr@chromium.org
==========

to

==========
Fix a bug in shader/program relationship.

A program can have two shaders attached in ES2/3. However, a linked program's
shaders may not be the current attached shaders to that program. Consider the
following sequence.

1) attach Shader1 and Shader2 to Program
2) link Program, succeed
3) detach Shader1 and Shader2 from Program
4) attach Shader3 and Shader4 to program

So after step 3, no shaders are attached to program. After step 4, Shader3 and
Shader4 are attached to Program, not Shader1 and Shader2. However, if we want
to use shaders to access information from Program, we still need to use Shader1
and Shader2.

BUG=735784
TEST=conformance2/uniforms/query-uniform-blocks-after-shader-detach.html
R=kbr@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[Zhenyao Mo, 2017-06-26 19:44:02]

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

[commit-bot: I haz the power, 2017-06-26 19:44:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhenyao Mo, 2017-06-26 19:44:34]

This is the CL. I'll ping you for review when bots are green.

[commit-bot: I haz the power, 2017-06-26 20:05:33]

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

[commit-bot: I haz the power, 2017-06-26 20:05:35]

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

[Ken Russell (switch to Gerrit), 2017-06-26 20:18:32]

Great work, Mo. Thanks for picking this up. It looks good overall but please do
tell me when the trybots are green.

[Zhenyao Mo, 2017-06-26 21:30:17]

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

[commit-bot: I haz the power, 2017-06-26 21:30:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhenyao Mo, 2017-06-26 22:30:04]

kbr: Ready for review
(The original CL got a bug.  Basically any use of shaders before link should be
attached_shaders_, any use of shaders after link should be
shaders_from_last_successful_link_).

[commit-bot: I haz the power, 2017-06-26 22:44:52]

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

[commit-bot: I haz the power, 2017-06-26 22:44:52]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2017-06-26 22:56:20]

lgtm

https://codereview.chromium.org/2958763003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/2958763003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:1156: Shader* vertex_shader =
attached_shaders_[0].get();
Here and in several more places, there are some methods which are called before
or during program linking, and that's why attached_shaders_ is used instead of
shaders_from_last_successful_link_. This is a little subtle; is it worth adding
a comment like:

  // This is called before program linking, so refer to attached_shaders_.

at each of these sites?

https://codereview.chromium.org/2958763003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:1191:
attached_shaders_[ShaderTypeToIndex(GL_FRAGMENT_SHADER)].get();
Same here.

https://codereview.chromium.org/2958763003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:1520: for (auto shader :
attached_shaders_) {
Same here.

https://codereview.chromium.org/2958763003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:1533: for (auto shader :
attached_shaders_) {
Same here.

https://codereview.chromium.org/2958763003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:1546: for (auto shader :
attached_shaders_) {
Same here. There are more places, too.

[Zhenyao Mo, 2017-06-26 23:57:18]

Description was changed from

==========
Fix a bug in shader/program relationship.

A program can have two shaders attached in ES2/3. However, a linked program's
shaders may not be the current attached shaders to that program. Consider the
following sequence.

1) attach Shader1 and Shader2 to Program
2) link Program, succeed
3) detach Shader1 and Shader2 from Program
4) attach Shader3 and Shader4 to program

So after step 3, no shaders are attached to program. After step 4, Shader3 and
Shader4 are attached to Program, not Shader1 and Shader2. However, if we want
to use shaders to access information from Program, we still need to use Shader1
and Shader2.

BUG=735784
TEST=conformance2/uniforms/query-uniform-blocks-after-shader-detach.html
R=kbr@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Fix a bug in shader/program relationship.

A program can have two shaders attached in ES2/3. However, a linked program's
shaders may not be the current attached shaders to that program. Consider the
following sequence.

1) attach Shader1 and Shader2 to Program
2) link Program, succeed
3) detach Shader1 and Shader2 from Program
4) attach Shader3 and Shader4 to program

So after step 3, no shaders are attached to program. After step 4, Shader3 and
Shader4 are attached to Program, not Shader1 and Shader2. However, if we want
to use shaders to access information from Program, we still need to use Shader1
and Shader2.

BUG=735784
TEST=conformance2/uniforms/query-uniform-blocks-after-shader-detach.html
R=kbr@chromium.org
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[Zhenyao Mo, 2017-06-26 23:58:53]

Added comments per kbr review
Landing with NOTRY=true because patchset2 already passed all bots and patchset3
only adds a few comments.

[Zhenyao Mo, 2017-06-26 23:58:57]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2017-06-26 23:58:57]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2958763003/#ps40001
(title: "comments")

[commit-bot: I haz the power, 2017-06-26 23:59:14]

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, 2017-06-27 00:03:49]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498521537036890,
"parent_rev": "c8eea79006640a377dd1e71bcf7c9dfaa551607f", "commit_rev":
"8e4a133d94d88ad3fcf2705fb45f16ab72b58092"}

[commit-bot: I haz the power, 2017-06-27 00:04:01]

Description was changed from

==========
Fix a bug in shader/program relationship.

A program can have two shaders attached in ES2/3. However, a linked program's
shaders may not be the current attached shaders to that program. Consider the
following sequence.

1) attach Shader1 and Shader2 to Program
2) link Program, succeed
3) detach Shader1 and Shader2 from Program
4) attach Shader3 and Shader4 to program

So after step 3, no shaders are attached to program. After step 4, Shader3 and
Shader4 are attached to Program, not Shader1 and Shader2. However, if we want
to use shaders to access information from Program, we still need to use Shader1
and Shader2.

BUG=735784
TEST=conformance2/uniforms/query-uniform-blocks-after-shader-detach.html
R=kbr@chromium.org
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Fix a bug in shader/program relationship.

A program can have two shaders attached in ES2/3. However, a linked program's
shaders may not be the current attached shaders to that program. Consider the
following sequence.

1) attach Shader1 and Shader2 to Program
2) link Program, succeed
3) detach Shader1 and Shader2 from Program
4) attach Shader3 and Shader4 to program

So after step 3, no shaders are attached to program. After step 4, Shader3 and
Shader4 are attached to Program, not Shader1 and Shader2. However, if we want
to use shaders to access information from Program, we still need to use Shader1
and Shader2.

BUG=735784
TEST=conformance2/uniforms/query-uniform-blocks-after-shader-detach.html
R=kbr@chromium.org
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2958763003
Cr-Commit-Position: refs/heads/master@{#482477}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e4a133d94d88ad3fcf2705fb45f...
==========

[commit-bot: I haz the power, 2017-06-27 00:04:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8e4a133d94d88ad3fcf2705fb45f...

[Ken Russell (switch to Gerrit), 2017-06-27 20:58:36]

lgtm again after the fact.