Add gpu_angle_passthrough_fuzzer.

Add gpu_angle_passthrough_fuzzer.

Tests the passthrough command decoder and the ANGLE null backend.

Reset the context after each iteration. When the same context was
re-used, it lead to crashes that could not be reproduced with the input
file provided. Theoretically, the fuzzer will still find the same
crashes with a single run using one context unless the crash was in the
intialization/destruction code of the command decoder and depended on a
context having specific state.

BUG=602737
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/2691643002
Cr-Commit-Position: refs/heads/master@{#458103}
Committed: https://chromium.googlesource.com/chromium/src/+/547de958066823514c34b264b0711bb24eb31dc6

[Geoff Lang, 2017-02-10 20:35:43]

Description was changed from

==========
Add gpu_angle_passthrough_fuzzer.

Tests the passthrough command decoder and the ANGLE null backend.

Reset the context after each iteration. When the same context was
re-used, it lead to crashes that could not be reproduced with the input
file provided. Theoretically, the fuzzer will still find the same
crashes with a single run using one context unless the crash was in the
intialization/destruction code of the command decoder and depended on a
context having specific state.

BUG=602737
==========

to

==========
Add gpu_angle_passthrough_fuzzer.

Tests the passthrough command decoder and the ANGLE null backend.

Reset the context after each iteration. When the same context was
re-used, it lead to crashes that could not be reproduced with the input
file provided. Theoretically, the fuzzer will still find the same
crashes with a single run using one context unless the crash was in the
intialization/destruction code of the command decoder and depended on a
context having specific state.

BUG=602737
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
==========

[Geoff Lang, 2017-02-23 19:08:52]

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

[commit-bot: I haz the power, 2017-02-23 19:09:29]

Dry run: 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-02-23 20:32:07]

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

[commit-bot: I haz the power, 2017-02-23 20:32:07]

Dry run: This issue passed the CQ dry run.

[Geoff Lang, 2017-03-09 00:44:26]

Description was changed from

==========
Add gpu_angle_passthrough_fuzzer.

Tests the passthrough command decoder and the ANGLE null backend.

Reset the context after each iteration. When the same context was
re-used, it lead to crashes that could not be reproduced with the input
file provided. Theoretically, the fuzzer will still find the same
crashes with a single run using one context unless the crash was in the
intialization/destruction code of the command decoder and depended on a
context having specific state.

BUG=602737
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

==========
Add gpu_angle_passthrough_fuzzer.

Tests the passthrough command decoder and the ANGLE null backend.

Reset the context after each iteration. When the same context was
re-used, it lead to crashes that could not be reproduced with the input
file provided. Theoretically, the fuzzer will still find the same
crashes with a single run using one context unless the crash was in the
intialization/destruction code of the command decoder and depended on a
context having specific state.

BUG=602737
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
==========

[Geoff Lang, 2017-03-09 00:44:26]

geofflang@chromium.org changed reviewers:
+ mmoroz@chromium.org, piman@chromium.org

[Geoff Lang, 2017-03-09 00:45:25]

PTAL, I haven't uploaded the corpus yet but I'll be using the same one as
gpu_angle_fuzzer but minified for this target.

[mmoroz, 2017-03-09 10:26:04]

LGTM

[piman, 2017-03-09 18:15:14]

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
File gpu/command_buffer/tests/fuzzer_main.cc (right):

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
gpu/command_buffer/tests/fuzzer_main.cc:101: (void)command_line;
nit: ALLOW_UNUSED_LOCAL

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
gpu/command_buffer/tests/fuzzer_main.cc:139:
context_->Initialize(surface_.get(), gl::GLContextAttribs());
Did you intend to recreate the context on every test string run? It feels
heavyweight. Does it make a measurable difference on the execution rate?

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
gpu/command_buffer/tests/fuzzer_main.cc:199: decoder_initialized_ = true;
Why the need for this? Before this change, we initialize and reset in
RunCommandBuffer. Here you added a call to InitDecoder in the constructor, but
it seems redundant with the one in RunCommandBuffer.

[Geoff Lang, 2017-03-09 19:28:55]

Description was changed from

==========
Add gpu_angle_passthrough_fuzzer.

Tests the passthrough command decoder and the ANGLE null backend.

Reset the context after each iteration. When the same context was
re-used, it lead to crashes that could not be reproduced with the input
file provided. Theoretically, the fuzzer will still find the same
crashes with a single run using one context unless the crash was in the
intialization/destruction code of the command decoder and depended on a
context having specific state.

BUG=602737
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

==========
Add gpu_angle_passthrough_fuzzer.

Tests the passthrough command decoder and the ANGLE null backend.

Reset the context after each iteration. When the same context was
re-used, it lead to crashes that could not be reproduced with the input
file provided. Theoretically, the fuzzer will still find the same
crashes with a single run using one context unless the crash was in the
intialization/destruction code of the command decoder and depended on a
context having specific state.

BUG=602737
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
==========

[Geoff Lang, 2017-03-09 19:29:03]

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

