Avoid crash above CFWL_ListItem::GetText()

The issue at hand is caused by a raw pointer rather than a
retained pointer in InheritedData::m_pFontFamily. But the
larger issue is that it's bad to Get() raw pointers from
these, especially when its so cheap to pass them by const
reference.

One reason to Get() a raw pointer is to aid in down-casts, so
add a helper to CFX_RetainPtr to give us downcasted retained
pointers.

BUG=pdfium:665

Change-Id: Ic8624af09664ff603de2e1fda8dbde0cf889f80d
Reviewed-on: https://pdfium-review.googlesource.com/2871
Commit-Queue: dsinclair <dsinclair@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>

9 files changed, 63 insertions, 66 deletions

diff --git a/xfa/fde/css/cfde_csscomputedstyle.cpp b/xfa/fde/css/cfde_csscomputedstyle.cpp
index 50247420be..01872d18fc 100644
--- a/xfa/fde/css/cfde_csscomputedstyle.cpp
+++ b/xfa/fde/css/cfde_csscomputedstyle.cpp
@@ -33,8 +33,8 @@ int32_t CFDE_CSSComputedStyle::CountFontFamilies() const {
 }
 
 const CFX_WideString CFDE_CSSComputedStyle::GetFontFamily(int32_t index) const {
-  return static_cast<CFDE_CSSStringValue*>(
-             m_InheritedData.m_pFontFamily->GetValue(index))
+  return m_InheritedData.m_pFontFamily->GetValue(index)
+      .As<CFDE_CSSStringValue>()
       ->Value();
 }
 
@@ -180,6 +180,8 @@ CFDE_CSSComputedStyle::InheritedData::InheritedData()
       m_eFontStyle(FDE_CSSFontStyle::Normal),
       m_eTextAlign(FDE_CSSTextAlign::Left) {}
 
+CFDE_CSSComputedStyle::InheritedData::~InheritedData() {}
+
 CFDE_CSSComputedStyle::NonInheritedData::NonInheritedData()
     : m_MarginWidth(FDE_CSSLengthUnit::Point, 0),
       m_BorderWidth(FDE_CSSLengthUnit::Point, 0),
