Bug 108394

Summary: Begin to make XSSAuditor thread aware
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: New BugsAssignee: Tony Gentilcore <tonyg>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dbates, eric, gyuyoung.kim, mkwst+watchlist, ojan.autocc, rakuco, 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

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.