[Geoff Lang, 2017-03-09 19:29:10]

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
File gpu/command_buffer/tests/fuzzer_main.cc (right):

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
gpu/command_buffer/tests/fuzzer_main.cc:101: (void)command_line;
On 2017/03/09 18:15:13, piman - On leave - No reviews wrote:
> nit: ALLOW_UNUSED_LOCAL

Done.

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
gpu/command_buffer/tests/fuzzer_main.cc:139:
context_->Initialize(surface_.get(), gl::GLContextAttribs());
On 2017/03/09 18:15:13, piman - On leave - No reviews wrote:
> Did you intend to recreate the context on every test string run? It feels
> heavyweight. Does it make a measurable difference on the execution rate?

Yea, it didn't seem to have too much of an effect on the runtime but I don't
have the numbers on hand so I'll re-verify when I get back to the office.  The
problem was that I encountered a fuzzer failure that was not reproducible with
the test case that the fuzzer gave because it depended on prior state of the
context.

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
gpu/command_buffer/tests/fuzzer_main.cc:199: decoder_initialized_ = true;
On 2017/03/09 18:15:13, piman - On leave - No reviews wrote:
> Why the need for this? Before this change, we initialize and reset in
> RunCommandBuffer. Here you added a call to InitDecoder in the constructor, but
> it seems redundant with the one in RunCommandBuffer.

I added this due to the comment referenced below.  It looks like there were
issues when the context was not initialized before the first run because some of
the sanitizers require that the ANGLE DLLs are loaded first.  The decoder is now
initialized in the constructor when it wasn't before but shouldn't be re-created
redundantly because of the early-out check at the top of InitDecoder.

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
gpu/command_buffer/tests/fuzzer_main.cc:346: CommandBufferSetup* g_setup = new
CommandBufferSetup();
ref

[commit-bot: I haz the power, 2017-03-09 19:29:38]

Dry run: 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-03-09 19:45:15]

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

[commit-bot: I haz the power, 2017-03-09 19:45:16]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[piman, 2017-03-09 22:12:35]

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
File gpu/command_buffer/tests/fuzzer_main.cc (right):

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
gpu/command_buffer/tests/fuzzer_main.cc:199: decoder_initialized_ = true;
On 2017/03/09 19:29:10, Geoff Lang wrote:
> On 2017/03/09 18:15:13, piman - On leave - No reviews wrote:
> > Why the need for this? Before this change, we initialize and reset in
> > RunCommandBuffer. Here you added a call to InitDecoder in the constructor,
but
> > it seems redundant with the one in RunCommandBuffer.
> 
> I added this due to the comment referenced below.  It looks like there were
> issues when the context was not initialized before the first run because some
of
> the sanitizers require that the ANGLE DLLs are loaded first.  The decoder is
now
> initialized in the constructor when it wasn't before but shouldn't be
re-created
> redundantly because of the early-out check at the top of InitDecoder.

Right, the important part is that the ANGLE DLL is loaded, and as long as
InitializeGLOneOffImplementation is called in the constructor, that should be
enough. So I think, even if we move the context creation to InitDecoder, we
don't need to call that in the constructor, and then shouldn't need this bool
and related functionality.

[Geoff Lang, 2017-03-09 22:22:14]

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
File gpu/command_buffer/tests/fuzzer_main.cc (right):

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
gpu/command_buffer/tests/fuzzer_main.cc:199: decoder_initialized_ = true;
On 2017/03/09 22:12:35, piman - On leave - No reviews wrote:
> On 2017/03/09 19:29:10, Geoff Lang wrote:
> > On 2017/03/09 18:15:13, piman - On leave - No reviews wrote:
> > > Why the need for this? Before this change, we initialize and reset in
> > > RunCommandBuffer. Here you added a call to InitDecoder in the constructor,
> but
> > > it seems redundant with the one in RunCommandBuffer.
> > 
> > I added this due to the comment referenced below.  It looks like there were
> > issues when the context was not initialized before the first run because
some
> of
> > the sanitizers require that the ANGLE DLLs are loaded first.  The decoder is
> now
> > initialized in the constructor when it wasn't before but shouldn't be
> re-created
> > redundantly because of the early-out check at the top of InitDecoder.
> 
> Right, the important part is that the ANGLE DLL is loaded, and as long as
> InitializeGLOneOffImplementation is called in the constructor, that should be
> enough. So I think, even if we move the context creation to InitDecoder, we
> don't need to call that in the constructor, and then shouldn't need this bool
> and related functionality.

Sounds good, I'll update that when I get back to Montreal.

[Geoff Lang, 2017-03-16 17:20:21]

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

[commit-bot: I haz the power, 2017-03-16 17:21:10]

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

[Geoff Lang, 2017-03-16 17:22:08]

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
File gpu/command_buffer/tests/fuzzer_main.cc (right):

