AmbientLightSensor: Take |latest_reading_| into account in hasReading().

We were occasionally hitting a DCHECK in AmbientLightSensor's illuminance
accessor after commit 2f0fc46c ("[sensors] Round off Ambient Light
Sensor readouts to the nearest 50 Lux") when running the Generic Sensors
web tests.

We assumed that if Sensor::hasReading() returns true, then
Sensor::OnSensorReadingChanged() would have been called first and
|latest_reading_| would always be set. It turns out there is a small window
when this is not true, when one sensor has been fully initialized by the
time it received a "reading" event, but other sensors of the same type might
not have had OnSensorReadingChanged() be called on them yet.

Fix it by overriding Sensor::hasReading() in AmbientLightSensor: an ALS'
definition of "having a reading" depends on whether |latest_reading_| has
already been set (which means OnSensorReadingChanged() has been called and
the value has been properly processed).

Bug: 606766, 1078172
Change-Id: I115233e745b402e97e4223fffd4d68a4350451ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2199287
Commit-Queue: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Auto-Submit: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#772187}

third_party/blink/renderer/modules/sensor/ambient_light_sensor.cc

@@ -57,6 +57,10 @@ AmbientLightSensor::AmbientLightSensor(ExecutionContext* execution_context,
              SensorType::AMBIENT_LIGHT,
              {mojom::blink::FeaturePolicyFeature::kAmbientLightSensor}) {}
 
+bool AmbientLightSensor::hasReading() const {
+  return latest_reading_.has_value() && Sensor::hasReading();
+}
+
 base::Optional<double> AmbientLightSensor::illuminance() const {
   if (hasReading()) {
     DCHECK(latest_reading_.has_value());

third_party/blink/renderer/modules/sensor/ambient_light_sensor.h

@@ -23,6 +23,7 @@ class MODULES_EXPORT AmbientLightSensor final : public Sensor {
 
   AmbientLightSensor(ExecutionContext*, const SensorOptions*, ExceptionState&);
 
+  bool hasReading() const override;
   base::Optional<double> illuminance() const;
 
   void OnSensorReadingChanged() override;

third_party/blink/renderer/modules/sensor/ambient_light_sensor_test.cc

@@ -188,13 +188,17 @@ TEST(AmbientLightSensorTest, PlatformSensorReadingsBeforeActivation) {
   mock_observer->WaitForSensorInitialization();
   context.sensor_provider()->UpdateAmbientLightSensorData(42);
   ASSERT_FALSE(sensor->activated());
+  EXPECT_FALSE(sensor->hasReading());
   EXPECT_FALSE(sensor->illuminance().has_value());
+  EXPECT_FALSE(sensor->timestamp(context.GetScriptState()).has_value());
 
   SensorTestUtils::WaitForEvent(sensor, event_type_names::kReading);
 
   EXPECT_EQ(42, sensor->latest_reading_);
+  EXPECT_TRUE(sensor->hasReading());
   ASSERT_TRUE(sensor->illuminance().has_value());
   EXPECT_EQ(50, sensor->illuminance().value());
+  EXPECT_TRUE(sensor->timestamp(context.GetScriptState()).has_value());
 }
 
 }  // namespace blink

third_party/blink/renderer/modules/sensor/sensor.cc

@@ -346,7 +346,12 @@ void Sensor::NotifyActivated() {
   DCHECK_EQ(state_, SensorState::kActivating);
   state_ = SensorState::kActivated;
 
-  if (hasReading()) {
+  // Explicitly call the Sensor implementation of hasReading(). Subclasses may
+  // override the method and introduce additional requirements, but in this case
+  // we are really only interested in whether there is data in the shared
+  // buffer, so that we can then process it possibly for the first time in
+  // OnSensorReadingChanged().
+  if (Sensor::hasReading()) {
     // If reading has already arrived, process the reading values (a subclass
     // may do some filtering, for example) and then send an initial "reading"
     // event right away.

third_party/blink/renderer/modules/sensor/sensor.h

@@ -53,7 +53,7 @@ class MODULES_EXPORT Sensor : public EventTargetWithInlineData,
 
   // Getters
   bool activated() const;
-  bool hasReading() const;
+  virtual bool hasReading() const;
   base::Optional<DOMHighResTimeStamp> timestamp(ScriptState*) const;
 
   DEFINE_ATTRIBUTE_EVENT_LISTENER(error, kError)

third_party/blink/renderer/modules/sensor/sensor_test_utils.cc

@@ -13,6 +13,7 @@
 #include "third_party/blink/renderer/core/page/focus_controller.h"
 #include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/modules/sensor/sensor_test_utils.h"
+#include "third_party/blink/renderer/platform/bindings/script_state.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
 
 namespace blink {
@@ -61,6 +62,10 @@ ExecutionContext* SensorTestContext::GetExecutionContext() const {
   return testing_scope_.GetExecutionContext();
 }
 
+ScriptState* SensorTestContext::GetScriptState() const {
+  return testing_scope_.GetScriptState();
+}
+
 void SensorTestContext::BindSensorProviderRequest(
     mojo::ScopedMessagePipeHandle handle) {
   sensor_provider_.Bind(

third_party/blink/renderer/modules/sensor/sensor_test_utils.h

@@ -16,6 +16,7 @@ namespace blink {
 
 class EventTarget;
 class ExecutionContext;
+class ScriptState;
 
 class SensorTestContext final {
   STACK_ALLOCATED();
@@ -26,6 +27,7 @@ class SensorTestContext final {
   ~SensorTestContext();
 
   ExecutionContext* GetExecutionContext() const;
+  ScriptState* GetScriptState() const;
 
   device::FakeSensorProvider* sensor_provider() { return &sensor_provider_; }
 

third_party/blink/web_tests/wpt_internal/ambient-light/AmbientLightSensor.https.html

@@ -36,4 +36,50 @@ runGenericSensorTests(
     verifyAlsSensorReading,
     ['ambient-light-sensor']);
 
+// Regression test for https://crbug.com/1078172.
+// If we create 2 sensors, S1 and S2, by the time we dispatch a "reading" event
+// for S1 the shared memory buffer used by both S1 and S2 will have data, but
+// S2's AmbientLightSensor::OnSensorReadingChanged() has not been called yet.
+// This test checks the code does not crash if we reach
+// AmbientLightSensor::illuminance() during this window, and that other values
+// are still null or false.
+// TODO: This could be generalized and added to runGenericSensorTests().
+sensor_test(async (t, sensorProvider) => {
+  const sensor1 = new AmbientLightSensor();
+  const sensor2 = new AmbientLightSensor();
+
+  return new Promise((resolve, reject) => {
+    sensor1.addEventListener('reading', () => {
+      sensor2.addEventListener('activate', () => {
+        try {
+          assert_true(sensor1.activated);
+          assert_true(sensor1.hasReading);
+          assert_not_equals(sensor1.illuminance, null);
+          assert_not_equals(sensor1.timestamp, null);
+
+          assert_true(sensor2.activated);
+          assert_false(sensor2.hasReading);
+          assert_equals(sensor2.illuminance, null);
+          assert_equals(sensor2.timestamp, null);
+        } catch (e) {
+          reject(e);
+        }
+      }, { once: true });
+      sensor2.addEventListener('reading', () => {
+        try {
+          assert_true(sensor2.activated);
+          assert_true(sensor2.hasReading);
+          assert_equals(sensor1.illuminance, sensor2.illuminance);
+          assert_equals(sensor1.timestamp, sensor2.timestamp);
+          resolve();
+        } catch (e) {
+          reject(e);
+        }
+      }, { once: true });
+      sensor2.start();
+    }, { once: true });
+    sensor1.start();
+  });
+}, "Accessing illuminance when a sensor is still activating does not crash.");
+
 </script>