Late delegate setting to avoid init order issues

When a Property is created, initialized in the constructor
initialization list, and uses 'this' as the delegate, there can
be an ordering issue where the delegate gets called that can use the
Property before it is initialized, possibly causing a crash.

The recommended method to initialize a Property of this type is
to create the Property without the delegate and then in the
Constructor body, after the object is fully initialized, set
the delegate.  This will follow the same execution path as setting
the delegate in the initialization list, including calling the
appropriate WasWritten method, but eliminates the possibility
the Property will be used before it is initialized.

BUG=b:253585974
TEST=USE="coverage" FEATURES="test noclean" emerge-brya chromeos-base/gestures

Change-Id: Ia658ea903656d5ad6be638ca0d49b169cfe3127c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4286619
Code-Coverage: Kenneth Albanowski <kenalba@google.com>
Commit-Queue: Kenneth Albanowski <kenalba@google.com>
Reviewed-by: Kenneth Albanowski <kenalba@google.com>
Auto-Submit: Denis Brockus <dbrockus@chromium.org>
Tested-by: Denis Brockus <dbrockus@chromium.org>

5 files changed, 47 insertions, 8 deletions

diff --git a/include/iir_filter_interpreter.h b/include/iir_filter_interpreter.h
index 156a331..db940b8 100644
--- a/include/iir_filter_interpreter.h
+++ b/include/iir_filter_interpreter.h
@@ -79,9 +79,11 @@ class IirFilterInterpreter : public FilterInterpreter, public PropertyDelegate {
   virtual void DoubleWasWritten(DoubleProperty* prop);
 
  private:
-  // Whether IIR filter should be used. Put as a member varible for
+  // Whether IIR filter should be used. Put as a member variable for
   // unittest purpose.
   bool using_iir_;
+
+  // Sync state history information
   std::map<short, IoHistory> histories_;
 
   // y[0] = b[0]*x[0] + b[1]*x[1] + b[2]*x[2] + b[3]*x[3]
diff --git a/include/prop_registry.h b/include/prop_registry.h
index 8d57de1..836b98b 100644
--- a/include/prop_registry.h
+++ b/include/prop_registry.h
@@ -59,6 +59,19 @@ class Property {
   virtual void CreatePropImpl() = 0;
   void DestroyProp();
 
+  // b/253585974
+  // delegate used to get passed as a constructor parameter but that
+  // led to a chance that the delegate was 'this' and setting the
+  // delegate to 'this' in the initializer list allowed the property
+  // creation to use 'this' before it was initialized.  This could
+  // lead to unexpected behavior and if you were lucky to a crash.
+  //
+  // Now the delegate is always NULL on initilization of the property
+  // instance and after the delegate and property are completely
+  // created the user should set the delegate in the constructor
+  // body. This will allow access to this in a safe manner.
+  void SetDelegate(PropertyDelegate* delegate);
+
   const char* name() { return name_; }
   // Returns a newly allocated Value object
   virtual Json::Value NewValue() const = 0;
diff --git a/src/iir_filter_interpreter.cc b/src/iir_filter_interpreter.cc
index 3b9d4a0..0558ef6 100644
--- a/src/iir_filter_interpreter.cc
+++ b/src/iir_filter_interpreter.cc
@@ -33,14 +33,21 @@ IirFilterInterpreter::IirFilterInterpreter(PropRegistry* prop_reg,
                                            Tracer* tracer)
     : FilterInterpreter(NULL, next, tracer, false),
       using_iir_(true),
-      b0_(prop_reg, "IIR b0", 0.0674552738890719, this),
-      b1_(prop_reg, "IIR b1", 0.134910547778144, this),
-      b2_(prop_reg, "IIR b2", 0.0674552738890719, this),
-      b3_(prop_reg, "IIR b3", 0.0, this),
-      a1_(prop_reg, "IIR a1", -1.1429805025399, this),
-      a2_(prop_reg, "IIR a2", 0.412801598096189, this),
-      iir_dist_thresh_(prop_reg, "IIR Distance Threshold", 10, this),
+      b0_(prop_reg, "IIR b0", 0.0674552738890719),
+      b1_(prop_reg, "IIR b1", 0.134910547778144),
+      b2_(prop_reg, "IIR b2", 0.0674552738890719),
+      b3_(prop_reg, "IIR b3", 0.0),
+      a1_(prop_reg, "IIR a1", -1.1429805025399),
+      a2_(prop_reg, "IIR a2", 0.412801598096189),
+      iir_dist_thresh_(prop_reg, "IIR Distance Threshold", 10),
       adjust_iir_on_warp_(prop_reg, "Adjust IIR History On Warp", false) {
+  b0_.SetDelegate(this);
+  b1_.SetDelegate(this);
+  b2_.SetDelegate(this);
+  b3_.SetDelegate(this);
+  a1_.SetDelegate(this);
+  a2_.SetDelegate(this);
+  iir_dist_thresh_.SetDelegate(this);
   InitName();
 }
 
diff --git a/src/prop_registry.cc b/src/prop_registry.cc
index 2d9d454..58e3a1a 100644
--- a/src/prop_registry.cc
+++ b/src/prop_registry.cc
@@ -70,6 +70,18 @@ void Property::DestroyProp() {
   gprop_ = NULL;
 }
 
+void Property::SetDelegate(PropertyDelegate* delegate) {
+  bool delegate_was_late_set = !delegate_ && delegate;
+  delegate_ = delegate;
+  // In the normal path of creation of a property with a parent, the
+  // delegate is set late and the CreatePropImpl will not call the
+  // WasWritten method.  So catch the transition from NULL to not
+  // NULL in this condition and make sure to call the initial
+  // WasWritten to mimick the original code.
+  if (delegate_was_late_set && parent_ && parent_->PropProvider())
+    HandleGesturesPropWritten();
+}
+
 void BoolProperty::CreatePropImpl() {
   GesturesPropBool orig_val = val_;
   gprop_ = parent_->PropProvider()->create_bool_fn(
diff --git a/src/prop_registry_unittest.cc b/src/prop_registry_unittest.cc
index 66a7f6c..23ba9d8 100644
--- a/src/prop_registry_unittest.cc
+++ b/src/prop_registry_unittest.cc
@@ -203,6 +203,11 @@ TEST(PropRegistryTest, SetAtCreateShouldNotifyTest) {
   EXPECT_EQ(0, delegate.call_cnt_);
   reg.SetPropProvider(&mock_gestures_props_provider, NULL);
   EXPECT_EQ(4, delegate.call_cnt_);
+
+  BoolProperty my_bool2(&reg, "MyBool2", 1);
+  EXPECT_EQ(4, delegate.call_cnt_);
+  my_bool2.SetDelegate(&delegate);
+  EXPECT_EQ(5, delegate.call_cnt_);
 }
 
 TEST(PropRegistryTest, DoublePromoteIntTest) {