Bug 91981 – Ref-count AtomicHTMLToken

• Component: DOM
• Status: RESOLVED FIXED

[Kwang Yul Seo]

To avoid copying AtomicHTMLToken in Bug 91703, AtomicHTMLToken needs to be ref-counted.

[Kwang Yul Seo]

Created attachment 153772 [details]
Patch

[Adam Barth]

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?

[Kwang Yul Seo]

(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.

[Kwang Yul Seo]

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