Summary: | Continue making XSSAuditor thread safe: Remove dependency on the parser's tokenizer | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tony Gentilcore <tonyg> | ||||||||||||
Component: | New Bugs | Assignee: | Tony Gentilcore <tonyg> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, buildbot, dbates, ojan.autocc, rniwa, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 106127, 107603 | ||||||||||||||
Attachments: |
|
Description
Tony Gentilcore
2013-02-01 11:07:11 PST
Created attachment 186082 [details]
Patch
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. 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? Created attachment 186100 [details]
Patch
(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. Comment on attachment 186100 [details] Patch Attachment 186100 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16335011 Created attachment 186112 [details]
Patch
This will need rebasing after bug 108698. Created attachment 186666 [details]
Patch
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 Created attachment 186705 [details]
Patch for landing
(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 Comment on attachment 186705 [details] Patch for landing Clearing flags on attachment: 186705 Committed r141938: <http://trac.webkit.org/changeset/141938> All reviewed patches have been landed. Closing bug. |