[webkit-reviews] review granted: [Bug 66101] Two null crashes in Treebuilder : [Attachment 121005] Patch

Tue Jan 3 16:43:16 PST 2012

Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 66101: Two null crashes in Treebuilder
https://bugs.webkit.org/show_bug.cgi?id=66101

Attachment 121005: Patch
https://bugs.webkit.org/attachment.cgi?id=121005&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121005&action=review

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:136
> +    AttachmentQueue queue;
> +    queue.swap(m_attachmentQueue);

You might note why we do a swap here.  Maybe it's obvious to other readers, but
it seems subtle yet important that you're doing this to allow executeTask to
cause reentry.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:252
> +	   // We need to actually add the Doctype node to the DOM.
> +	   executeQueuedTasks();
>	   m_document->setCompatibilityModeFromDoctype();

You should probably add a FIXME here about ways to improve this.  Could this
cause some of the same rentrancy troubles as before?

> Source/WebCore/html/parser/HTMLConstructionSite.h:148
> +    typedef Vector<HTMLConstructionSiteTask, 5> AttachmentQueue;

You should note here that it's rare to have more than one item in this queue. 
You should also note when things get queued.

As you explained in person, this happens when a single token can cause more
than one DOM node (such as reconstructing active formatting elements, or adding
implicit elements like <head>, or <tbody>, etc.)