[webkit-reviews] review granted: [Bug 41129] CSSSelector: Avoid chaining tagHistory of CSSSelector, which causes stack overflow. : [Attachment 62035] iterative-delete-4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Hayato Ito <hayato at chromium.org>'s
request for review:
Bug 41129: CSSSelector: Avoid chaining tagHistory of CSSSelector, which causes
stack overflow.
https://bugs.webkit.org/show_bug.cgi?id=41129

Attachment 62035: iterative-delete-4
https://bugs.webkit.org/attachment.cgi?id=62035&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.