Changeset 201690 in webkit


Ignore:
Timestamp:
Jun 4, 2016 4:02:48 PM (8 years ago)
Author:
Darin Adler
Message:

leaks seen in fast/css/variables tests
​https://bugs.webkit.org/show_bug.cgi?id=150728

Reviewed by Anders Carlsson.

Fixes leaks seen running fast/css/variables tests with leak checking turned on.

  • css/CSSPrimitiveValue.cpp:

(WebCore::isStringType): Added. For debugging purposes so we catch cases where we
are not treating strings consistently between construction and destruction.
(WebCore::CSSPrimitiveValue::CSSPrimitiveValue): Assert isStringType returns true.
(WebCore::CSSPrimitiveValue::cleanup): Added CSS_DIMENSION and CSS_PARSER_IDENTIFIER
to the list of types that have to decrement the reference count of the string we own.
Both types are passed to the string constructor above.

  • css/CSSValueList.cpp:

(WebCore::CSSValueList::buildParserValueListSubstitutingVariables): Restructured the
code so we destroy any CSSParserValue that we don't use. This is needed because of the
peculiar requirements of CSSParserValue: it has a be a struct without a destructor so
it can be used in the CSS grammar, so we have to destroy it explicitly. Ideally we would
minimize any use of it outside the CSSParser itself, but as long as we are using it, we
need to do this explicit destruction.

