[webkit-reviews] review granted: [Bug 91981] Ref-count AtomicHTMLToken : [Attachment 153772] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Kwang Yul Seo
<skyul at company100.net>'s request for review:
Bug 91981: Ref-count AtomicHTMLToken
https://bugs.webkit.org/show_bug.cgi?id=91981

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=153772&action=review


Thanks for splitting this out into a separate patch.  The only concern I have
with this patch is that AtomicHTMLToken has an external character buffer.  If
we retain the atomic token too long, then that will point to garbage memory.

> Source/WebCore/html/parser/HTMLToken.h:105
> +    AtomicHTMLToken(HTMLToken& token) :
AtomicMarkupTokenBase<HTMLToken>(&token) { }

This should be split onto four lines like the other constructor.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:436
> -    AtomicHTMLToken token(rawToken);
> +    RefPtr<AtomicHTMLToken> token = AtomicHTMLToken::create(rawToken);

I wonder if we should clear the external character buffer pointers at the end
of this function...  That way we won't risk having any dangling pointers. 
Would you be willing to post a follow up patch that tries that?