2011-06-10 Abhishek Arya <inferno@chromium.org>
authorinferno@chromium.org <inferno@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 11 Jun 2011 16:37:59 +0000 (16:37 +0000)
committerinferno@chromium.org <inferno@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 11 Jun 2011 16:37:59 +0000 (16:37 +0000)
        Reviewed by Simon Fraser.

        Null parent element sheet pointers in CSSMutableStyleDeclaration consumers
        when removed from document, set them when reinserted into document.
        https://bugs.webkit.org/show_bug.cgi?id=62230

        When a HTMLBodyElement, StyledElement are removed from document,
        we didn't clear out the parent pointers from their link, style declarations.
        These parent pointers pointed to the document's element sheet which will
        get removed when document is getting destroyed. It does make sense to
        clear out parent pointers when we are getting removed from document and
        readd them when we get inserted again.

        Tests: fast/dom/body-link-decl-parent-crash.html
               fast/dom/styled-inline-style-decl-parent-crash.html

        * dom/StyledElement.cpp:
        (WebCore::StyledElement::insertedIntoDocument):
        (WebCore::StyledElement::removedFromDocument):
        * dom/StyledElement.h:
        * html/HTMLBodyElement.cpp:
        (WebCore::HTMLBodyElement::parseMappedAttribute):
        (WebCore::HTMLBodyElement::insertedIntoDocument):
        (WebCore::HTMLBodyElement::removedFromDocument):
        (WebCore::HTMLBodyElement::didMoveToNewOwnerDocument):
        * html/HTMLBodyElement.h:
2011-06-10  Abhishek Arya  <inferno@chromium.org>

        Reviewed by Simon Fraser.

        Tests that accessing the parent element sheet of an inline style, link
        declaration of styled, body elements which are removed from document,
        does not result in crash.
        https://bugs.webkit.org/show_bug.cgi?id=62230

        * fast/dom/body-link-decl-parent-crash-expected.txt: Added.
        * fast/dom/body-link-decl-parent-crash.html: Added.
        * fast/dom/styled-inline-style-decl-parent-crash-expected.txt: Added.
        * fast/dom/styled-inline-style-decl-parent-crash.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@88601 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/dom/body-link-decl-parent-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/body-link-decl-parent-crash.html [new file with mode: 0644]
LayoutTests/fast/dom/styled-inline-style-decl-parent-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/styled-inline-style-decl-parent-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/StyledElement.cpp
Source/WebCore/dom/StyledElement.h
Source/WebCore/html/HTMLBodyElement.cpp
Source/WebCore/html/HTMLBodyElement.h

index b8fb307..dfbc218 100644 (file)
@@ -1,3 +1,17 @@
+2011-06-10  Abhishek Arya  <inferno@chromium.org>
+
+        Reviewed by Simon Fraser.
+
+        Tests that accessing the parent element sheet of an inline style, link
+        declaration of styled, body elements which are removed from document,
+        does not result in crash.
+        https://bugs.webkit.org/show_bug.cgi?id=62230
+
+        * fast/dom/body-link-decl-parent-crash-expected.txt: Added.
+        * fast/dom/body-link-decl-parent-crash.html: Added.
+        * fast/dom/styled-inline-style-decl-parent-crash-expected.txt: Added.
+        * fast/dom/styled-inline-style-decl-parent-crash.html: Added.
+
 2011-06-10  Ryosuke Niwa  <rniwa@webkit.org>
 
         Skip inspector/profiler/cpu-profiler-profiling-without-inspector.html and