Location:
trunk/Source/WebCore
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r201687 r201690  
     12016-06-04  Darin Adler  <darin@apple.com>
     2
     3        leaks seen in fast/css/variables tests
     4        https://bugs.webkit.org/show_bug.cgi?id=150728
     5
     6        Reviewed by Anders Carlsson.
     7
     8        Fixes leaks seen running fast/css/variables tests with leak checking turned on.
     9
     10        * css/CSSPrimitiveValue.cpp:
     11        (WebCore::isStringType): Added. For debugging purposes so we catch cases where we
     12        are not treating strings consistently between construction and destruction.
     13        (WebCore::CSSPrimitiveValue::CSSPrimitiveValue): Assert isStringType returns true.
     14        (WebCore::CSSPrimitiveValue::cleanup): Added CSS_DIMENSION and CSS_PARSER_IDENTIFIER
     15        to the list of types that have to decrement the reference count of the string we own.
     16        Both types are passed to the string constructor above.
     17
     18        * css/CSSValueList.cpp:
     19        (WebCore::CSSValueList::buildParserValueListSubstitutingVariables): Restructured the
     20        code so we destroy any CSSParserValue that we don't use. This is needed because of the
     21        peculiar requirements of CSSParserValue: it has a be a struct without a destructor so
     22        it can be used in the CSS grammar, so we have to destroy it explicitly. Ideally we would
     23        minimize any use of it outside the CSSParser itself, but as long as we are using it, we
     24        need to do this explicit destruction.
     25
    1262016-06-04  Anders Carlsson  <andersca@apple.com>
    227

  • trunk/Source/WebCore/css/CSSPrimitiveValue.cpp

    r201290 r201690  
    6565    switch (unitType) {
    6666    case CSSPrimitiveValue::CSS_CALC:
     67    case CSSPrimitiveValue::CSS_CALC_PERCENTAGE_WITH_LENGTH:
    6768    case CSSPrimitiveValue::CSS_CALC_PERCENTAGE_WITH_NUMBER:
    68     case CSSPrimitiveValue::CSS_CALC_PERCENTAGE_WITH_LENGTH:
     69    case CSSPrimitiveValue::CSS_CHS:
    6970    case CSSPrimitiveValue::CSS_CM:
    7071    case CSSPrimitiveValue::CSS_DEG:
    7172    case CSSPrimitiveValue::CSS_DIMENSION:
    72 #if ENABLE(CSS_IMAGE_RESOLUTION) || ENABLE(RESOLUTION_MEDIA_QUERY)
    73     case CSSPrimitiveValue::CSS_DPPX:
    74     case CSSPrimitiveValue::CSS_DPI:
    75     case CSSPrimitiveValue::CSS_DPCM:
    76 #endif
    7773    case CSSPrimitiveValue::CSS_EMS:
    7874    case CSSPrimitiveValue::CSS_EXS:
     75    case CSSPrimitiveValue::CSS_FR:
    7976    case CSSPrimitiveValue::CSS_GRAD:
    8077    case CSSPrimitiveValue::CSS_HZ:
    … …  
    8481    case CSSPrimitiveValue::CSS_MS:
    8582    case CSSPrimitiveValue::CSS_NUMBER:
     83    case CSSPrimitiveValue::CSS_PC:
    8684    case CSSPrimitiveValue::CSS_PERCENTAGE:
    87     case CSSPrimitiveValue::CSS_PC:
    8885    case CSSPrimitiveValue::CSS_PT:
    8986    case CSSPrimitiveValue::CSS_PX:
    9087    case CSSPrimitiveValue::CSS_RAD:
    9188    case CSSPrimitiveValue::CSS_REMS:
    92     case CSSPrimitiveValue::CSS_CHS:
    9389    case CSSPrimitiveValue::CSS_S:
    9490    case CSSPrimitiveValue::CSS_TURN:
     91    case CSSPrimitiveValue::CSS_VH:
     92    case CSSPrimitiveValue::CSS_VMAX:
     93    case CSSPrimitiveValue::CSS_VMIN:
    9594    case CSSPrimitiveValue::CSS_VW:
    96     case CSSPrimitiveValue::CSS_VH:
    97     case CSSPrimitiveValue::CSS_VMIN:
    98     case CSSPrimitiveValue::CSS_VMAX:
    99     case CSSPrimitiveValue::CSS_FR:
    10095        return true;
     96    case CSSPrimitiveValue::CSS_DPCM:
     97    case CSSPrimitiveValue::CSS_DPI:
     98    case CSSPrimitiveValue::CSS_DPPX:
     99#if ENABLE(CSS_IMAGE_RESOLUTION) || ENABLE(RESOLUTION_MEDIA_QUERY)
     100        return true;
     101#else
     102        return false;
     103#endif
    101104    case CSSPrimitiveValue::CSS_ATTR:
    102105    case CSSPrimitiveValue::CSS_COUNTER:
    103106    case CSSPrimitiveValue::CSS_COUNTER_NAME:
    104 #if ENABLE(DASHBOARD_SUPPORT)
    105     case CSSPrimitiveValue::CSS_DASHBOARD_REGION:
    106 #endif
    107 #if !ENABLE(CSS_IMAGE_RESOLUTION) && !ENABLE(RESOLUTION_MEDIA_QUERY)
    108     case CSSPrimitiveValue::CSS_DPPX:
    109     case CSSPrimitiveValue::CSS_DPI:
    110     case CSSPrimitiveValue::CSS_DPCM:
    111 #endif
     107    case CSSPrimitiveValue::CSS_FONT_FAMILY:
    112108    case CSSPrimitiveValue::CSS_IDENT:
    113     case CSSPrimitiveValue::CSS_PROPERTY_ID:
    114     case CSSPrimitiveValue::CSS_VALUE_ID:
    115109    case CSSPrimitiveValue::CSS_PAIR:
    116110    case CSSPrimitiveValue::CSS_PARSER_HEXCOLOR:
    … …  
    119113    case CSSPrimitiveValue::CSS_PARSER_OPERATOR:
    120114    case CSSPrimitiveValue::CSS_PARSER_WHITESPACE:
     115    case CSSPrimitiveValue::CSS_PROPERTY_ID:
     116    case CSSPrimitiveValue::CSS_QUAD:
    121117    case CSSPrimitiveValue::CSS_RECT:
    122     case CSSPrimitiveValue::CSS_QUAD:
    123 #if ENABLE(CSS_SCROLL_SNAP)
    124     case CSSPrimitiveValue::CSS_LENGTH_REPEAT:
    125 #endif
    126118    case CSSPrimitiveValue::CSS_RGBCOLOR:
    127119    case CSSPrimitiveValue::CSS_SHAPE:
    128120    case CSSPrimitiveValue::CSS_STRING:
    129     case CSSPrimitiveValue::CSS_FONT_FAMILY:
    130121    case CSSPrimitiveValue::CSS_UNICODE_RANGE:
    131122    case CSSPrimitiveValue::CSS_UNKNOWN:
    132123    case CSSPrimitiveValue::CSS_URI:
     124    case CSSPrimitiveValue::CSS_VALUE_ID:
     125#if ENABLE(CSS_SCROLL_SNAP)
     126    case CSSPrimitiveValue::CSS_LENGTH_REPEAT:
     127#endif
     128#if ENABLE(DASHBOARD_SUPPORT)
     129    case CSSPrimitiveValue::CSS_DASHBOARD_REGION:
     130#endif
    133131        return false;
    134132    }
    … …  
    137135    return false;
    138136}
     137
     138#if !ASSERT_DISABLED
     139
     140static inline bool isStringType(CSSPrimitiveValue::UnitTypes type)
     141{
     142    switch (type) {
     143    case CSSPrimitiveValue::CSS_STRING:
     144    case CSSPrimitiveValue::CSS_URI:
     145    case CSSPrimitiveValue::CSS_ATTR:
     146    case CSSPrimitiveValue::CSS_COUNTER_NAME:
     147    case CSSPrimitiveValue::CSS_DIMENSION:
     148    case CSSPrimitiveValue::CSS_PARSER_HEXCOLOR:
     149    case CSSPrimitiveValue::CSS_PARSER_IDENTIFIER:
     150    case CSSPrimitiveValue::CSS_PARSER_WHITESPACE:
     151        return true;
     152    case CSSPrimitiveValue::CSS_CALC:
     153    case CSSPrimitiveValue::CSS_CALC_PERCENTAGE_WITH_LENGTH:
     154    case CSSPrimitiveValue::CSS_CALC_PERCENTAGE_WITH_NUMBER:
     155    case CSSPrimitiveValue::CSS_CHS:
     156    case CSSPrimitiveValue::CSS_CM:
     157    case CSSPrimitiveValue::CSS_COUNTER:
     158    case CSSPrimitiveValue::CSS_DEG:
     159    case CSSPrimitiveValue::CSS_DPCM:
     160    case CSSPrimitiveValue::CSS_DPI:
     161    case CSSPrimitiveValue::CSS_DPPX:
     162    case CSSPrimitiveValue::CSS_EMS:
     163    case CSSPrimitiveValue::CSS_EXS:
     164    case CSSPrimitiveValue::CSS_FONT_FAMILY:
     165    case CSSPrimitiveValue::CSS_FR:
     166    case CSSPrimitiveValue::CSS_GRAD:
     167    case CSSPrimitiveValue::CSS_HZ:
     168    case CSSPrimitiveValue::CSS_IDENT:
     169    case CSSPrimitiveValue::CSS_IN:
     170    case CSSPrimitiveValue::CSS_KHZ:
     171    case CSSPrimitiveValue::CSS_MM:
     172    case CSSPrimitiveValue::CSS_MS:
     173    case CSSPrimitiveValue::CSS_NUMBER:
     174    case CSSPrimitiveValue::CSS_PAIR:
     175    case CSSPrimitiveValue::CSS_PARSER_INTEGER:
     176    case CSSPrimitiveValue::CSS_PARSER_OPERATOR:
     177    case CSSPrimitiveValue::CSS_PC:
     178    case CSSPrimitiveValue::CSS_PERCENTAGE:
     179    case CSSPrimitiveValue::CSS_PROPERTY_ID:
     180    case CSSPrimitiveValue::CSS_PT:
     181    case CSSPrimitiveValue::CSS_PX:
     182    case CSSPrimitiveValue::CSS_QUAD:
     183    case CSSPrimitiveValue::CSS_RAD:
     184    case CSSPrimitiveValue::CSS_RECT:
     185    case CSSPrimitiveValue::CSS_REMS:
     186    case CSSPrimitiveValue::CSS_RGBCOLOR:
     187    case CSSPrimitiveValue::CSS_S:
     188    case CSSPrimitiveValue::CSS_SHAPE:
     189    case CSSPrimitiveValue::CSS_TURN:
     190    case CSSPrimitiveValue::CSS_UNICODE_RANGE:
     191    case CSSPrimitiveValue::CSS_UNKNOWN:
     192    case CSSPrimitiveValue::CSS_VALUE_ID:
     193    case CSSPrimitiveValue::CSS_VH:
     194    case CSSPrimitiveValue::CSS_VMAX:
     195    case CSSPrimitiveValue::CSS_VMIN:
     196    case CSSPrimitiveValue::CSS_VW:
     197#if ENABLE(DASHBOARD_SUPPORT)
     198    case CSSPrimitiveValue::CSS_DASHBOARD_REGION:
     199#endif
     200#if ENABLE(CSS_SCROLL_SNAP)
     201    case CSSPrimitiveValue::CSS_LENGTH_REPEAT:
     202#endif
     203        return false;
     204    }
     205
     206    ASSERT_NOT_REACHED();
     207    return false;
     208}
     209
     210#endif // !ASSERT_DISABLED
    139211
    140212CSSPrimitiveValue::UnitCategory CSSPrimitiveValue::unitCategory(CSSPrimitiveValue::UnitTypes type)
    … …  
    268340}
    269341
    270 CSSPrimitiveValue::CSSPrimitiveValue(const String& str, UnitTypes type)
     342CSSPrimitiveValue::CSSPrimitiveValue(const String& string, UnitTypes type)
    271343    : CSSValue(PrimitiveClass)
    272344{
     345    ASSERT(isStringType(type));
    273346    m_primitiveUnitType = type;
    274     if ((m_value.string = str.impl()))
     347    if ((m_value.string = string.impl()))
    275348        m_value.string->ref();
    276349}
    … …  
    448521void CSSPrimitiveValue::cleanup()
    449522{
    450     switch (static_cast<UnitTypes>(m_primitiveUnitType)) {
     523    auto type = static_cast<UnitTypes>(m_primitiveUnitType);
     524    switch (type) {
    451525    case CSS_STRING:
    452526    case CSS_URI:
    453527    case CSS_ATTR:
    454528    case CSS_COUNTER_NAME:
     529    case CSS_DIMENSION:
    455530    case CSS_PARSER_HEXCOLOR:
     531    case CSS_PARSER_IDENTIFIER:
    456532    case CSS_PARSER_WHITESPACE:
     533        ASSERT(isStringType(type));
    457534        if (m_value.string)
    458535            m_value.string->deref();
    … …  
    527604    case CSS_IDENT:
    528605    case CSS_RGBCOLOR:
    529     case CSS_DIMENSION:
    530606    case CSS_UNKNOWN:
    531607    case CSS_UNICODE_RANGE:
    532608    case CSS_PARSER_OPERATOR:
    533     case CSS_PARSER_IDENTIFIER:
    534609    case CSS_PROPERTY_ID:
    535610    case CSS_VALUE_ID:
     611        ASSERT(!isStringType(type));
    536612        break;
    537613    }

  • trunk/Source/WebCore/css/CSSValueList.cpp

    r201333 r201690  
    248248bool CSSValueList::buildParserValueListSubstitutingVariables(CSSParserValueList* parserList, const CustomPropertyValueMap& customProperties) const
    249249{
    250     for (unsigned i = 0; i < m_values.size(); ++i) {
    251         CSSParserValue result;
    252         result.id = CSSValueInvalid;
    253         switch (m_values[i]->classType()) {
    254         case FunctionClass:
    255             if (!downcast<CSSFunctionValue>(*m_values[i].ptr()).buildParserValueSubstitutingVariables(&result, customProperties))
    256                 return false;
     250    for (auto& value : m_values) {
     251        switch (value.get().classType()) {
     252        case FunctionClass: {
     253            CSSParserValue result;
     254            result.id = CSSValueInvalid;
     255            if (!downcast<CSSFunctionValue>(value.get()).buildParserValueSubstitutingVariables(&result, customProperties)) {
     256                WebCore::destroy(result);
     257                return false;
     258            }
    257259            parserList->addValue(result);
    258260            break;
    259         case ValueListClass:
    260             if (!downcast<CSSValueList>(*m_values[i].ptr()).buildParserValueSubstitutingVariables(&result, customProperties))
    261                 return false;
     261        }
     262        case ValueListClass: {
     263            CSSParserValue result;
     264            result.id = CSSValueInvalid;
     265            if (!downcast<CSSValueList>(value.get()).buildParserValueSubstitutingVariables(&result, customProperties)) {
     266                WebCore::destroy(result);
     267                return false;
     268            }
    262269            parserList->addValue(result);
    263270            break;
     271        }
    264272        case VariableClass: {
    265             if (!downcast<CSSVariableValue>(*m_values[i].ptr()).buildParserValueListSubstitutingVariables(parserList, customProperties))
    266                 return false;
    267             break;
    268         }
    269         case PrimitiveClass:
     273            if (!downcast<CSSVariableValue>(value.get()).buildParserValueListSubstitutingVariables(parserList, customProperties))
     274                return false;
     275            break;
     276        }
     277        case PrimitiveClass: {
    270278            // FIXME: Will have to change this if we start preserving invalid tokens.
    271             if (downcast<CSSPrimitiveValue>(*m_values[i].ptr()).buildParserValue(&result))
     279            CSSParserValue result;
     280            result.id = CSSValueInvalid;
     281            if (downcast<CSSPrimitiveValue>(value.get()).buildParserValue(&result))
    272282                parserList->addValue(result);
    273             break;
     283            else
     284                WebCore::destroy(result);
     285            break;
     286        }
    274287        default:
    275288            ASSERT_NOT_REACHED();
    276             break;
    277289            return false;
    278290        }
Note: See TracChangeset for help on using the changeset viewer.