From c668a50ebdb9e9ce9067838f79efd28ca1c36402 Mon Sep 17 00:00:00 2001 From: "arv@chromium.org" Date: Wed, 11 Apr 2012 19:21:59 +0000 Subject: [PATCH] StyleElement ownerNode is not cleared correctly https://bugs.webkit.org/show_bug.cgi?id=83696 Reviewed by Antti Koivisto. Source/WebCore: When the css text changes in such a way that we remove the sheet of a style element or a link[rel=stylesheet] element we need to ensure that the ownerNode of the sheet is cleared. If we don't do this and there is a wrapper for the sheet the sheet is kept alive but the ownerNode of the sheet may point to a deleted node. Tests: fast/dom/StyleSheet/detached-sheet-owner-node-link.html fast/dom/StyleSheet/detached-sheet-owner-node.html * dom/StyleElement.cpp: (WebCore::StyleElement::removedFromDocument): (WebCore::StyleElement::clearSheet): (WebCore): (WebCore::StyleElement::createSheet): * dom/StyleElement.h: (StyleElement): * html/HTMLLinkElement.cpp: (WebCore::HTMLLinkElement::process): (WebCore::HTMLLinkElement::clearSheet): (WebCore): * html/HTMLLinkElement.h: (HTMLLinkElement): LayoutTests: * fast/dom/StyleSheet/detached-sheet-owner-node-expected.txt: Added. * fast/dom/StyleSheet/detached-sheet-owner-node-link-expected.txt: Added. * fast/dom/StyleSheet/detached-sheet-owner-node-link.html: Added. * fast/dom/StyleSheet/detached-sheet-owner-node.html: Added. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@113887 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- LayoutTests/ChangeLog | 12 ++++++++++ .../detached-sheet-owner-node-expected.txt | 9 +++++++ .../detached-sheet-owner-node-link-expected.txt | 9 +++++++ .../StyleSheet/detached-sheet-owner-node-link.html | 23 ++++++++++++++++++ .../dom/StyleSheet/detached-sheet-owner-node.html | 23 ++++++++++++++++++ Source/WebCore/ChangeLog | 28 ++++++++++++++++++++++ Source/WebCore/dom/StyleElement.cpp | 16 ++++++++----- Source/WebCore/dom/StyleElement.h | 1 + Source/WebCore/html/HTMLLinkElement.cpp | 17 ++++++++----- Source/WebCore/html/HTMLLinkElement.h | 1 + 10 files changed, 127 insertions(+), 12 deletions(-) create mode 100644 LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-expected.txt create mode 100644 LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-link-expected.txt create mode 100644 LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-link.html create mode 100644 LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node.html diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 6a47a7f..92f7aab 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,3 +1,15 @@ +2012-04-11 Erik Arvidsson + + StyleElement ownerNode is not cleared correctly + https://bugs.webkit.org/show_bug.cgi?id=83696 + + Reviewed by Antti Koivisto. + + * fast/dom/StyleSheet/detached-sheet-owner-node-expected.txt: Added. + * fast/dom/StyleSheet/detached-sheet-owner-node-link-expected.txt: Added. + * fast/dom/StyleSheet/detached-sheet-owner-node-link.html: Added. + * fast/dom/StyleSheet/detached-sheet-owner-node.html: Added. + 2012-04-11 David Hyatt https://bugs.webkit.org/show_bug.cgi?id=83614 diff --git a/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-expected.txt b/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-expected.txt new file mode 100644 index 0000000..2cf0ea7 --- /dev/null +++ b/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-expected.txt @@ -0,0 +1,9 @@ +This tests that accessing ownerNode on a disconnected style sheet does not crash + +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". + + +PASS successfullyParsed is true + +TEST COMPLETE + diff --git a/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-link-expected.txt b/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-link-expected.txt new file mode 100644 index 0000000..2cf0ea7 --- /dev/null +++ b/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-link-expected.txt @@ -0,0 +1,9 @@ +This tests that accessing ownerNode on a disconnected style sheet does not crash + +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". + + +PASS successfullyParsed is true + +TEST COMPLETE + diff --git a/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-link.html b/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-link.html new file mode 100644 index 0000000..9399c98 --- /dev/null +++ b/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node-link.html @@ -0,0 +1,23 @@ + + + + + \ No newline at end of file diff --git a/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node.html b/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node.html new file mode 100644 index 0000000..e77a7cb --- /dev/null +++ b/LayoutTests/fast/dom/StyleSheet/detached-sheet-owner-node.html @@ -0,0 +1,23 @@ + + + + + \ No newline at end of file diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index 6e761d9..da0b083 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,31 @@ +2012-04-11 Erik Arvidsson + + StyleElement ownerNode is not cleared correctly + https://bugs.webkit.org/show_bug.cgi?id=83696 + + Reviewed by Antti Koivisto. + + When the css text changes in such a way that we remove the sheet of a style element or a link[rel=stylesheet] + element we need to ensure that the ownerNode of the sheet is cleared. If we don't do this and there is a + wrapper for the sheet the sheet is kept alive but the ownerNode of the sheet may point to a deleted node. + + Tests: fast/dom/StyleSheet/detached-sheet-owner-node-link.html + fast/dom/StyleSheet/detached-sheet-owner-node.html + + * dom/StyleElement.cpp: + (WebCore::StyleElement::removedFromDocument): + (WebCore::StyleElement::clearSheet): + (WebCore): + (WebCore::StyleElement::createSheet): + * dom/StyleElement.h: + (StyleElement): + * html/HTMLLinkElement.cpp: + (WebCore::HTMLLinkElement::process): + (WebCore::HTMLLinkElement::clearSheet): + (WebCore): + * html/HTMLLinkElement.h: + (HTMLLinkElement): + 2012-04-11 David Hyatt https://bugs.webkit.org/show_bug.cgi?id=83614 diff --git a/Source/WebCore/dom/StyleElement.cpp b/Source/WebCore/dom/StyleElement.cpp index 831f785..11a6236 100644 --- a/Source/WebCore/dom/StyleElement.cpp +++ b/Source/WebCore/dom/StyleElement.cpp @@ -74,11 +74,8 @@ void StyleElement::removedFromDocument(Document* document, Element* element) ASSERT(element); document->removeStyleSheetCandidateNode(element); - if (m_sheet) { - ASSERT(m_sheet->ownerNode() == element); - m_sheet->clearOwnerNode(); - m_sheet = 0; - } + if (m_sheet) + clearSheet(); // If we're in document teardown, then we don't need to do any notification of our sheet's removal. if (document->renderer()) @@ -139,6 +136,13 @@ void StyleElement::process(Element* e) createSheet(e, m_startLineNumber, sheetText.toString()); } +void StyleElement::clearSheet() +{ + ASSERT(m_sheet); + m_sheet->clearOwnerNode(); + m_sheet = 0; +} + void StyleElement::createSheet(Element* e, int startLineNumber, const String& text) { ASSERT(e); @@ -147,7 +151,7 @@ void StyleElement::createSheet(Element* e, int startLineNumber, const String& te if (m_sheet) { if (m_sheet->isLoading()) document->removePendingSheet(); - m_sheet = 0; + clearSheet(); } // If type is empty or CSS, this is a CSS style sheet. diff --git a/Source/WebCore/dom/StyleElement.h b/Source/WebCore/dom/StyleElement.h index 7ca99da..3c31abb 100644 --- a/Source/WebCore/dom/StyleElement.h +++ b/Source/WebCore/dom/StyleElement.h @@ -54,6 +54,7 @@ protected: private: void createSheet(Element*, int startLineNumber, const String& text = String()); void process(Element*); + void clearSheet(); bool m_createdByParser; bool m_loading; diff --git a/Source/WebCore/html/HTMLLinkElement.cpp b/Source/WebCore/html/HTMLLinkElement.cpp index df51cca..0db57b9 100644 --- a/Source/WebCore/html/HTMLLinkElement.cpp +++ b/Source/WebCore/html/HTMLLinkElement.cpp @@ -230,11 +230,19 @@ void HTMLLinkElement::process() } } else if (m_sheet) { // we no longer contain a stylesheet, e.g. perhaps rel or type was changed - m_sheet = 0; + clearSheet(); document()->styleSelectorChanged(DeferRecalcStyle); } } +void HTMLLinkElement::clearSheet() +{ + ASSERT(m_sheet); + ASSERT(m_sheet->ownerNode() == this); + m_sheet->clearOwnerNode(); + m_sheet = 0; +} + void HTMLLinkElement::insertedIntoDocument() { HTMLElement::insertedIntoDocument(); @@ -258,11 +266,8 @@ void HTMLLinkElement::removedFromDocument() } document()->removeStyleSheetCandidateNode(this); - if (m_sheet) { - ASSERT(m_sheet->ownerNode() == this); - m_sheet->clearOwnerNode(); - m_sheet = 0; - } + if (m_sheet) + clearSheet(); if (styleSheetIsLoading()) removePendingSheet(); diff --git a/Source/WebCore/html/HTMLLinkElement.h b/Source/WebCore/html/HTMLLinkElement.h index 55c5777..9411842 100644 --- a/Source/WebCore/html/HTMLLinkElement.h +++ b/Source/WebCore/html/HTMLLinkElement.h @@ -73,6 +73,7 @@ private: virtual bool shouldLoadLink(); void process(); static void processCallback(Node*); + void clearSheet(); virtual void insertedIntoDocument(); virtual void removedFromDocument(); -- 2.7.4