https://codereview.chromium.org/2691643002/diff/1/gpu/command_buffer/tests/fu...
gpu/command_buffer/tests/fuzzer_main.cc:199: decoder_initialized_ = true;
On 2017/03/09 22:22:13, Geoff Lang wrote:
> On 2017/03/09 22:12:35, piman - On leave - No reviews wrote:
> > On 2017/03/09 19:29:10, Geoff Lang wrote:
> > > On 2017/03/09 18:15:13, piman - On leave - No reviews wrote:
> > > > Why the need for this? Before this change, we initialize and reset in
> > > > RunCommandBuffer. Here you added a call to InitDecoder in the
constructor,
> > but
> > > > it seems redundant with the one in RunCommandBuffer.
> > > 
> > > I added this due to the comment referenced below.  It looks like there
were
> > > issues when the context was not initialized before the first run because
> some
> > of
> > > the sanitizers require that the ANGLE DLLs are loaded first.  The decoder
is
> > now
> > > initialized in the constructor when it wasn't before but shouldn't be
> > re-created
> > > redundantly because of the early-out check at the top of InitDecoder.
> > 
> > Right, the important part is that the ANGLE DLL is loaded, and as long as
> > InitializeGLOneOffImplementation is called in the constructor, that should
be
> > enough. So I think, even if we move the context creation to InitDecoder, we
> > don't need to call that in the constructor, and then shouldn't need this
bool
> > and related functionality.
> 
> Sounds good, I'll update that when I get back to Montreal.

Done, I also updated it to only re-create the context for the passthrough fuzzer
so there are no performance issues.

[piman, 2017-03-16 18:14:34]

LGTM with 1 nit

https://codereview.chromium.org/2691643002/diff/60001/gpu/command_buffer/test...
File gpu/command_buffer/tests/fuzzer_main.cc (right):

https://codereview.chromium.org/2691643002/diff/60001/gpu/command_buffer/test...
gpu/command_buffer/tests/fuzzer_main.cc:290: surface_ = new gl::GLSurfaceStub;
nit: do we need to recreate the surface too? We don't in the other case. I don't
think the stub one keeps any useful state, so I think we can just leave this
out.

[commit-bot: I haz the power, 2017-03-16 19:09:40]

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

[commit-bot: I haz the power, 2017-03-16 19:09:41]

Dry run: This issue passed the CQ dry run.

[Geoff Lang, 2017-03-16 19:43:03]

https://codereview.chromium.org/2691643002/diff/60001/gpu/command_buffer/test...
File gpu/command_buffer/tests/fuzzer_main.cc (right):

https://codereview.chromium.org/2691643002/diff/60001/gpu/command_buffer/test...
gpu/command_buffer/tests/fuzzer_main.cc:290: surface_ = new gl::GLSurfaceStub;
On 2017/03/16 18:14:33, piman wrote:
> nit: do we need to recreate the surface too? We don't in the other case. I
don't
> think the stub one keeps any useful state, so I think we can just leave this
> out.

Whoops, yes, that should not be re-created.  Fixed.

[piman, 2017-03-16 20:18:24]

lgtm

[Geoff Lang, 2017-03-20 15:45:15]

corpus is uploaded.  Landing.

[Geoff Lang, 2017-03-20 15:45:19]

The CQ bit was checked by geofflang@chromium.org

[Geoff Lang, 2017-03-20 15:45:19]

The patchset sent to the CQ was uploaded after l-g-t-m from mmoroz@chromium.org
Link to the patchset: https://codereview.chromium.org/2691643002/#ps80001
(title: "Fix incorrect surface re-creation")

[commit-bot: I haz the power, 2017-03-20 15:45:47]

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-03-20 17:05:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490024719513950,
"parent_rev": "534a0fda6258fcff015ea6a95f9fba981596c07e", "commit_rev":
"63474f3166f720f61539325c0e592bb4734ccdb5"}

[commit-bot: I haz the power, 2017-03-20 17:11:19]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490024719513950,
"parent_rev": "5045dfe5e3183bf5ca319f55f64eadf5ce64ae31", "commit_rev":
"547de958066823514c34b264b0711bb24eb31dc6"}

[commit-bot: I haz the power, 2017-03-20 17:12:21]

Description was changed from

==========
Add gpu_angle_passthrough_fuzzer.

Tests the passthrough command decoder and the ANGLE null backend.

Reset the context after each iteration. When the same context was
re-used, it lead to crashes that could not be reproduced with the input
file provided. Theoretically, the fuzzer will still find the same
crashes with a single run using one context unless the crash was in the
intialization/destruction code of the command decoder and depended on a
context having specific state.

BUG=602737
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

==========
Add gpu_angle_passthrough_fuzzer.

Tests the passthrough command decoder and the ANGLE null backend.

Reset the context after each iteration. When the same context was
re-used, it lead to crashes that could not be reproduced with the input
file provided. Theoretically, the fuzzer will still find the same
crashes with a single run using one context unless the crash was in the
intialization/destruction code of the command decoder and depended on a
context having specific state.

BUG=602737
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/2691643002
Cr-Commit-Position: refs/heads/master@{#458103}
Committed:
https://chromium.googlesource.com/chromium/src/+/547de958066823514c34b264b071...
==========

[commit-bot: I haz the power, 2017-03-20 17:12:22]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/547de958066823514c34b264b071...