diff --git a/LayoutTests/fast/dom/body-link-decl-parent-crash-expected.txt b/LayoutTests/fast/dom/body-link-decl-parent-crash-expected.txt
new file mode 100644 (file)
index 0000000..48bdc14
--- /dev/null
@@ -0,0 +1,5 @@
+Test passes if it does not crash.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/body-link-decl-parent-crash.html b/LayoutTests/fast/dom/body-link-decl-parent-crash.html
new file mode 100644 (file)
index 0000000..891afbf
--- /dev/null
@@ -0,0 +1,27 @@
+<!DOCTYPE html>\r
+<html>\r
+<head>\r
+<script src="../js/resources/js-test-pre.js"></script>\r
+</head>\r
+<body>\r
+Test passes if it does not crash.\r
+<div id="console"></div>\r
+<script>\r
+if (window.layoutTestController)\r
+    layoutTestController.dumpAsText();\r
+\r
+iframe1 = document.createElement('iframe');\r
+document.body.appendChild(iframe1);\r
+document1 = iframe1.contentDocument.implementation.createHTMLDocument("document");\r
+var body1 = document1.body;\r
+document1.alinkColor = "blue";\r
+document1.body = document1.createElement('body');\r
+delete document1;\r
+gc();\r
+body1.vLink = 1;\r
+\r
+var successfullyParsed = true;\r
+</script>\r
+<script src="../js/resources/js-test-post.js"></script>\r
+</body>\r
+</html>\r
diff --git a/LayoutTests/fast/dom/styled-inline-style-decl-parent-crash-expected.txt b/LayoutTests/fast/dom/styled-inline-style-decl-parent-crash-expected.txt
new file mode 100644 (file)
index 0000000..48bdc14
--- /dev/null
@@ -0,0 +1,5 @@
+Test passes if it does not crash.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/styled-inline-style-decl-parent-crash.html b/LayoutTests/fast/dom/styled-inline-style-decl-parent-crash.html
new file mode 100644 (file)
index 0000000..b3d91b5
--- /dev/null
@@ -0,0 +1,28 @@
+<!DOCTYPE html>\r
+<html>\r
+<head>\r
+<script src="../js/resources/js-test-pre.js"></script>\r
+</head>\r
+<body>\r
+Test passes if it does not crash.\r
+<div id="console"></div>\r
+<script>\r
+if (window.layoutTestController)\r
+    layoutTestController.dumpAsText();\r
+\r
+iframe1 = document.createElement('iframe');\r
+document.body.appendChild(iframe1);\r
+document1 = iframe1.contentDocument.implementation.createHTMLDocument("document");\r
+var div1 = document1.createElement('div');\r
+document1.body.appendChild(div1);\r
+div1.style.color = "blue";\r
+document1.body.removeChild(div1);\r
+delete document1;\r
+gc();\r
+div1.style.color = "red";\r
+\r
+var successfullyParsed = true;\r
+</script>\r
+<script src="../js/resources/js-test-post.js"></script>\r
+</body>\r
+</html>\r
index 854edca..6bad7c7 100644 (file)
@@ -1,3 +1,32 @@
+2011-06-10  Abhishek Arya  <inferno@chromium.org>
+
+        Reviewed by Simon Fraser.
+
+        Null parent element sheet pointers in CSSMutableStyleDeclaration consumers
+        when removed from document, set them when reinserted into document.
+        https://bugs.webkit.org/show_bug.cgi?id=62230
+
+        When a HTMLBodyElement, StyledElement are removed from document,
+        we didn't clear out the parent pointers from their link, style declarations.
+        These parent pointers pointed to the document's element sheet which will
+        get removed when document is getting destroyed. It does make sense to
+        clear out parent pointers when we are getting removed from document and
+        readd them when we get inserted again.
+
+        Tests: fast/dom/body-link-decl-parent-crash.html
+               fast/dom/styled-inline-style-decl-parent-crash.html
+
+        * dom/StyledElement.cpp:
+        (WebCore::StyledElement::insertedIntoDocument):
+        (WebCore::StyledElement::removedFromDocument):
+        * dom/StyledElement.h:
+        * html/HTMLBodyElement.cpp:
+        (WebCore::HTMLBodyElement::parseMappedAttribute):
+        (WebCore::HTMLBodyElement::insertedIntoDocument):
+        (WebCore::HTMLBodyElement::removedFromDocument):
+        (WebCore::HTMLBodyElement::didMoveToNewOwnerDocument):
+        * html/HTMLBodyElement.h:
+
 2011-06-10  Adam Barth  <abarth@webkit.org>
 
         Remove bogus ASSERTs.  These ASSERTs used to be correct before I
