[Webkit-unassigned] [Bug 41129] CSSSelector: Avoid chaining tagHistory of CSSSelector, which causes stack overflow.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=41129


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #62035|review?, commit-queue?      |review+, commit-queue+
               Flag|                            |




--- Comment #30 from Darin Adler <darin at apple.com>  2010-07-20 08:04:23 PST ---
(From update of attachment 62035)
I don't think CSSSelectorBag needs a comment. The fact that it's a helper class and the fact that it holds CSS selectors are both things that are clear from the context without a comment.

> +    void append(PassOwnPtr<CSSSelector> selector)

Should probably be named add instead of append.

> +    // We should avoid a recursive destructor call, which causes stack overflow
> +    // if CSS Selectors are deeply nested.

I would just say "selectors" here in this comment instead of "CSS Selectors".

> +    // Early exit if we have already processed the children of this selector.
> +    if (m_hasRareData) {
> +        if (!m_data.m_rareData)
> +            return;
> +    } else if (!m_data.m_tagHistory)
> +        return;

I think it would be slightly better to inline this part of the destructor. Then we could have a separate function named deleteChildren for the rest of the destruction process. That would be more like the old code, in fact, which did have the destructor compiled inline.

> +    if (m_hasRareData) {
> +        selectorsToBeDeleted.append(m_data.m_rareData->m_tagHistory.release());
> +        selectorsToBeDeleted.append(m_data.m_rareData->m_simpleSelector.release());
> +        delete m_data.m_rareData;
> +    } else
> +        selectorsToBeDeleted.append(adoptPtr(m_data.m_tagHistory));

The only difference between this and the code in the body of the loop is that in the loop we need to zero out the fields. It might be better to have a helper function that takes a bag and a selector to do this work, so we don't have to repeat this logic once outside the loop and once inside. We can inline the function if we like. The only cost to using such a function is that the zeroing would either have to be repeated, or be done separately outside the function.

> +    // FIXME: Avoid recursive calls to prevent possible stack overflow.
>      if (CSSSelector* tagHistory = this->tagHistory())
>          s += tagHistory->specificity();

Are you going to tackle this next? Is there a way to construct a regression test that shows the problem?

r=me as is -- The comments above give some possible future improvements.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.