- Revision
- 104000
- Author
- [email protected]
- Date
- 2012-01-03 18:57:43 -0800 (Tue, 03 Jan 2012)
Log Message
Two null crashes in Treebuilder
https://bugs.webkit.org/show_bug.cgi?id=66101
Reviewed by Eric Seidel.
Source/WebCore:
The underly issue with causing the crash is that we're re-entering the
tree builder. We've done a bunch of point fixes around tree builder
re-entrancy, but neither the implementation nor the specification are
really designed to handle re-entrancy.
Firefox avoids this problem by putting the parser on its own thread. I
don't think we're quite ready to do that yet (although we will
eventually, presumably, as computers become ever more parallel). The
approach in this patch is to queue up the DOM mutations and actually
perform them on a shallower stack. That's essentially the approach
we've used for executing <scripts>.
This patch queues up DOM modifications and executes them from a clean
call stack, stopping us from re-entering the tree builder. We might
need to experiment with exactly where to kick off the queue, but this
location seems reasonable and fixes the crash.
Test: fast/parser/re-enter-and-crash.html
* html/parser/HTMLConstructionSite.cpp:
(WebCore::executeTask):
- Add a helper function for actually executing a queue DOM
mutation.
(WebCore::HTMLConstructionSite::attach):
- Instead of actually attaching the element to the DOM, this
function now queues the element for attachment. In a follow-up
patch, I plan to change this function to return void (and I'll
probably rename it to something that makes it clear that it only
queues the attachment).
(WebCore::HTMLConstructionSite::executeQueuedTasks):
(WebCore::HTMLConstructionSite::insertDoctype):
- Setting the compatmode from the Doctype requires the doctype to
actually be in the DOM, so we need to execute our queued tasks
synchronously. We can likely improve this function by passing
the Doctype element explicitly in a follow-up patch.
(WebCore::HTMLConstructionSite::insertSelfClosingHTMLElement):
(WebCore::HTMLConstructionSite::insertTextNode):
(WebCore::HTMLConstructionSite::findFosterSite):
(WebCore::HTMLConstructionSite::fosterParent):
* html/parser/HTMLConstructionSite.h:
(WebCore::HTMLConstructionSiteTask::HTMLConstructionSiteTask):
- Add a task object that holds on to the relevant elements. We
define some vector traits for this object to match the traits on
RefPtr (which make Vector operations faster by explaining that
this type is moveable without having to churn reference counts).
* html/parser/HTMLElementStack.cpp:
(WebCore::HTMLElementStack::pushCommon):
- We delay the "begin parsing" call until we actually attach the
element to the DOM. That splits the responsibility for calling
begin/finished, which is less than ideal, but I didn't see
another solution.
* html/parser/HTMLTreeBuilder.cpp:
(WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken):
- Kick off the queued mutations.
LayoutTests:
Test that we no longer crash on this test case.
* fast/parser/re-enter-and-crash-expected.txt: Added.
* fast/parser/re-enter-and-crash.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (103999 => 104000)
--- trunk/LayoutTests/ChangeLog 2012-01-04 02:41:57 UTC (rev 103999)
+++ trunk/LayoutTests/ChangeLog 2012-01-04 02:57:43 UTC (rev 104000)
@@ -1,5 +1,17 @@
2012-01-03 Adam Barth <[email protected]>
+ Two null crashes in Treebuilder
+ https://bugs.webkit.org/show_bug.cgi?id=66101
+
+ Reviewed by Eric Seidel.
+
+ Test that we no longer crash on this test case.
+
+ * fast/parser/re-enter-and-crash-expected.txt: Added.
+ * fast/parser/re-enter-and-crash.html: Added.
+
+2012-01-03 Adam Barth <[email protected]>
+
view-source doesn't colorize </script> correctly
https://bugs.webkit.org/show_bug.cgi?id=62971
Added: trunk/LayoutTests/fast/parser/re-enter-and-crash-expected.txt (0 => 104000)
--- trunk/LayoutTests/fast/parser/re-enter-and-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/parser/re-enter-and-crash-expected.txt 2012-01-04 02:57:43 UTC (rev 104000)
@@ -0,0 +1,2 @@
+ALERT: This test pases if it doesn't crash.
+
Added: trunk/LayoutTests/fast/parser/re-enter-and-crash.html (0 => 104000)
--- trunk/LayoutTests/fast/parser/re-enter-and-crash.html (rev 0)
+++ trunk/LayoutTests/fast/parser/re-enter-and-crash.html 2012-01-04 02:57:43 UTC (rev 104000)
@@ -0,0 +1,7 @@
+<script>
+if (window.layoutTestController)
+ layoutTestController.dumpAsText();
+
+alert("This test pases if it doesn't crash.");
+</script>
+xxx<iframe _onload_="document.write('<iframe _onload_="document.write(\'<script>\');document.close();"></iframe>');">yyy
Modified: trunk/Source/WebCore/ChangeLog (103999 => 104000)
--- trunk/Source/WebCore/ChangeLog 2012-01-04 02:41:57 UTC (rev 103999)
+++ trunk/Source/WebCore/ChangeLog 2012-01-04 02:57:43 UTC (rev 104000)
@@ -1,5 +1,67 @@
2012-01-03 Adam Barth <[email protected]>
+ Two null crashes in Treebuilder
+ https://bugs.webkit.org/show_bug.cgi?id=66101
+
+ Reviewed by Eric Seidel.
+
+ The underly issue with causing the crash is that we're re-entering the
+ tree builder. We've done a bunch of point fixes around tree builder
+ re-entrancy, but neither the implementation nor the specification are
+ really designed to handle re-entrancy.
+
+ Firefox avoids this problem by putting the parser on its own thread. I
+ don't think we're quite ready to do that yet (although we will
+ eventually, presumably, as computers become ever more parallel). The
+ approach in this patch is to queue up the DOM mutations and actually
+ perform them on a shallower stack. That's essentially the approach
+ we've used for executing <scripts>.
+
+ This patch queues up DOM modifications and executes them from a clean
+ call stack, stopping us from re-entering the tree builder. We might
+ need to experiment with exactly where to kick off the queue, but this
+ location seems reasonable and fixes the crash.
+
+ Test: fast/parser/re-enter-and-crash.html
+
+ * html/parser/HTMLConstructionSite.cpp:
+ (WebCore::executeTask):
+ - Add a helper function for actually executing a queue DOM
+ mutation.
+ (WebCore::HTMLConstructionSite::attach):
+ - Instead of actually attaching the element to the DOM, this
+ function now queues the element for attachment. In a follow-up
+ patch, I plan to change this function to return void (and I'll
+ probably rename it to something that makes it clear that it only
+ queues the attachment).
+ (WebCore::HTMLConstructionSite::executeQueuedTasks):
+ (WebCore::HTMLConstructionSite::insertDoctype):
+ - Setting the compatmode from the Doctype requires the doctype to
+ actually be in the DOM, so we need to execute our queued tasks
+ synchronously. We can likely improve this function by passing
+ the Doctype element explicitly in a follow-up patch.
+ (WebCore::HTMLConstructionSite::insertSelfClosingHTMLElement):
+ (WebCore::HTMLConstructionSite::insertTextNode):
+ (WebCore::HTMLConstructionSite::findFosterSite):
+ (WebCore::HTMLConstructionSite::fosterParent):
+ * html/parser/HTMLConstructionSite.h:
+ (WebCore::HTMLConstructionSiteTask::HTMLConstructionSiteTask):
+ - Add a task object that holds on to the relevant elements. We
+ define some vector traits for this object to match the traits on
+ RefPtr (which make Vector operations faster by explaining that
+ this type is moveable without having to churn reference counts).
+ * html/parser/HTMLElementStack.cpp:
+ (WebCore::HTMLElementStack::pushCommon):
+ - We delay the "begin parsing" call until we actually attach the
+ element to the DOM. That splits the responsibility for calling
+ begin/finished, which is less than ideal, but I didn't see
+ another solution.
+ * html/parser/HTMLTreeBuilder.cpp:
+ (WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken):
+ - Kick off the queued mutations.
+
+2012-01-03 Adam Barth <[email protected]>
+
view-source doesn't colorize </script> correctly
https://bugs.webkit.org/show_bug.cgi?id=62971
Modified: trunk/Source/WebCore/html/parser/HTMLConstructionSite.cpp (103999 => 104000)
--- trunk/Source/WebCore/html/parser/HTMLConstructionSite.cpp 2012-01-04 02:41:57 UTC (rev 103999)
+++ trunk/Source/WebCore/html/parser/HTMLConstructionSite.cpp 2012-01-04 02:57:43 UTC (rev 104000)
@@ -87,53 +87,64 @@
} // namespace
+static inline void executeTask(HTMLConstructionSiteTask& task)
+{
+ if (task.nextChild)
+ task.parent->parserInsertBefore(task.child.get(), task.nextChild.get());
+ else
+ task.parent->parserAddChild(task.child.get());
+
+ // _javascript_ run from beforeload (or DOM Mutation or event handlers)
+ // might have removed the child, in which case we should not attach it.
+
+ if (task.child->parentNode() && task.parent->attached() && !task.child->attached())
+ task.child->attach();
+
+ task.child->beginParsingChildren();
+
+ if (task.selfClosing)
+ task.child->finishParsingChildren();
+}
+
+// FIXME: This function should return void. Callers should keep a pointer to
+// the child if they want one.
template<typename ChildType>
-PassRefPtr<ChildType> HTMLConstructionSite::attach(ContainerNode* rawParent, PassRefPtr<ChildType> prpChild)
+PassRefPtr<ChildType> HTMLConstructionSite::attach(ContainerNode* parent, PassRefPtr<ChildType> prpChild)
{
RefPtr<ChildType> child = prpChild;
- RefPtr<ContainerNode> parent = rawParent;
- // FIXME: It's confusing that HTMLConstructionSite::attach does the magic
- // redirection to the foster parent but HTMLConstructionSite::attachAtSite
- // doesn't. It feels like we're missing a concept somehow.
+ HTMLConstructionSiteTask task;
+ task.parent = parent;
+ task.child = child.get();
+
if (shouldFosterParent()) {
- fosterParent(child.get());
- ASSERT(child->attached() || !child->parentNode() || !child->parentNode()->attached());
+ fosterParent(task.child);
return child.release();
}
// Add as a sibling of the parent if we have reached the maximum depth allowed.
if (m_openElements.stackDepth() > m_maximumDOMTreeDepth)
- parent = parent->parentNode();
+ task.parent = task.parent->parentNode();
- parent->parserAddChild(child);
-
- // An event handler (DOM Mutation, beforeload, et al.) could have removed
- // the child, in which case we shouldn't try attaching it.
- if (!child->parentNode())
- return child.release();
-
- if (parent->attached() && !child->attached())
- child->attach();
+ m_attachmentQueue.append(task);
return child.release();
}
-void HTMLConstructionSite::attachAtSite(const AttachmentSite& site, PassRefPtr<Node> prpChild)
+void HTMLConstructionSite::executeQueuedTasks()
{
- // FIXME: It's unfortunate that we need to hold a reference to child
- // here to call attach(). We should investigate whether we can rely on
- // |site.parent| to hold a ref at this point.
- RefPtr<Node> child = prpChild;
+ const size_t size = m_attachmentQueue.size();
+ if (!size)
+ return;
- if (site.nextChild)
- site.parent->parserInsertBefore(child, site.nextChild);
- else
- site.parent->parserAddChild(child);
+ // Copy the task queue into a local variable in case executeTask
+ // re-enters the parser.
+ AttachmentQueue queue;
+ queue.swap(m_attachmentQueue);
- // _javascript_ run from beforeload (or DOM Mutation or event handlers)
- // might have removed the child, in which case we should not attach it.
- if (child->parentNode() && site.parent->attached() && !child->attached())
- child->attach();
+ for (size_t i = 0; i < size; ++i)
+ executeTask(queue[i]);
+
+ // We might be detached now.
}
HTMLConstructionSite::HTMLConstructionSite(Document* document, unsigned maximumDOMTreeDepth)
@@ -229,7 +240,7 @@
{
ASSERT(token.type() == HTMLTokenTypes::DOCTYPE);
attach(m_attachmentRoot, DocumentType::create(m_document, token.name(), String::adopt(token.publicIdentifier()), String::adopt(token.systemIdentifier())));
-
+
// DOCTYPE nodes are only processed when parsing fragments w/o contextElements, which
// never occurs. However, if we ever chose to support such, this code is subtly wrong,
// because context-less fragments can determine their own quirks mode, and thus change
@@ -238,11 +249,14 @@
ASSERT(!m_isParsingFragment);
if (m_isParsingFragment)
return;
-
+
if (token.forceQuirks())
m_document->setCompatibilityMode(Document::QuirksMode);
- else
+ else {
+ // We need to actually add the Doctype node to the DOM.
+ executeQueuedTasks();
m_document->setCompatibilityModeFromDoctype();
+ }
}
void HTMLConstructionSite::insertComment(AtomicHTMLToken& token)
@@ -306,7 +320,7 @@
// Normally HTMLElementStack is responsible for calling finishParsingChildren,
// but self-closing elements are never in the element stack so the stack
// doesn't get a chance to tell them that we're done parsing their children.
- element->finishParsingChildren();
+ m_attachmentQueue.last().selfClosing = true;
// FIXME: Do we want to acknowledge the token's self-closing flag?
// http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#acknowledge-self-closing-flag
}
@@ -340,11 +354,11 @@
void HTMLConstructionSite::insertTextNode(const String& characters, WhitespaceMode whitespaceMode)
{
- AttachmentSite site;
- site.parent = currentNode();
- site.nextChild = 0;
+ HTMLConstructionSiteTask task;
+ task.parent = currentNode();
+ task.nextChild = 0;
if (shouldFosterParent())
- findFosterSite(site);
+ findFosterSite(task);
// Strings composed entirely of whitespace are likely to be repeated.
// Turn them into AtomicString so we share a single string for each.
@@ -356,7 +370,7 @@
// FIXME: Splitting text nodes into smaller chunks contradicts HTML5 spec, but is currently necessary
// for performance, see <https://bugs.webkit.org/show_bug.cgi?id=55898>.
- Node* previousChild = site.nextChild ? site.nextChild->previousSibling() : site.parent->lastChild();
+ Node* previousChild = task.nextChild ? task.nextChild->previousSibling() : task.parent->lastChild();
if (previousChild && previousChild->isTextNode()) {
// FIXME: We're only supposed to append to this text node if it
// was the last text node inserted by the parser.
@@ -365,16 +379,17 @@
}
while (currentPosition < characters.length()) {
- RefPtr<Text> textNode = Text::createWithLengthLimit(site.parent->document(), shouldUseAtomicString ? AtomicString(characters).string() : characters, currentPosition);
+ RefPtr<Text> textNode = Text::createWithLengthLimit(task.parent->document(), shouldUseAtomicString ? AtomicString(characters).string() : characters, currentPosition);
// If we have a whole string of unbreakable characters the above could lead to an infinite loop. Exceeding the length limit is the lesser evil.
if (!textNode->length()) {
String substring = characters.substring(currentPosition);
- textNode = Text::create(site.parent->document(), shouldUseAtomicString ? AtomicString(substring).string() : substring);
+ textNode = Text::create(task.parent->document(), shouldUseAtomicString ? AtomicString(substring).string() : substring);
}
currentPosition += textNode->length();
ASSERT(currentPosition <= characters.length());
- attachAtSite(site, textNode.release());
+ task.child = textNode.release();
+ executeTask(task);
}
}
@@ -481,23 +496,21 @@
m_openElements.pop();
}
-void HTMLConstructionSite::findFosterSite(AttachmentSite& site)
+void HTMLConstructionSite::findFosterSite(HTMLConstructionSiteTask& task)
{
HTMLElementStack::ElementRecord* lastTableElementRecord = m_openElements.topmost(tableTag.localName());
if (lastTableElementRecord) {
Element* lastTableElement = lastTableElementRecord->element();
if (ContainerNode* parent = lastTableElement->parentNode()) {
- site.parent = parent;
- site.nextChild = lastTableElement;
+ task.parent = parent;
+ task.nextChild = lastTableElement;
return;
}
- site.parent = lastTableElementRecord->next()->element();
- site.nextChild = 0;
+ task.parent = lastTableElementRecord->next()->element();
return;
}
// Fragment case
- site.parent = m_openElements.rootNode(); // DocumentFragment
- site.nextChild = 0;
+ task.parent = m_openElements.rootNode(); // DocumentFragment
}
bool HTMLConstructionSite::shouldFosterParent() const
@@ -507,11 +520,13 @@
&& causesFosterParenting(currentElement()->tagQName());
}
-void HTMLConstructionSite::fosterParent(Node* node)
+void HTMLConstructionSite::fosterParent(PassRefPtr<Node> node)
{
- AttachmentSite site;
- findFosterSite(site);
- attachAtSite(site, node);
+ HTMLConstructionSiteTask task;
+ task.nextChild = 0;
+ findFosterSite(task);
+ task.child = node;
+ executeTask(task);
}
}
Modified: trunk/Source/WebCore/html/parser/HTMLConstructionSite.h (103999 => 104000)
--- trunk/Source/WebCore/html/parser/HTMLConstructionSite.h 2012-01-04 02:41:57 UTC (rev 103999)
+++ trunk/Source/WebCore/html/parser/HTMLConstructionSite.h 2012-01-04 02:57:43 UTC (rev 104000)
@@ -34,9 +34,38 @@
#include <wtf/Noncopyable.h>
#include <wtf/PassRefPtr.h>
#include <wtf/RefPtr.h>
+#include <wtf/Vector.h>
namespace WebCore {
+struct HTMLConstructionSiteTask {
+ HTMLConstructionSiteTask()
+ : selfClosing(false)
+ {
+ }
+
+ void take(HTMLConstructionSiteTask& other)
+ {
+ parent = other.parent.release();
+ nextChild = other.nextChild.release();
+ child = other.child.release();
+ selfClosing = other.selfClosing;
+ }
+
+ RefPtr<ContainerNode> parent;
+ RefPtr<Node> nextChild;
+ RefPtr<Node> child;
+ bool selfClosing;
+};
+
+} // namespace WebCore
+
+namespace WTF {
+template<> struct VectorTraits<WebCore::HTMLConstructionSiteTask> : SimpleClassVectorTraits { };
+} // namespace WTF
+
+namespace WebCore {
+
enum WhitespaceMode {
AllWhitespace,
NotAllWhitespace,
@@ -55,6 +84,7 @@
~HTMLConstructionSite();
void detach();
+ void executeQueuedTasks();
void insertDoctype(AtomicHTMLToken&);
void insertComment(AtomicHTMLToken&);
@@ -78,7 +108,7 @@
PassRefPtr<Element> createHTMLElementFromElementRecord(HTMLElementStack::ElementRecord*);
bool shouldFosterParent() const;
- void fosterParent(Node*);
+ void fosterParent(PassRefPtr<Node>);
bool indexOfFirstUnopenFormattingElement(unsigned& firstUnopenElementIndex) const;
void reconstructTheActiveFormattingElements();
@@ -121,17 +151,15 @@
};
private:
- struct AttachmentSite {
- ContainerNode* parent;
- Node* nextChild;
- };
+ // In the common case, this queue will have only one task because most
+ // tokens produce only one DOM mutation.
+ typedef Vector<HTMLConstructionSiteTask, 1> AttachmentQueue;
template<typename ChildType>
PassRefPtr<ChildType> attach(ContainerNode* parent, PassRefPtr<ChildType> child);
PassRefPtr<Element> attachToCurrent(PassRefPtr<Element>);
- void attachAtSite(const AttachmentSite&, PassRefPtr<Node> child);
- void findFosterSite(AttachmentSite&);
+ void findFosterSite(HTMLConstructionSiteTask&);
PassRefPtr<Element> createHTMLElementFromSavedElement(Element*);
PassRefPtr<Element> createElement(AtomicHTMLToken&, const AtomicString& namespaceURI);
@@ -151,6 +179,8 @@
mutable HTMLElementStack m_openElements;
mutable HTMLFormattingElementList m_activeFormattingElements;
+ AttachmentQueue m_attachmentQueue;
+
FragmentScriptingPermission m_fragmentScriptingPermission;
bool m_isParsingFragment;
@@ -163,6 +193,6 @@
unsigned m_maximumDOMTreeDepth;
};
-}
+} // namespace WebCore
#endif
Modified: trunk/Source/WebCore/html/parser/HTMLElementStack.cpp (103999 => 104000)
--- trunk/Source/WebCore/html/parser/HTMLElementStack.cpp 2012-01-04 02:41:57 UTC (rev 103999)
+++ trunk/Source/WebCore/html/parser/HTMLElementStack.cpp 2012-01-04 02:57:43 UTC (rev 104000)
@@ -585,7 +585,6 @@
m_stackDepth++;
m_top = adoptPtr(new ElementRecord(node, m_top.release()));
- topNode()->beginParsingChildren();
}
void HTMLElementStack::popCommon()
Modified: trunk/Source/WebCore/html/parser/HTMLTreeBuilder.cpp (103999 => 104000)
--- trunk/Source/WebCore/html/parser/HTMLTreeBuilder.cpp 2012-01-04 02:41:57 UTC (rev 103999)
+++ trunk/Source/WebCore/html/parser/HTMLTreeBuilder.cpp 2012-01-04 02:57:43 UTC (rev 104000)
@@ -478,6 +478,9 @@
m_parser->tokenizer()->setForceNullCharacterReplacement(m_insertionMode == TextMode || inForeignContent);
m_parser->tokenizer()->setShouldAllowCDATA(inForeignContent);
+
+ m_tree.executeQueuedTasks();
+ // We might be detached now.
}
void HTMLTreeBuilder::processToken(AtomicHTMLToken& token)