index b45ff9f..bd9543d 100644 (file)
@@ -439,6 +439,21 @@ void StyledElement::addSubresourceAttributeURLs(ListHashSet<KURL>& urls) const
         style->addSubresourceStyleURLs(urls);
 }
 
+void StyledElement::insertedIntoDocument()
+{
+    Element::insertedIntoDocument();
+
+    if (m_inlineStyleDecl)
+        m_inlineStyleDecl->setParent(document()->elementSheet());
+}
+
+void StyledElement::removedFromDocument()
+{
+    if (m_inlineStyleDecl)
+        m_inlineStyleDecl->setParent(0);
+
+    Element::removedFromDocument();
+}
 
 void StyledElement::didMoveToNewOwnerDocument()
 {
index 32fc4c2..5bf3ed1 100644 (file)
@@ -84,6 +84,8 @@ protected:
     // svgAttributeChanged (called when element.className.baseValue is set)
     void classAttributeChanged(const AtomicString& newClassString);
     
+    virtual void insertedIntoDocument();
+    virtual void removedFromDocument();
     virtual void didMoveToNewOwnerDocument();
 
 private:
index a19dc3e..64a6bf9 100644 (file)
@@ -116,6 +116,13 @@ void HTMLBodyElement::parseMappedAttribute(Attribute* attr)
     } else if (attr->name() == vlinkAttr ||
                attr->name() == alinkAttr ||
                attr->name() == linkAttr) {
+        // This tells us that we are removed from document. If our document is later destroyed
+        // (not deleted since we hold a guardRef), our stylesheet list will be null causing a crash
+        // later in document()->styleSelector(). So, we bail out early because we shouldn't be
+        // modifying anything in that document. See webkit bug 62230.
+        if (m_linkDecl && !m_linkDecl->parent())
+            return;
+
         if (attr->isNull()) {
             if (attr->name() == linkAttr)
                 document()->resetLinkColor();
@@ -202,6 +209,25 @@ void HTMLBodyElement::insertedIntoDocument()
 
     if (document() && document()->page())
         document()->page()->updateViewportArguments();
+
+    if (m_linkDecl)
+        m_linkDecl->setParent(document()->elementSheet());
+}
+
+void HTMLBodyElement::removedFromDocument()
+{
+    if (m_linkDecl)
+        m_linkDecl->setParent(0);
+    
+    HTMLElement::removedFromDocument();
+}
+
+void HTMLBodyElement::didMoveToNewOwnerDocument()
+{
+    if (m_linkDecl)
+        m_linkDecl->setParent(document()->elementSheet());
+    
+    HTMLElement::didMoveToNewOwnerDocument();
 }
 
 bool HTMLBodyElement::isURLAttribute(Attribute *attr) const
@@ -345,16 +371,4 @@ void HTMLBodyElement::addSubresourceAttributeURLs(ListHashSet<KURL>& urls) const
     addSubresourceURL(urls, document()->completeURL(getAttribute(backgroundAttr)));
 }
 
-void HTMLBodyElement::didMoveToNewOwnerDocument()
-{
-    // When moving body elements between documents, we should have to reset the parent sheet for any
-    // link style declarations.  If we don't we might crash later.
-    // In practice I can't reproduce this theoretical problem.
-    // webarchive/adopt-attribute-styled-body-webarchive.html tries to make sure this crash won't surface.
-    if (m_linkDecl)
-        m_linkDecl->setParent(document()->elementSheet());
-    
-    HTMLElement::didMoveToNewOwnerDocument();
-}
-
 } // namespace WebCore
index cd8a38f..884522c 100644 (file)
@@ -74,6 +74,8 @@ private:
     virtual void parseMappedAttribute(Attribute*);
 
     virtual void insertedIntoDocument();
+    virtual void removedFromDocument();
+    virtual void didMoveToNewOwnerDocument();
 
     void createLinkDecl();
     
@@ -91,8 +93,6 @@ private:
     virtual int scrollWidth() const;
     
     virtual void addSubresourceAttributeURLs(ListHashSet<KURL>&) const;
-    
-    virtual void didMoveToNewOwnerDocument();
 
     RefPtr<CSSMutableStyleDeclaration> m_linkDecl;
 };