diff --git a/xfa/fde/css/cfde_csscomputedstyle.h b/xfa/fde/css/cfde_csscomputedstyle.h
index 92d832eff7..bba4ccbcd4 100644
--- a/xfa/fde/css/cfde_csscomputedstyle.h
+++ b/xfa/fde/css/cfde_csscomputedstyle.h
@@ -21,11 +21,12 @@ class CFDE_CSSComputedStyle : public CFX_Retainable {
   class InheritedData {
    public:
     InheritedData();
+    ~InheritedData();
 
     FDE_CSSLength m_LetterSpacing;
     FDE_CSSLength m_WordSpacing;
     FDE_CSSLength m_TextIndent;
-    CFDE_CSSValueList* m_pFontFamily;
+    CFX_RetainPtr<CFDE_CSSValueList> m_pFontFamily;
     FX_FLOAT m_fFontSize;
     FX_FLOAT m_fLineHeight;
     FX_ARGB m_dwFontColor;
diff --git a/xfa/fde/css/cfde_cssdeclaration.cpp b/xfa/fde/css/cfde_cssdeclaration.cpp
index 481a31e423..3c776ca771 100644
--- a/xfa/fde/css/cfde_cssdeclaration.cpp
+++ b/xfa/fde/css/cfde_cssdeclaration.cpp
@@ -133,12 +133,13 @@ CFDE_CSSDeclaration::CFDE_CSSDeclaration() {}
 
 CFDE_CSSDeclaration::~CFDE_CSSDeclaration() {}
 
-CFDE_CSSValue* CFDE_CSSDeclaration::GetProperty(FDE_CSSProperty eProperty,
-                                                bool& bImportant) const {
+CFX_RetainPtr<CFDE_CSSValue> CFDE_CSSDeclaration::GetProperty(
+    FDE_CSSProperty eProperty,
+    bool* bImportant) const {
   for (const auto& p : properties_) {
     if (p->eProperty == eProperty) {
-      bImportant = p->bImportant;
-      return p->pValue.Get();
+      *bImportant = p->bImportant;
+      return p->pValue;
     }
   }
   return nullptr;
diff --git a/xfa/fde/css/cfde_cssdeclaration.h b/xfa/fde/css/cfde_cssdeclaration.h
index 7d61675bea..eb287308f8 100644
--- a/xfa/fde/css/cfde_cssdeclaration.h
+++ b/xfa/fde/css/cfde_cssdeclaration.h
@@ -34,7 +34,8 @@ class CFDE_CSSDeclaration {
   CFDE_CSSDeclaration();
   ~CFDE_CSSDeclaration();
 
-  CFDE_CSSValue* GetProperty(FDE_CSSProperty eProperty, bool& bImportant) const;
+  CFX_RetainPtr<CFDE_CSSValue> GetProperty(FDE_CSSProperty eProperty,
+                                           bool* bImportant) const;
 
   const_prop_iterator begin() const { return properties_.begin(); }
   const_prop_iterator end() const { return properties_.end(); }
diff --git a/xfa/fde/css/cfde_cssstyleselector.cpp b/xfa/fde/css/cfde_cssstyleselector.cpp
index 1319a64a84..5a7aa1b7ff 100644
--- a/xfa/fde/css/cfde_cssstyleselector.cpp
+++ b/xfa/fde/css/cfde_cssstyleselector.cpp
@@ -22,20 +22,6 @@
 #include "xfa/fde/css/cfde_cssvaluelist.h"
 #include "xfa/fxfa/app/cxfa_csstagprovider.h"
 
-namespace {
-
-template <class T>
-T* ToValue(CFDE_CSSValue* val) {
-  return static_cast<T*>(val);
-}
-
-template <class T>
-const T* ToValue(const CFDE_CSSValue* val) {
-  return static_cast<T*>(val);
-}
-
-}  // namespace
-
 CFDE_CSSStyleSelector::CFDE_CSSStyleSelector(CFGAS_FontMgr* pFontMgr)
     : m_pFontMgr(pFontMgr), m_fDefFontSize(12.0f) {}
 
@@ -122,15 +108,18 @@ void CFDE_CSSStyleSelector::ApplyDeclarations(
 
   for (auto& decl : declArray)
     ExtractValues(decl, &importants, &normals, &customs);
+
   if (extraDecl)
     ExtractValues(extraDecl, &importants, &normals, &customs);
 
   for (auto& prop : normals)
-    ApplyProperty(prop->eProperty, prop->pValue.Get(), pComputedStyle);
+    ApplyProperty(prop->eProperty, prop->pValue, pComputedStyle);
+
   for (auto& prop : customs)
     pComputedStyle->AddCustomStyle(*prop);
+
   for (auto& prop : importants)
-    ApplyProperty(prop->eProperty, prop->pValue.Get(), pComputedStyle);
+    ApplyProperty(prop->eProperty, prop->pValue, pComputedStyle);
 }
 
 void CFDE_CSSStyleSelector::ExtractValues(
@@ -184,7 +173,7 @@ void CFDE_CSSStyleSelector::AppendInlineStyle(CFDE_CSSDeclaration* pDecl,
 
 void CFDE_CSSStyleSelector::ApplyProperty(
     FDE_CSSProperty eProperty,
-    CFDE_CSSValue* pValue,
+    const CFX_RetainPtr<CFDE_CSSValue>& pValue,
     CFDE_CSSComputedStyle* pComputedStyle) {
   if (pValue->GetType() != FDE_CSSPrimitiveType::List) {
     FDE_CSSPrimitiveType eType = pValue->GetType();
@@ -192,21 +181,22 @@ void CFDE_CSSStyleSelector::ApplyProperty(
       case FDE_CSSProperty::Display:
         if (eType == FDE_CSSPrimitiveType::Enum) {
           pComputedStyle->m_NonInheritedData.m_eDisplay =
-              ToDisplay(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+              ToDisplay(pValue.As<CFDE_CSSEnumValue>()->Value());
         }
         break;
       case FDE_CSSProperty::FontSize: {
         FX_FLOAT& fFontSize = pComputedStyle->m_InheritedData.m_fFontSize;
         if (eType == FDE_CSSPrimitiveType::Number) {
-          fFontSize = ToValue<CFDE_CSSNumberValue>(pValue)->Apply(fFontSize);
+          fFontSize = pValue.As<CFDE_CSSNumberValue>()->Apply(fFontSize);
         } else if (eType == FDE_CSSPrimitiveType::Enum) {
-          fFontSize = ToFontSize(ToValue<CFDE_CSSEnumValue>(pValue)->Value(),
-                                 fFontSize);
+          fFontSize =
+              ToFontSize(pValue.As<CFDE_CSSEnumValue>()->Value(), fFontSize);
         }
       } break;
       case FDE_CSSProperty::LineHeight:
         if (eType == FDE_CSSPrimitiveType::Number) {
-          const CFDE_CSSNumberValue* v = ToValue<CFDE_CSSNumberValue>(pValue);
+          CFX_RetainPtr<CFDE_CSSNumberValue> v =
+              pValue.As<CFDE_CSSNumberValue>();
           if (v->Kind() == FDE_CSSNumberType::Number) {
             pComputedStyle->m_InheritedData.m_fLineHeight =
                 v->Value() * pComputedStyle->m_InheritedData.m_fFontSize;
@@ -219,7 +209,7 @@ void CFDE_CSSStyleSelector::ApplyProperty(
       case FDE_CSSProperty::TextAlign:
         if (eType == FDE_CSSPrimitiveType::Enum) {
           pComputedStyle->m_InheritedData.m_eTextAlign =
-              ToTextAlign(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+              ToTextAlign(pValue.As<CFDE_CSSEnumValue>()->Value());
         }
         break;
       case FDE_CSSProperty::TextIndent:
@@ -230,10 +220,10 @@ void CFDE_CSSStyleSelector::ApplyProperty(
       case FDE_CSSProperty::FontWeight:
         if (eType == FDE_CSSPrimitiveType::Enum) {
           pComputedStyle->m_InheritedData.m_wFontWeight =
-              ToFontWeight(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+              ToFontWeight(pValue.As<CFDE_CSSEnumValue>()->Value());
         } else if (eType == FDE_CSSPrimitiveType::Number) {
           int32_t iValue =
-              (int32_t)ToValue<CFDE_CSSNumberValue>(pValue)->Value() / 100;
+              (int32_t)pValue.As<CFDE_CSSNumberValue>()->Value() / 100;
           if (iValue >= 1 && iValue <= 9) {
             pComputedStyle->m_InheritedData.m_wFontWeight = iValue * 100;
           }
@@ -242,13 +232,13 @@ void CFDE_CSSStyleSelector::ApplyProperty(
       case FDE_CSSProperty::FontStyle:
         if (eType == FDE_CSSPrimitiveType::Enum) {
           pComputedStyle->m_InheritedData.m_eFontStyle =
-              ToFontStyle(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+              ToFontStyle(pValue.As<CFDE_CSSEnumValue>()->Value());
         }
         break;
       case FDE_CSSProperty::Color:
         if (eType == FDE_CSSPrimitiveType::RGB) {
           pComputedStyle->m_InheritedData.m_dwFontColor =
-              ToValue<CFDE_CSSColorValue>(pValue)->Value();
+              pValue.As<CFDE_CSSColorValue>()->Value();
         }
         break;
       case FDE_CSSProperty::MarginLeft:
@@ -338,19 +328,19 @@ void CFDE_CSSStyleSelector::ApplyProperty(
       case FDE_CSSProperty::VerticalAlign:
         if (eType == FDE_CSSPrimitiveType::Enum) {
           pComputedStyle->m_NonInheritedData.m_eVerticalAlign =
-              ToVerticalAlign(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+              ToVerticalAlign(pValue.As<CFDE_CSSEnumValue>()->Value());
         } else if (eType == FDE_CSSPrimitiveType::Number) {
           pComputedStyle->m_NonInheritedData.m_eVerticalAlign =
               FDE_CSSVerticalAlign::Number;
           pComputedStyle->m_NonInheritedData.m_fVerticalAlign =
-              ToValue<CFDE_CSSNumberValue>(pValue)->Apply(
+              pValue.As<CFDE_CSSNumberValue>()->Apply(
                   pComputedStyle->m_InheritedData.m_fFontSize);
         }
         break;
       case FDE_CSSProperty::FontVariant:
         if (eType == FDE_CSSPrimitiveType::Enum) {
           pComputedStyle->m_InheritedData.m_eFontVariant =
-              ToFontVariant(ToValue<CFDE_CSSEnumValue>(pValue)->Value());
+              ToFontVariant(pValue.As<CFDE_CSSEnumValue>()->Value());
         }
         break;
       case FDE_CSSProperty::LetterSpacing:
@@ -358,7 +348,7 @@ void CFDE_CSSStyleSelector::ApplyProperty(
           pComputedStyle->m_InheritedData.m_LetterSpacing.Set(
               FDE_CSSLengthUnit::Normal);
         } else if (eType == FDE_CSSPrimitiveType::Number) {
-          if (ToValue<CFDE_CSSNumberValue>(pValue)->Kind() ==
+          if (pValue.As<CFDE_CSSNumberValue>()->Kind() ==
               FDE_CSSNumberType::Percent) {
             break;
           }
@@ -373,7 +363,7 @@ void CFDE_CSSStyleSelector::ApplyProperty(
           pComputedStyle->m_InheritedData.m_WordSpacing.Set(
               FDE_CSSLengthUnit::Normal);
         } else if (eType == FDE_CSSPrimitiveType::Number) {
-          if (ToValue<CFDE_CSSNumberValue>(pValue)->Kind() ==
+          if (pValue.As<CFDE_CSSNumberValue>()->Kind() ==
               FDE_CSSNumberType::Percent) {
             break;
           }
@@ -406,7 +396,7 @@ void CFDE_CSSStyleSelector::ApplyProperty(
         break;
     }
   } else if (pValue->GetType() == FDE_CSSPrimitiveType::List) {
-    CFDE_CSSValueList* pList = ToValue<CFDE_CSSValueList>(pValue);
+    CFX_RetainPtr<CFDE_CSSValueList> pList = pValue.As<CFDE_CSSValueList>();
     int32_t iCount = pList->CountValues();
     if (iCount > 0) {
       switch (eProperty) {
@@ -484,15 +474,16 @@ FDE_CSSFontStyle CFDE_CSSStyleSelector::ToFontStyle(
   }
 }
 
-bool CFDE_CSSStyleSelector::SetLengthWithPercent(FDE_CSSLength& width,
-                                                 FDE_CSSPrimitiveType eType,
-                                                 CFDE_CSSValue* pValue,
-                                                 FX_FLOAT fFontSize) {
+bool CFDE_CSSStyleSelector::SetLengthWithPercent(
+    FDE_CSSLength& width,
+    FDE_CSSPrimitiveType eType,
+    const CFX_RetainPtr<CFDE_CSSValue>& pValue,
+    FX_FLOAT fFontSize) {
   if (eType == FDE_CSSPrimitiveType::Number) {
-    const CFDE_CSSNumberValue* v = ToValue<CFDE_CSSNumberValue>(pValue);
+    CFX_RetainPtr<CFDE_CSSNumberValue> v = pValue.As<CFDE_CSSNumberValue>();
     if (v->Kind() == FDE_CSSNumberType::Percent) {
       width.Set(FDE_CSSLengthUnit::Percent,
-                ToValue<CFDE_CSSNumberValue>(pValue)->Value() / 100.0f);
+                pValue.As<CFDE_CSSNumberValue>()->Value() / 100.0f);
       return width.NonZero();
     }
 
@@ -500,7 +491,7 @@ bool CFDE_CSSStyleSelector::SetLengthWithPercent(FDE_CSSLength& width,
     width.Set(FDE_CSSLengthUnit::Point, fValue);
     return width.NonZero();
   } else if (eType == FDE_CSSPrimitiveType::Enum) {
-    switch (ToValue<CFDE_CSSEnumValue>(pValue)->Value()) {
+    switch (pValue.As<CFDE_CSSEnumValue>()->Value()) {
       case FDE_CSSPropertyValue::Auto:
         width.Set(FDE_CSSLengthUnit::Auto);
         return true;
@@ -572,14 +563,15 @@ FDE_CSSVerticalAlign CFDE_CSSStyleSelector::ToVerticalAlign(
   }
 }
 
-uint32_t CFDE_CSSStyleSelector::ToTextDecoration(CFDE_CSSValueList* pValue) {
+uint32_t CFDE_CSSStyleSelector::ToTextDecoration(
+    const CFX_RetainPtr<CFDE_CSSValueList>& pValue) {
   uint32_t dwDecoration = 0;
   for (int32_t i = pValue->CountValues() - 1; i >= 0; --i) {
-    CFDE_CSSValue* pVal = pValue->GetValue(i);
+    const CFX_RetainPtr<CFDE_CSSValue> pVal = pValue->GetValue(i);
     if (pVal->GetType() != FDE_CSSPrimitiveType::Enum)
       continue;
 
-    switch (ToValue<CFDE_CSSEnumValue>(pVal)->Value()) {
+    switch (pVal.As<CFDE_CSSEnumValue>()->Value()) {
       case FDE_CSSPropertyValue::Underline:
         dwDecoration |= FDE_CSSTEXTDECORATION_Underline;
         break;
diff --git a/xfa/fde/css/cfde_cssstyleselector.h b/xfa/fde/css/cfde_cssstyleselector.h
index b4eaa685ae..c7b6b4164a 100644
--- a/xfa/fde/css/cfde_cssstyleselector.h
+++ b/xfa/fde/css/cfde_cssstyleselector.h
@@ -58,7 +58,7 @@ class CFDE_CSSStyleSelector {
       const CFDE_CSSDeclaration* extraDecl,
       CFDE_CSSComputedStyle* pDestStyle);
   void ApplyProperty(FDE_CSSProperty eProperty,
-                     CFDE_CSSValue* pValue,
+                     const CFX_RetainPtr<CFDE_CSSValue>& pValue,
                      CFDE_CSSComputedStyle* pComputedStyle);
   void ExtractValues(const CFDE_CSSDeclaration* decl,
                      std::vector<const CFDE_CSSPropertyHolder*>* importants,
@@ -67,7 +67,7 @@ class CFDE_CSSStyleSelector {
 
   bool SetLengthWithPercent(FDE_CSSLength& width,
                             FDE_CSSPrimitiveType eType,
-                            CFDE_CSSValue* pValue,
+                            const CFX_RetainPtr<CFDE_CSSValue>& pValue,
                             FX_FLOAT fFontSize);
   FX_FLOAT ToFontSize(FDE_CSSPropertyValue eValue, FX_FLOAT fCurFontSize);
   FDE_CSSDisplay ToDisplay(FDE_CSSPropertyValue eValue);
@@ -75,7 +75,7 @@ class CFDE_CSSStyleSelector {
   uint16_t ToFontWeight(FDE_CSSPropertyValue eValue);
   FDE_CSSFontStyle ToFontStyle(FDE_CSSPropertyValue eValue);
   FDE_CSSVerticalAlign ToVerticalAlign(FDE_CSSPropertyValue eValue);
-  uint32_t ToTextDecoration(CFDE_CSSValueList* pList);
+  uint32_t ToTextDecoration(const CFX_RetainPtr<CFDE_CSSValueList>& pList);
   FDE_CSSFontVariant ToFontVariant(FDE_CSSPropertyValue eValue);
 
   CFGAS_FontMgr* const m_pFontMgr;
diff --git a/xfa/fde/css/cfde_cssstylesheet_unittest.cpp b/xfa/fde/css/cfde_cssstylesheet_unittest.cpp
index c6886428ab..fa73a7a9ba 100644
--- a/xfa/fde/css/cfde_cssstylesheet_unittest.cpp
+++ b/xfa/fde/css/cfde_cssstylesheet_unittest.cpp
@@ -51,19 +51,19 @@ class CFDE_CSSStyleSheetTest : public testing::Test {
     ASSERT(decl_);
 
     bool important;
-    CFDE_CSSValue* v = decl_->GetProperty(prop, important);
+    CFX_RetainPtr<CFDE_CSSValue> v = decl_->GetProperty(prop, &important);
     EXPECT_EQ(v->GetType(), FDE_CSSPrimitiveType::Number);
-    EXPECT_EQ(static_cast<CFDE_CSSNumberValue*>(v)->Kind(), type);
-    EXPECT_EQ(static_cast<CFDE_CSSNumberValue*>(v)->Value(), val);
+    EXPECT_EQ(v.As<CFDE_CSSNumberValue>()->Kind(), type);
+    EXPECT_EQ(v.As<CFDE_CSSNumberValue>()->Value(), val);
   }
 
   void VerifyEnum(FDE_CSSProperty prop, FDE_CSSPropertyValue val) {
     ASSERT(decl_);
 
     bool important;
-    CFDE_CSSValue* v = decl_->GetProperty(prop, important);
+    CFX_RetainPtr<CFDE_CSSValue> v = decl_->GetProperty(prop, &important);
     EXPECT_EQ(v->GetType(), FDE_CSSPrimitiveType::Enum);
-    EXPECT_EQ(static_cast<CFDE_CSSEnumValue*>(v)->Value(), val);
+    EXPECT_EQ(v.As<CFDE_CSSEnumValue>()->Value(), val);
   }
 
   void VerifyList(FDE_CSSProperty prop,
@@ -71,14 +71,14 @@ class CFDE_CSSStyleSheetTest : public testing::Test {
     ASSERT(decl_);
 
     bool important;
-    CFDE_CSSValue* v = decl_->GetProperty(prop, important);
-    CFDE_CSSValueList* list = static_cast<CFDE_CSSValueList*>(v);
+    CFX_RetainPtr<CFDE_CSSValueList> list =
+        decl_->GetProperty(prop, &important).As<CFDE_CSSValueList>();
     EXPECT_EQ(list->CountValues(), pdfium::CollectionSize<int32_t>(values));
 
     for (size_t i = 0; i < values.size(); i++) {
-      CFDE_CSSValue* val = list->GetValue(i);
+      CFX_RetainPtr<CFDE_CSSValue> val = list->GetValue(i);
       EXPECT_EQ(val->GetType(), FDE_CSSPrimitiveType::Enum);
-      EXPECT_EQ(static_cast<CFDE_CSSEnumValue*>(val)->Value(), values[i]);
+      EXPECT_EQ(val.As<CFDE_CSSEnumValue>()->Value(), values[i]);
     }
   }
 
diff --git a/xfa/fde/css/cfde_cssvaluelist.cpp b/xfa/fde/css/cfde_cssvaluelist.cpp
index 64ffb9a174..737ffcb045 100644
--- a/xfa/fde/css/cfde_cssvaluelist.cpp
+++ b/xfa/fde/css/cfde_cssvaluelist.cpp
@@ -20,6 +20,6 @@ int32_t CFDE_CSSValueList::CountValues() const {
   return m_ppList.size();
 }
 
-CFDE_CSSValue* CFDE_CSSValueList::GetValue(int32_t index) const {
-  return m_ppList[index].Get();
+CFX_RetainPtr<CFDE_CSSValue> CFDE_CSSValueList::GetValue(int32_t index) const {
+  return m_ppList[index];
 }
diff --git a/xfa/fde/css/cfde_cssvaluelist.h b/xfa/fde/css/cfde_cssvaluelist.h
index 117b925678..a47f8a3250 100644
--- a/xfa/fde/css/cfde_cssvaluelist.h
+++ b/xfa/fde/css/cfde_cssvaluelist.h
@@ -17,7 +17,7 @@ class CFDE_CSSValueList : public CFDE_CSSValue {
   ~CFDE_CSSValueList() override;
 
   int32_t CountValues() const;
-  CFDE_CSSValue* GetValue(int32_t index) const;
+  CFX_RetainPtr<CFDE_CSSValue> GetValue(int32_t index) const;
 
  protected:
   std::vector<CFX_RetainPtr<CFDE_CSSValue>> m_ppList;