Bug 108666

Summary: Continue making XSSAuditor thread safe: Remove dependency on the parser's tokenizer
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Tony Gentilcore
Reported 2013-02-01 11:07:11 PST
Continue making XSSAuditor thread safe: Remove dependency on the parser's tokenizer
Attachments
Patch (4.42 KB, patch)
2013-02-01 11:07 PST, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Patch (8.97 KB, patch)
2013-02-01 12:11 PST, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Patch (9.14 KB, patch)
2013-02-01 12:50 PST, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Patch (9.98 KB, patch)
2013-02-05 11:27 PST, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Patch for landing (9.93 KB, patch)
2013-02-05 14:21 PST, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Tony Gentilcore
Comment 1 2013-02-01 11:07:37 PST
Created attachment 186082 [details] Patch
Tony Gentilcore
Comment 2 2013-02-01 11:18:14 PST
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 3 2013-02-01 11:44:59 PST
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
Comment 4 2013-02-01 12:11:15 PST
Created attachment 186100 [details] Patch
Tony Gentilcore
Comment 5 2013-02-01 12:12:11 PST
(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 6 2013-02-01 12:38:38 PST
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
Comment 7 2013-02-01 12:50:59 PST
Created attachment 186112 [details] Patch
Tony Gentilcore
Comment 8 2013-02-02 16:34:32 PST
This will need rebasing after bug 108698.
Tony Gentilcore
Comment 9 2013-02-05 11:27:22 PST
Created attachment 186666 [details] Patch
Adam Barth
Comment 10 2013-02-05 13:26:49 PST
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
Comment 11 2013-02-05 14:21:37 PST
Created attachment 186705 [details] Patch for landing
Tony Gentilcore
Comment 12 2013-02-05 14:22:01 PST
(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 13 2013-02-05 14:40:35 PST
Comment on attachment 186705 [details] Patch for landing Clearing flags on attachment: 186705 Committed r141938: <http://trac.webkit.org/changeset/141938>
WebKit Review Bot
Comment 14 2013-02-05 14:40:39 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.