Bug 108698 - Continue making XSSAuditor thread safe: Remove dependency on parser's sourceForToken and TextResourceDecoder
Summary: Continue making XSSAuditor thread safe: Remove dependency on parser's sourceF...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks: 106127 107603
  Show dependency treegraph
 
Reported: 2013-02-01 14:45 PST by Tony Gentilcore
Modified: 2013-02-05 09:57 PST (History)
4 users (show)

See Also:


Attachments
Patch (22.67 KB, patch)
2013-02-01 14:46 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (23.22 KB, patch)
2013-02-01 14:55 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (25.21 KB, patch)
2013-02-02 16:31 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (25.73 KB, patch)
2013-02-03 12:35 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch for landing (26.08 KB, patch)
2013-02-05 09:40 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2013-02-01 14:45:23 PST
Continue making XSSAuditor thread safe: Remove dependency on parser's sourceForToken and TextResourceDecoder
Comment 1 Tony Gentilcore 2013-02-01 14:46:51 PST
Created attachment 186143 [details]
Patch
Comment 2 Tony Gentilcore 2013-02-01 14:55:44 PST
Created attachment 186151 [details]
Patch
Comment 3 Adam Barth 2013-02-02 00:10:13 PST
Comment on attachment 186151 [details]
Patch

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

> Source/WebCore/html/parser/HTMLDocumentParser.h:-78
> -    String sourceForToken(const HTMLToken&);

Nice!

> Source/WebCore/html/parser/XSSAuditor.cpp:275
> -PassOwnPtr<DidBlockScriptRequest> XSSAuditor::filterToken(HTMLToken& token)
> +PassOwnPtr<DidBlockScriptRequest> XSSAuditor::filterToken(HTMLToken& token, HTMLSourceTracker& sourceTracker, const TextResourceDecoder* decoder)

Can we put these things in a struct so that we don't need to pass all 3x the parameters to all these functions?
Comment 4 Tony Gentilcore 2013-02-02 16:31:32 PST
Created attachment 186240 [details]
Patch
Comment 5 Tony Gentilcore 2013-02-02 16:33:37 PST
> > Source/WebCore/html/parser/XSSAuditor.cpp:275
> > -PassOwnPtr<DidBlockScriptRequest> XSSAuditor::filterToken(HTMLToken& token)
> > +PassOwnPtr<DidBlockScriptRequest> XSSAuditor::filterToken(HTMLToken& token, HTMLSourceTracker& sourceTracker, const TextResourceDecoder* decoder)
> 
> Can we put these things in a struct so that we don't need to pass all 3x the parameters to all these functions?

Thank you, that struct is much cleaner! Especially since bug 108666 will add another member.
Comment 6 Sam Weinig 2013-02-02 16:58:34 PST
Comment on attachment 186240 [details]
Patch

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

> Source/WebCore/ChangeLog:35
> +        * html/parser/HTMLDocumentParser.cpp:
> +        (WebCore::HTMLDocumentParser::pumpTokenizer):
> +        * html/parser/HTMLDocumentParser.h:
> +        * html/parser/XSSAuditor.cpp:
> +        (WebCore::XSSAuditor::filterToken):
> +        (WebCore::XSSAuditor::filterStartToken):
> +        (WebCore::XSSAuditor::filterCharacterToken):
> +        (WebCore::XSSAuditor::filterScriptToken):
> +        (WebCore::XSSAuditor::filterObjectToken):
> +        (WebCore::XSSAuditor::filterParamToken):
> +        (WebCore::XSSAuditor::filterEmbedToken):
> +        (WebCore::XSSAuditor::filterAppletToken):
> +        (WebCore::XSSAuditor::filterIframeToken):
> +        (WebCore::XSSAuditor::filterMetaToken):
> +        (WebCore::XSSAuditor::filterBaseToken):
> +        (WebCore::XSSAuditor::filterFormToken):
> +        (WebCore::XSSAuditor::eraseDangerousAttributesIfInjected):
> +        (WebCore::XSSAuditor::eraseAttributeIfInjected):
> +        (WebCore::XSSAuditor::decodedSnippetForName):
> +        (WebCore::XSSAuditor::decodedSnippetForAttribute):
> +        (WebCore::XSSAuditor::decodedSnippetForJavaScript):
> +        * html/parser/XSSAuditor.h:
> +        (WebCore):
> +        (WebCore::FilterTokenRequest::FilterTokenRequest):
> +        (FilterTokenRequest):
> +        (XSSAuditor):

It would be nice to have some more detailed explanation here.
Comment 7 Build Bot 2013-02-02 17:35:40 PST
Comment on attachment 186240 [details]
Patch

Attachment 186240 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16356126
Comment 8 Build Bot 2013-02-02 18:39:56 PST
Comment on attachment 186240 [details]
Patch

Attachment 186240 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16353165
Comment 9 Tony Gentilcore 2013-02-03 12:35:35 PST
Created attachment 186266 [details]
Patch
Comment 10 Tony Gentilcore 2013-02-03 12:35:58 PST
Comment on attachment 186240 [details]
Patch

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

>> Source/WebCore/ChangeLog:35
>> +        (XSSAuditor):
> 
> It would be nice to have some more detailed explanation here.

Good idea. I beefed up the ChangeLog a bit.
Comment 11 Build Bot 2013-02-03 13:37:59 PST
Comment on attachment 186266 [details]
Patch

Attachment 186266 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16358673
Comment 12 Build Bot 2013-02-03 14:39:45 PST
Comment on attachment 186266 [details]
Patch

Attachment 186266 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16352772
Comment 13 Tony Gentilcore 2013-02-04 10:10:48 PST
Comment on attachment 186266 [details]
Patch

The win-ews failure looks unrelated to this patch.
Comment 14 Adam Barth 2013-02-04 21:29:16 PST
Comment on attachment 186266 [details]
Patch

Great!  Thanks.
Comment 15 WebKit Review Bot 2013-02-04 21:33:33 PST
Comment on attachment 186266 [details]
Patch

Rejecting attachment 186266 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 186266, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
e/WebCore/html/parser/XSSAuditor.cpp
Hunk #1 succeeded at 278 (offset 6 lines).
Hunk #2 FAILED at 338.
Hunk #3 succeeded at 573 (offset 9 lines).
Hunk #4 succeeded at 630 (offset 9 lines).
1 out of 4 hunks FAILED -- saving rejects to file Source/WebCore/html/parser/XSSAuditor.cpp.rej
patching file Source/WebCore/html/parser/XSSAuditor.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16375537
Comment 16 Tony Gentilcore 2013-02-05 09:40:13 PST
Created attachment 186647 [details]
Patch for landing
Comment 17 WebKit Review Bot 2013-02-05 09:57:26 PST
Comment on attachment 186647 [details]
Patch for landing

Clearing flags on attachment: 186647

Committed r141897: <http://trac.webkit.org/changeset/141897>
Comment 18 WebKit Review Bot 2013-02-05 09:57:30 PST
All reviewed patches have been landed.  Closing bug.