Bug 108666 – Continue making XSSAuditor thread safe: Remove dependency on the parser's tokenizer

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Gentilcore]

Continue making XSSAuditor thread safe: Remove dependency on the parser's tokenizer

[Tony Gentilcore]

Created attachment 186082 [details]
Patch

[Tony Gentilcore]

I'm not thrilled with the addition of a new method, but the only alternative I could think of is to pass the tokenizer or shouldAllowCDATA to the filterToken() method which seemed even worse. Let me know if you have any preferences or better ideas here.

[Adam Barth]

Comment on attachment 186082 [details]
Patch

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

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:95
> +    m_xssAuditor.setTokenizer(m_tokenizer.get());

Hum...  This approach look dangerous because m_tokenizer now has a shorter lifetime than HTMLDocumentParser and therefore m_xssAuditor.  We do that because we now have the ability to move the tokenizer back and forth between the main and background thread.

Can we pass the tokenizer each time we call filterToken?

[Tony Gentilcore]

Created attachment 186100 [details]
Patch

[Tony Gentilcore]

(In reply to comment #3)
> (From update of attachment 186082 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186082&action=review
> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:95
> > +    m_xssAuditor.setTokenizer(m_tokenizer.get());
> 
> Hum...  This approach look dangerous because m_tokenizer now has a shorter lifetime than HTMLDocumentParser and therefore m_xssAuditor.  We do that because we now have the ability to move the tokenizer back and forth between the main and background thread.
> 
> Can we pass the tokenizer each time we call filterToken?

I went with just passing the shouldAllowCDATA bool. LMK what you think.

[Build Bot]

Comment on attachment 186100 [details]
Patch

Attachment 186100 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16335011

[Tony Gentilcore]

Created attachment 186112 [details]
Patch

[Tony Gentilcore]

This will need rebasing after bug 108698.

[Tony Gentilcore]

Created attachment 186666 [details]
Patch

[Adam Barth]

Comment on attachment 186666 [details]
Patch

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

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:85
> +    , m_xssAuditor()

This line probably isn't need.  The compiler will call the default constructor for us.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:105
> +    , m_xssAuditor()

ditto

[Tony Gentilcore]

Created attachment 186705 [details]
Patch for landing

[Tony Gentilcore]

(In reply to comment #10)
> (From update of attachment 186666 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186666&action=review
> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:85
> > +    , m_xssAuditor()
> 
> This line probably isn't need.  The compiler will call the default constructor for us.

Fixed

> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:105
> > +    , m_xssAuditor()
> 
> ditto

Fixed

[WebKit Review Bot]

Comment on attachment 186705 [details]
Patch for landing

Clearing flags on attachment: 186705

Committed r141938: <http://trac.webkit.org/changeset/141938>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.