[webkit-reviews] review denied: [Bug 41129] CSSSelector: Avoid chaining tagHistory of CSSSelector, which causes stack overflow. : [Attachment 61781] iterative-delete-2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 61781: iterative-delete-2
https://bugs.webkit.org/attachment.cgi?id=61781&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks, this looks like a good approach!

I’d like CSSSelectorBag to be entirely in the .cpp file. There's no need to
have it exposed in the header since it's used only internally.

Typically compilers cope with inline functions best if they come before the use
of those functions. Thus, the CSSSelectorBag implementation should be at the
top of the file, before the CSSSelector destructor.

> +    if (m_hasRareData) {
> +	   if (!m_data.m_rareData)
> +	       return;
> +    } else if (!m_data.m_tagHistory)
> +	   return;

Do we really need to optimize this case? I think the code should be quite fast
without a special case and early exit.

> +	   if (m_data.m_rareData->m_tagHistory)
> +	      
selectorsToBeDeleted.append(m_data.m_rareData->m_tagHistory.release());
> +	   if (m_data.m_rareData->m_simpleSelector)
> +	      
selectorsToBeDeleted.append(m_data.m_rareData->m_simpleSelector.release());

I'd prefer to see the null check inside the append function instead of having a
null check at each call site.

> +	       selector->m_data.m_rareData = 0;

Why clear out this pointer? The selector is about to be deleted anyway.

> +	       selector->m_data.m_tagHistory = 0;

Same comment here.

> +inline CSSSelectorBag::CSSSelectorBag()
> +	   : m_oneSelector()
> +	   , m_overflowSelectors() {}

There is no need to define and explicitly implement this constructor. The
compiler will do this automatically and get it right as long as you don’t
declare it.

> +inline CSSSelectorBag::~CSSSelectorBag()
> +{
> +    size_t size = m_overflowSelectors.size();
> +    for (size_t i = 0; i < size; ++i)
> +	   delete m_overflowSelectors[i];
> +}

You can use the deleteAllValues function instead of writing the loop
explicitly. The only reason I wrote the loop last time was that I was doing the
Deque thing.

> +inline PassOwnPtr<CSSSelector> CSSSelectorBag::takeAny()
> +{
> +    ASSERT(!isEmpty());
> +    if (m_oneSelector)
> +	   return m_oneSelector.release();
> +    OwnPtr<CSSSelector> selector = adoptPtr(m_overflowSelectors.last());
> +    m_overflowSelectors.removeLast();
> +    return selector.release();
> +}

I don't think the m_oneSelector optimization is needed now that you are using a
stack. You can just rip out all the m_oneSelector code and rename
m_overflowSelectors to m_selectors or m_vector or even m_stack.