2010-07-20 Hayato Ito <hayato@chromium.org>

Reviewed by Darin Adler. Fixed a crash when deeply nested CSS selector is used. https://bugs.webkit.org/show_bug.cgi?id=41129 This patch deletes CSSSelectors iteratively so that it doesn't cause stack overflow. * fast/css/css-selector-deeply-nested-expected.txt: Added. * fast/css/css-selector-deeply-nested.html: Added. 2010-07-20 Hayato Ito <hayato@chromium.org> Reviewed by Darin Adler. Fixed a crash when deeply nested CSS selector is used. https://bugs.webkit.org/show_bug.cgi?id=41129 This patch deletes CSSSelectors iteratively so that it doesn't cause stack overflow. Test: fast/css/css-selector-deeply-nested.html * css/CSSSelector.cpp: (WebCore::CSSSelectorBag::~CSSSelectorBag): (WebCore::CSSSelectorBag::isEmpty): (WebCore::CSSSelectorBag::append): (WebCore::CSSSelectorBag::takeAny): (WebCore::CSSSelector::~CSSSelector): (WebCore::CSSSelector::specificity): * css/CSSSelector.h: Canonical link: https://commits.webkit.org/54585@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@63747 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jul 20, 2010 · 20b7eec6a844d88f7b82a8f3aed4788b27a17df0 · 20b7eec
1 parent 4969f63
commit 20b7eec6a844d88f7b82a8f3aed4788b27a17df0
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2010-07-20  Hayato Ito  <hayato@chromium.org>
+
+        Reviewed by Darin Adler.
+
+        Fixed a crash when deeply nested CSS selector is used.
+        https://bugs.webkit.org/show_bug.cgi?id=41129
+
+        This patch deletes CSSSelectors iteratively so that it doesn't cause stack overflow.
+
+        * fast/css/css-selector-deeply-nested-expected.txt: Added.
+        * fast/css/css-selector-deeply-nested.html: Added.
+
 2010-07-20  Stephen White  <senorblanco@chromium.org>
 
         Unreviewed; expectations fix.

diff --git a/LayoutTests/fast/css/css-selector-deeply-nested-expected.txt b/LayoutTests/fast/css/css-selector-deeply-nested-expected.txt
@@ -0,0 +1,3 @@
+Test case for http://bugs.webkit.org/show_bug.cgi?id=41129
+
+If browser didn't crash, the test passed.
diff --git a/LayoutTests/fast/css/css-selector-deeply-nested.html b/LayoutTests/fast/css/css-selector-deeply-nested.html
@@ -0,0 +1,25 @@
+<html>
+<head></head>
+<body>
+<script>
+function generate_deeply_nested_selector(nest) {
+    var selector = [];
+    for (var i = 0; i < nest; ++i)
+        selector.push('* > ');
+    // We don't add last '*' to the selector so that this CSS Selector is invalid.
+    selector.push(' {background:blue}');
+    var style = document.createElement('style');
+    style.type = 'text/css';
+    style.innerHTML = selector.join('');
+    document.head.appendChild(style);
+}
+
+generate_deeply_nested_selector(400000);
+
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+</script>
+<p>Test case for <a href="http://bugs.webkit.org/show_bug.cgi?id=41129">http://bugs.webkit.org/show_bug.cgi?id=41129</a></p>
+<p>If browser didn't crash, the test passed.</p>
+</body>
+</html>
diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
@@ -1,3 +1,23 @@
+2010-07-20  Hayato Ito  <hayato@chromium.org>
+
+        Reviewed by Darin Adler.
+
+        Fixed a crash when deeply nested CSS selector is used.
+        https://bugs.webkit.org/show_bug.cgi?id=41129
+
+        This patch deletes CSSSelectors iteratively so that it doesn't cause stack overflow.
+
+        Test: fast/css/css-selector-deeply-nested.html
+
+        * css/CSSSelector.cpp:
+        (WebCore::CSSSelectorBag::~CSSSelectorBag):
+        (WebCore::CSSSelectorBag::isEmpty):
+        (WebCore::CSSSelectorBag::append):
+        (WebCore::CSSSelectorBag::takeAny):
+        (WebCore::CSSSelector::~CSSSelector):
+        (WebCore::CSSSelector::specificity):
+        * css/CSSSelector.h:
+
 2010-07-20  Yury Semikhatsky  <yurys@chromium.org>
 
         Reviewed by Pavel Feldman.

