Continue making XSSAuditor thread safe: Remove dependency on parser's sourceForToken and TextResourceDecoder

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Gentilcore]

Continue making XSSAuditor thread safe: Remove dependency on parser's sourceForToken and TextResourceDecoder

[Tony Gentilcore]

Created attachment 186143 [details]
Patch

[Tony Gentilcore]

Created attachment 186151 [details]
Patch

[Adam Barth]

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?

[Tony Gentilcore]

Created attachment 186240 [details]
Patch

[Tony Gentilcore]

> > 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.

[Sam Weinig]

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.

[Build Bot]

Comment on attachment 186240 [details]
Patch

Attachment 186240 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16356126

[Build Bot]

Comment on attachment 186240 [details]
Patch

Attachment 186240 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16353165

[Tony Gentilcore]

Created attachment 186266 [details]
Patch

[Tony Gentilcore]

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.

[Build Bot]

Comment on attachment 186266 [details]
Patch

Attachment 186266 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16358673

[Build Bot]

Comment on attachment 186266 [details]
Patch

Attachment 186266 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16352772

[Tony Gentilcore]

Comment on attachment 186266 [details]
Patch

The win-ews failure looks unrelated to this patch.

[Adam Barth]

Comment on attachment 186266 [details]
Patch

Great!  Thanks.

[WebKit Review Bot]

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

[Tony Gentilcore]

Created attachment 186647 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 186647 [details]
Patch for landing

Clearing flags on attachment: 186647

Committed r141897: <http://trac.webkit.org/changeset/141897>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.