Crash fix for when running under the Android emulator

Crash fix for when running under the Android emulator

For unknown reasons some versions of the Android emulator returns NULL
when asked for GL_SHADING_LANGUAGE_VERSION. Avoid passing NULL to
std::string, which causes undefined behavior.

Committed: https://crrev.com/4af61ccffa3d1af66d2f5c8df0028187ccfadd27
Cr-Commit-Position: refs/heads/master@{#353246}

[davve, 2015-09-21 15:13:45]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-21 15:13:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1358873002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1358873002/1

[davve, 2015-09-21 15:16:58]

davve@opera.com changed reviewers:
+ tobiasjs@chromium.org

[davve, 2015-09-21 15:16:58]

Got crashes on start-up when following:

https://chromium.googlesource.com/chromium/src/+/master/docs/android_test_ins...

Mentioned in

http://code.google.com/p/android/issues/detail?id=78977

that it might be a bug in the emulator.

WDYT?

[commit-bot: I haz the power, 2015-09-21 16:22:50]

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

[commit-bot: I haz the power, 2015-09-21 16:22:51]

Dry run: This issue passed the CQ dry run.

[Tobias Sargeant, 2015-09-22 12:12:04]

LGTM, apart from the comment about glGetError().

https://codereview.chromium.org/1358873002/diff/1/gpu/config/gpu_info_collect...
File gpu/config/gpu_info_collector_android.cc (right):

https://codereview.chromium.org/1358873002/diff/1/gpu/config/gpu_info_collect...
gpu/config/gpu_info_collector_android.cc:220: if (const char*
glsl_version_cstring = reinterpret_cast<const char*>(
Defining glsl_version_cstring in the if seems a little odd to me, but grepping
through the chromium source I can see other instances of this pattern, and I
can't see any comment in the style guide. Probably ok to do this.

In the case where glGetString returns NULL, I would expect a GL error to be set.
You probably want to call glGetError() to clear that error (whether you log it,
or just drop it is probably up to you) so that it doesn't hang around and
potentially cause later code to fail.

[davve, 2015-09-23 10:24:51]

https://codereview.chromium.org/1358873002/diff/1/gpu/config/gpu_info_collect...
File gpu/config/gpu_info_collector_android.cc (right):

https://codereview.chromium.org/1358873002/diff/1/gpu/config/gpu_info_collect...
gpu/config/gpu_info_collector_android.cc:220: if (const char*
glsl_version_cstring = reinterpret_cast<const char*>(
On 2015/09/22 12:12:03, Tobias Sargeant wrote:
> Defining glsl_version_cstring in the if seems a little odd to me, but grepping
> through the chromium source I can see other instances of this pattern, and I
> can't see any comment in the style guide. Probably ok to do this.

I'm open to other suggestions. It felt unnecessary to keep the const char*
variable in scope.
 
> In the case where glGetString returns NULL, I would expect a GL error to be
set.
> You probably want to call glGetError() to clear that error (whether you log
it,
> or just drop it is probably up to you) so that it doesn't hang around and
> potentially cause later code to fail.

While I agree with your logic, glGetError() returns GL_NO_ERROR right after
glGetStringFn returned NULL in this case.

Fwiw, Dart seemed to have encountered the same bug/issue as I have:

https://github.com/MarkBennett/dart/blob/9246c00961f137c2c0cfdfc292eb207f870a...

Note that this isn't from the official dart repo and is quite old. The entire
module it belongs to seems to have been removed on trunk.

What do you think about hard-coding the value in this case?

And sure I could add a glGetError() call, but it would address a very
theoretical problem.

[Tobias Sargeant, 2015-09-23 10:51:38]

On 2015/09/23 10:24:51, David Vest wrote:
> While I agree with your logic, glGetError() returns GL_NO_ERROR right after
> glGetStringFn returned NULL in this case.

Ok, in that case I think it's fine to land as is.

[davve, 2015-09-23 11:34:07]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2015-09-23 11:34:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1358873002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1358873002/1

[commit-bot: I haz the power, 2015-09-23 11:44:05]

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

[commit-bot: I haz the power, 2015-09-23 11:44:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Tobias Sargeant, 2015-09-23 11:49:19]

tobiasjs@chromium.org changed reviewers:
+ sievers@chromium.org

[davve, 2015-10-07 11:48:58]

On 2015/09/23 11:44:06, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

sievers, could you please take a look? Might be that the emulator is/will be
fixed but this happened to me only two weeks ago using the mentioned wiki
instructions. Not sure why others haven't already noticed, though.

[no sievers, 2015-10-07 17:18:15]

On 2015/10/07 11:48:58, David Vest wrote:
> On 2015/09/23 11:44:06, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> sievers, could you please take a look? Might be that the emulator is/will be
> fixed but this happened to me only two weeks ago using the mentioned wiki
> instructions. Not sure why others haven't already noticed, though.

lgtm. i wonder if it also generates a GL error then, which might leak elsewhere.

[davve, 2015-10-09 06:46:47]

On 2015/10/07 17:18:15, sievers wrote:

> lgtm. i wonder if it also generates a GL error then, which might leak
elsewhere.

Tobias was worried about this too so I checked and it didn't. See
https://codereview.chromium.org/1358873002/#msg8 for my "investigation".

[davve, 2015-10-09 06:48:09]

The CQ bit was checked by davve@opera.com

[davve, 2015-10-09 06:48:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
tobiasjs@chromium.org
Link to the patchset: https://codereview.chromium.org/1358873002/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-09 06:50:09]

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

[commit-bot: I haz the power, 2015-10-09 07:51:33]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-09 07:52:30]

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