Created attachment 153772 [details] Patch

Comment on attachment 153772 [details] Patch 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?

(In reply to comment #2) > > Source/WebCore/html/parser/HTMLToken.h:105 > > + AtomicHTMLToken(HTMLToken& token) : AtomicMarkupTokenBase<HTMLToken>(&token) { } > > This should be split onto four lines like the other constructor. Okay. I will change it when I land the patch. > > 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? Sure, I will file a bug. We keep only the start tag tokens in the element stack and the list of active formatting elements , so there is no dangling pointer in the current code paths. But I agree that we need to make sure this, so we won't accidentally have dangling pointers later.

Committed r123389: <http://trac.webkit.org/changeset/123389>