diff --git a/WebCore/css/CSSSelector.cpp b/WebCore/css/CSSSelector.cpp
@@ -31,11 +31,83 @@
 #include <wtf/Assertions.h>
 #include <wtf/HashMap.h>
 #include <wtf/StdLibExtras.h>
+#include <wtf/Vector.h>
 
 namespace WebCore {
 
 using namespace HTMLNames;
 
+// A helper class to hold CSSSelectors.
+class CSSSelectorBag : public Noncopyable {
+public:
+    ~CSSSelectorBag()
+    {
+        deleteAllValues(m_stack);
+    }
+
+    bool isEmpty() const
+    {
+        return m_stack.isEmpty();
+    }
+
+    void append(PassOwnPtr<CSSSelector> selector)
+    {
+        if (selector)
+            m_stack.append(selector.leakPtr());
+    }
+
+    PassOwnPtr<CSSSelector> takeAny()
+    {
+        ASSERT(!isEmpty());
+        OwnPtr<CSSSelector> selector = adoptPtr(m_stack.last());
+        m_stack.removeLast();
+        return selector.release();
+    }
+
+private:
+    Vector<CSSSelector*, 16> m_stack;
+};
+
+CSSSelector::~CSSSelector()
+{
+    // We should avoid a recursive destructor call, which causes stack overflow
+    // if CSS Selectors are deeply nested.
+
+    // 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;
+
+    CSSSelectorBag selectorsToBeDeleted;
+    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));
+
+    // Traverse the tree of CSSSelector and delete each CSSSelector iteratively.
+    while (!selectorsToBeDeleted.isEmpty()) {
+        OwnPtr<CSSSelector> selector(selectorsToBeDeleted.takeAny());
+        ASSERT(selector);
+        if (selector->m_hasRareData) {
+            ASSERT(selector->m_data.m_rareData);
+            selectorsToBeDeleted.append(selector->m_data.m_rareData->m_tagHistory.release());
+            selectorsToBeDeleted.append(selector->m_data.m_rareData->m_simpleSelector.release());
+            delete selector->m_data.m_rareData;
+            // Clear the pointer so that a destructor of the selector, which is
+            // about to be called, can know the children are already processed.
+            selector->m_data.m_rareData = 0;
+        } else {
+            selectorsToBeDeleted.append(adoptPtr(selector->m_data.m_tagHistory));
+            // Clear the pointer for the same reason.
+            selector->m_data.m_tagHistory = 0;
+        }
+    }
+}
+
 unsigned int CSSSelector::specificity()
 {
     if (m_isForPage)
@@ -63,6 +135,7 @@ unsigned int CSSSelector::specificity()
             break;
     }
 
+    // FIXME: Avoid recursive calls to prevent possible stack overflow.
     if (CSSSelector* tagHistory = this->tagHistory())
         s += tagHistory->specificity();
 
@@ -906,5 +979,5 @@ bool CSSSelector::RareData::matchNth(int count)
         return (m_b - count) % (-m_a) == 0;
     }
 }
-    
+
 } // namespace WebCore
diff --git a/WebCore/css/CSSSelector.h b/WebCore/css/CSSSelector.h
@@ -22,8 +22,8 @@
 #ifndef CSSSelector_h
 #define CSSSelector_h
 
-#include "RenderStyleConstants.h"
 #include "QualifiedName.h"
+#include "RenderStyleConstants.h"
 #include <wtf/Noncopyable.h>
 #include <wtf/OwnPtr.h>
 #include <wtf/PassOwnPtr.h>
@@ -57,13 +57,7 @@ namespace WebCore {
         {
         }
 
-        ~CSSSelector()
-        {
-            if (m_hasRareData)
-                delete m_data.m_rareData;
-            else
-                delete m_data.m_tagHistory;
-        }
+        ~CSSSelector();
 
         /**
          * Re-create selector text from selector's data