Bug 92056 - Clear the external characters pointer of an AtomicHTMLToken before the raw token is cleared.
Summary: Clear the external characters pointer of an AtomicHTMLToken before the raw to...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kwang Yul Seo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-23 19:27 PDT by Kwang Yul Seo
Modified: 2012-07-24 15:35 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.73 KB, patch)
2012-07-23 19:30 PDT, Kwang Yul Seo
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2012-07-23 19:27:34 PDT
AtomicHTMLToken keeps a pointer to the HTMLToken's buffer instead of copying the characters for performance. Clear the external characters pointer before the raw token is cleared to make sure that we won't have a dangling pointer.
Comment 1 Kwang Yul Seo 2012-07-23 19:30:33 PDT
Created attachment 153938 [details]
Patch
Comment 2 Kwang Yul Seo 2012-07-23 19:31:41 PDT
Adam, this is the follow-up patch you requested in Bug 91981.
Comment 3 Adam Barth 2012-07-24 10:07:17 PDT
Comment on attachment 153938 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153938&action=review

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:459
> +    if (token->type() == HTMLTokenTypes::Character)
> +        token->clearExternalCharacters();

I would just do this unconditionally.  There isn't any harm in overwriting m_externalCharacters with 0 for other token types and it saves us this branch.
Comment 4 Adam Barth 2012-07-24 10:10:51 PDT
Thanks for writing this patch.  :)
Comment 5 Kwang Yul Seo 2012-07-24 14:57:01 PDT
(In reply to comment #3)
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:459
> > +    if (token->type() == HTMLTokenTypes::Character)
> > +        token->clearExternalCharacters();
> 
> I would just do this unconditionally.  There isn't any harm in overwriting m_externalCharacters with 0 for other token types and it saves us this branch.

Thanks for the review! I will change it before I land the patch.
Comment 6 Kwang Yul Seo 2012-07-24 15:13:15 PDT
Committed r123536: <http://trac.webkit.org/changeset/123536>
Comment 7 Adam Barth 2012-07-24 15:27:38 PDT
This assert is wrong now that we're calling the function unconditionally.

        ASSERT(m_type == Token::Type::Character);
Comment 8 Kwang Yul Seo 2012-07-24 15:29:00 PDT
(In reply to comment #7)
> This assert is wrong now that we're calling the function unconditionally.
> 
>         ASSERT(m_type == Token::Type::Character);

Oops, sorry. I will fix it.
Comment 9 Kwang Yul Seo 2012-07-24 15:35:37 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > This assert is wrong now that we're calling the function unconditionally.
> > 
> >         ASSERT(m_type == Token::Type::Character);
> 
> Oops, sorry. I will fix it.

Done. http://trac.webkit.org/changeset/123542