1125084 - Uninitialised value use in mozilla::hal_impl::SetScreenBrightness(double)

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Julian Seward [:jseward]]

This occurs when starting up b2g on Nexus 5.  I haven't tested Flame (yet).

Problem is in gecko/hal/gonk/GonkHal.cpp.  It appears that GetKeyLightEnabled()
returns an undefined value.  It looks like this:

  bool
  GetKeyLightEnabled()
  {
    LightConfiguration config;
    GetLight(hal::eHalLightID_Buttons, &config);
    return (config.color != 0x00000000);
  }

The call to GetLight is intended to write values into |config|, but
in fact GetLight returns a |bool| indicating success/failure, which
this routine ignores.  So it just assumes that |config| got filled
in, which isn't true.

There's a second call point in this file to GetLight, that also 
ignores the return result, in GetScreenBrightness.

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind complaints

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1125084-1.diff

Adds (ultra-trivial) handling of failure of GetLight().  mwu, what are
suitable values for GetKeyLightEnabled() and GetScreenBrightness() in case
of failure of GetLight() ?

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 8610580 [details] [diff] [review]
bug1125084-1.diff

Review of attachment 8610580 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/gonk/GonkHal.cpp
@@ +767,5 @@
> +  bool ok = GetLight(eHalLightID_Buttons, &config);
> +  if (ok) {
> +    return (config.color != 0x00000000);
> +  } else {
> +    return false;

This seems ok to me.

@@ +807,5 @@
> +    // backlight is brightness only, so using one of the RGB elements as value.
> +    int brightness = config.color & 0xFF;
> +    return brightness / 255.0;
> +  } else {
> +    return 0.5;

The right thing to do here might be to crash. Alternately, we can cache the last brightness setting and return that.

Comment 4

[Dave Hylands [:dhylands]]

Comment on attachment 8610580 [details] [diff] [review]
bug1125084-1.diff

Review of attachment 8610580 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple minor nits.

::: hal/gonk/GonkHal.cpp
@@ +766,5 @@
>    LightConfiguration config;
> +  bool ok = GetLight(eHalLightID_Buttons, &config);
> +  if (ok) {
> +    return (config.color != 0x00000000);
> +  } else {

nit: lose the else, unindent the return

@@ +806,5 @@
> +  if (ok) {
> +    // backlight is brightness only, so using one of the RGB elements as value.
> +    int brightness = config.color & 0xFF;
> +    return brightness / 255.0;
> +  } else {

nit: lose the else

@@ +807,5 @@
> +    // backlight is brightness only, so using one of the RGB elements as value.
> +    int brightness = config.color & 0xFF;
> +    return brightness / 255.0;
> +  } else {
> +    return 0.5;

I think that if GetLight fails, then its because that light doesn't exist, so returning a cached value doesn't make sense. I think I'd just return 0 (or whatever value corresponds to off).

Comment 5

[Julian Seward [:jseward]]

Attachment: bug1125084-2.diff

Addresses review comments in comment 3 and comment 4.

Comment 6

[Dave Hylands [:dhylands]]

Comment on attachment 8612263 [details] [diff] [review]
bug1125084-2.diff

Review of attachment 8612263 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14a44154018a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14a44154018a