Bug 108394 - Begin to make XSSAuditor thread aware
Summary: Begin to make XSSAuditor thread aware
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-01-30 15:53 PST by Tony Gentilcore
Modified: 2013-01-31 15:20 PST (History)
8 users (show)

See Also:


Attachments
Patch (16.64 KB, patch)
2013-01-30 15:55 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (27.91 KB, patch)
2013-01-31 11:43 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (28.21 KB, patch)
2013-01-31 14:12 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-01-30 15:53:54 PST
Begin to make XSSAuditor thread aware
Comment 1 Tony Gentilcore 2013-01-30 15:55:49 PST
Created attachment 185605 [details]
Patch
Comment 2 Tony Gentilcore 2013-01-30 15:58:36 PST
Comment on attachment 185605 [details]
Patch

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

> Source/WebCore/html/parser/XSSAuditor.cpp:297
> +        callOnMainThread(bind(&XSSAuditorDelegate::didBlockScript, m_delegate, request.release()));

This patch works if I change this to a straight call instead of a callOnMainThread. But with callOnMainThread, didBlockScript is never reached. Any theories?
Comment 3 Adam Barth 2013-01-30 16:29:11 PST
Comment on attachment 185605 [details]
Patch

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

>> Source/WebCore/html/parser/XSSAuditor.cpp:297
>> +        callOnMainThread(bind(&XSSAuditorDelegate::didBlockScript, m_delegate, request.release()));
> 
> This patch works if I change this to a straight call instead of a callOnMainThread. But with callOnMainThread, didBlockScript is never reached. Any theories?

Rather than using callOnMainThread, we'll need to put this into the token stream so that it works correctly with speculative parsing.  Specifically, if we're speculation ahead, we only want the side effects of blocking an XSS to occur if and when we actually process that speculative token.
Comment 4 Adam Barth 2013-01-30 18:01:16 PST
> This patch works if I change this to a straight call instead of a callOnMainThread. But with callOnMainThread, didBlockScript is never reached. Any theories?

To answer your question specifically, my theory is that parsing is over by the time this callback gets scheduled on the main thread.  That means the m_delegate weak pointer gets invalidated and the callback gets cancelled.
Comment 5 Tony Gentilcore 2013-01-31 11:43:01 PST
Created attachment 185830 [details]
Patch
Comment 6 Eric Seidel (no email) 2013-01-31 11:46:43 PST
Comment on attachment 185830 [details]
Patch

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

Looks like a useful first step.  I think Adam should give the official r+ as he wrote much of this code to begin with.

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:1
> -<?xml version="1.0" encoding="utf-8"?>
> +<?xml version="1.0" encoding="utf-8"?>

I don't think you mean to remove the BOM?
Comment 7 Adam Barth 2013-01-31 12:27:26 PST
Comment on attachment 185830 [details]
Patch

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

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:371
> +                m_xssAuditorDelegate.didBlockScript(*request);            

Maybe request.release() ?  I can imagine the delegate wanting to take ownership of the request.

> Source/WebCore/html/parser/XSSAuditor.cpp:275
> +    OwnPtr<DidBlockScriptRequest> request;

Another minor style comment: you can probably skip this local variable entirely.

> Source/WebCore/html/parser/XSSAuditor.cpp:282
> -        return;
> +        return request.release();

Here you can "return nullptr;"

> Source/WebCore/html/parser/XSSAuditor.cpp:-319
> -

Here you can return request.release();

> Source/WebCore/html/parser/XSSAuditor.cpp:303
> +    return request.release();

Here again you can return nullptr;

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:43
> +    , m_notifyClient(true)

nit: I probably would have flipped this around as m_didNotifyClient, but that's a super minor issue.
Comment 8 Tony Gentilcore 2013-01-31 14:12:23 PST
Created attachment 185860 [details]
Patch
Comment 9 Tony Gentilcore 2013-01-31 14:17:46 PST
Comment on attachment 185830 [details]
Patch

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

>> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:1
>> +<?xml version="1.0" encoding="utf-8"?>
> 
> I don't think you mean to remove the BOM?

Good catch. Fixed.

>> Source/WebCore/html/parser/HTMLDocumentParser.cpp:371
>> +                m_xssAuditorDelegate.didBlockScript(*request);            
> 
> Maybe request.release() ?  I can imagine the delegate wanting to take ownership of the request.

Done

>> Source/WebCore/html/parser/XSSAuditor.cpp:275
>> +    OwnPtr<DidBlockScriptRequest> request;
> 
> Another minor style comment: you can probably skip this local variable entirely.

Done

>> Source/WebCore/html/parser/XSSAuditor.cpp:282
>> +        return request.release();
> 
> Here you can "return nullptr;"

Done

>> Source/WebCore/html/parser/XSSAuditor.cpp:-319
>> -
> 
> Here you can return request.release();

Done

>> Source/WebCore/html/parser/XSSAuditor.cpp:303
>> +    return request.release();
> 
> Here again you can return nullptr;

Done

>> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:43
>> +    , m_notifyClient(true)
> 
> nit: I probably would have flipped this around as m_didNotifyClient, but that's a super minor issue.

It was just a straight copy from XSSAuditor, but I went ahead and fixed it.
Comment 10 WebKit Review Bot 2013-01-31 15:20:08 PST
Comment on attachment 185860 [details]
Patch

Clearing flags on attachment: 185860

Committed r141494: <http://trac.webkit.org/changeset/141494>
Comment 11 WebKit Review Bot 2013-01-31 15:20:14 PST
All reviewed patches have been landed.  Closing bug.