[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





--- Comment #29 from Hayato Ito <hayato at chromium.org>  2010-07-19 23:13:07 PST ---
Hi Darin,
Thank you for the review.

(In reply to comment #26)
> (From update of attachment 61781 [details])
> 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.

Done. Thank you.
I'd like to use anonymous namespace to hide this class completely in linking, but it doesn't seem WebKit's convention requires such a hiding as far as I can see.
So I've just moved CSSSelectorBag entirely to .cpp. If there is a problem, let me know it.


> 
> > +    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.

Though I am not sure that how much this early exit contributes the performance, let me explain the intension of this 'early exit'.
By this early exit, we can avoid creating CSSSelectorBag in most cases.
Suppose we have the chain of CSSSelectors, which has length of 10. If we exit early, we create CSSSelectorBag at once only when deleting the root of chain.
If we don't exit early, we have to create CSSSelectorBag 10 times. 9 out of 10 are unnecessary ones.


> 
> > +        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.

Done. Thank you.

> 
> > +            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.

We need to clear out the pointer. Clearing it out acts as a 'flag' which means we already put the children of that selector in the 'queue', which is required to avoid recursion. If we don't clear it out, we have some problems. Let me explain. Suppose that we omit clearing m_data.m_rareData out here, 

           delete selector->m_data.m_rareData;
           // Comment out... selector->m_data.m_rareData = 0;

In this case, when a destructor will be called for that selector, the selector's m_data.m_rareData was already deleted, but m_data.m_radeData has a non-null value. As a result, we'll refer the selector's m_data.m_radeData wrongly in deleting that selector.

           // We refer the selector's m_data.m_rareData, which is already deleted.
           selectorsToBeDeleted.append(m_data.m_rareData->m_tagHistory.release()); 


> 
> > +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.

Done. Thank you.

> 
> > +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.

Done. That is just what I was looking for!

> 
> > +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.

Done.
In fact, I've tried to remove m_oneSelector before sending a patch, but I wonder this optimization might still work. So I left it as is.
Now, I totally agree the we don't need it anymore. Thank you!

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