AmbientLightSensor: Ignore readings received before sensor activation.

This follows what Sensor::OnSensorReadingChanged() already does, but which
we were missing in the AmbientLightSensor override.

Platform sensor initialization happens before the sensor infrastructure on
the Blink side transitions reaches Sensor::NotifyActivated() and transitions
to the SensorState::kActivated state.

In practice, this means we can start receiving readings too early, and in
the Ambient Light Sensor case it meant setting |latest_reading_| to an
actual value but never emitting a "reading" event, so users would never get
notified of the first reading after starting the sensor (which also
contradicts the spec).

To make it worse, we have privacy measures in place that require illuminance
readings to differ by a certain threshold before we emit a reading event, so
if the user's surroundings did not change no reading event would ever be
emitted at all, even though accessing the "illuminance" attribute in JS
would actually return a rounded version of the first reading the platform
sensor sent during initialization.

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

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

@@ -67,6 +67,12 @@ double AmbientLightSensor::illuminance(bool& is_null) const {
 // value, we discard this reading and do not emit any events. This is a privacy
 // measure to avoid giving readings that are too specific.
 void AmbientLightSensor::OnSensorReadingChanged() {
+  // The platform sensor may start sending readings before the sensor is fully
+  // activated on the Blink side. In this case, bail out early, otherwise we
+  // will set |latest_reading_| and not send a "reading" event.
+  if (!IsActivated())
+    return;
+
   const double new_reading = GetReading().als.value;
   if (latest_reading_.has_value() &&
       !IsSignificantlyDifferent(*latest_reading_, new_reading)) {

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

@@ -31,6 +31,8 @@ class MODULES_EXPORT AmbientLightSensor final : public Sensor {
   base::Optional<double> latest_reading_;
 
   FRIEND_TEST_ALL_PREFIXES(AmbientLightSensorTest, IlluminanceRounding);
+  FRIEND_TEST_ALL_PREFIXES(AmbientLightSensorTest,
+                           PlatformSensorReadingsBeforeActivation);
 };
 
 }  // namespace blink

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

@@ -26,17 +26,34 @@ class MockSensorProxyObserver
 
   // Synchronously waits for OnSensorReadingChanged() to be called.
   void WaitForOnSensorReadingChanged() {
-    run_loop_.emplace();
-    run_loop_->Run();
+    sensor_reading_changed_run_loop_.emplace();
+    sensor_reading_changed_run_loop_->Run();
+  }
+
+  // Synchronously waits for OnSensorInitialized() to be called.
+  void WaitForSensorInitialization() {
+    sensor_initialized_run_loop_.emplace();
+    sensor_initialized_run_loop_->Run();
+  }
+
+  // SensorProxy::Observer overrides.
+  void OnSensorInitialized() override {
+    if (sensor_initialized_run_loop_.has_value() &&
+        sensor_initialized_run_loop_->running()) {
+      sensor_initialized_run_loop_->Quit();
+    }
   }
 
   void OnSensorReadingChanged() override {
-    DCHECK(run_loop_.has_value() && run_loop_->running());
-    run_loop_->Quit();
+    if (sensor_reading_changed_run_loop_.has_value() &&
+        sensor_reading_changed_run_loop_->running()) {
+      sensor_reading_changed_run_loop_->Quit();
+    }
   }
 
  private:
-  base::Optional<base::RunLoop> run_loop_;
+  base::Optional<base::RunLoop> sensor_initialized_run_loop_;
+  base::Optional<base::RunLoop> sensor_reading_changed_run_loop_;
 };
 
 }  // namespace
@@ -149,4 +166,41 @@ TEST(AmbientLightSensorTest, IlluminanceRounding) {
   EXPECT_EQ(3U, event_counter->event_count());
 }
 
+TEST(AmbientLightSensorTest, PlatformSensorReadingsBeforeActivation) {
+  SensorTestContext context;
+  NonThrowableExceptionState exception_state;
+
+  auto* sensor = AmbientLightSensor::Create(context.GetExecutionContext(),
+                                            exception_state);
+  sensor->start();
+
+  auto* sensor_proxy =
+      SensorProviderProxy::From(To<Document>(context.GetExecutionContext()))
+          ->GetSensorProxy(device::mojom::blink::SensorType::AMBIENT_LIGHT);
+  ASSERT_NE(sensor_proxy, nullptr);
+  auto* mock_observer = MakeGarbageCollected<MockSensorProxyObserver>();
+  sensor_proxy->AddObserver(mock_observer);
+
+  bool illuminance_is_null;
+
+  // Instead of waiting for SensorProxy::Observer::OnSensorReadingChanged(), we
+  // wait for OnSensorInitialized(), which happens earlier. The platform may
+  // start sending readings and calling OnSensorReadingChanged() at any moment
+  // from this point on.
+  // This test verifies AmbientLightSensor::OnSensorReadingChanged() is able to
+  // handle the case of it being called before Sensor itself has transitioned to
+  // a fully activated state.
+  mock_observer->WaitForSensorInitialization();
+  context.sensor_provider()->UpdateAmbientLightSensorData(42);
+  ASSERT_FALSE(sensor->IsActivated());
+  EXPECT_EQ(0, sensor->illuminance(illuminance_is_null));
+  EXPECT_TRUE(illuminance_is_null);
+
+  SensorTestUtils::WaitForEvent(sensor, event_type_names::kReading);
+
+  EXPECT_EQ(42, sensor->latest_reading_);
+  EXPECT_EQ(50, sensor->illuminance(illuminance_is_null));
+  EXPECT_FALSE(illuminance_is_null);
+}
+
 }  // namespace blink