Bug 108726

Summary: Call XSSAuditor's didBlockScript() for the threaded HTML parser
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: New BugsAssignee: Tony Gentilcore <tonyg>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, benjamin, dbates, jberlin, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 108963    
Bug Blocks: 106127, 107603    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch none

Description Tony Gentilcore 2013-02-01 17:09:54 PST
Call XSSAuditor's didBlockScript() for the threaded HTML parser
Comment 1 Tony Gentilcore 2013-02-01 17:12:01 PST
Created attachment 186189 [details]
Patch
Comment 2 Tony Gentilcore 2013-02-01 17:14:07 PST
Comment on attachment 186189 [details]
Patch

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

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:44
> +    return isStringSafeToSendToAnotherThread(m_reportURL.string())

I'm skeptical this is going to work but am not sure what to do here. Is it going to be safe to send KURL across threads? How do we ASSERT that?
Comment 3 Adam Barth 2013-02-02 00:15:50 PST
Comment on attachment 186189 [details]
Patch

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

> Source/WebCore/html/parser/CompactHTMLToken.cpp:98
> +    *m_didBlockScriptRequest = *other.didBlockScriptRequest();

This doesn't seem right.  OwnPtr isn't copyable.  We need to implement some sort of fancier VectorTraits to teach Vector how to move the CompactHTMLToken.  There should be examples already in the codebase of how to do this.

> Source/WebCore/html/parser/CompactHTMLToken.h:80
> +    DidBlockScriptRequest* didBlockScriptRequest() const;

The "did" in this function name is very strange.  Can we rename DidBlockScriptRequest to XSSInfo ?

>> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:44
>> +    return isStringSafeToSendToAnotherThread(m_reportURL.string())
> 
> I'm skeptical this is going to work but am not sure what to do here. Is it going to be safe to send KURL across threads? How do we ASSERT that?

Maybe we should add a isSafeToSendToAnotherThread function to KURL?
Comment 4 Tony Gentilcore 2013-02-04 14:00:25 PST
Created attachment 186454 [details]
Patch
Comment 5 Tony Gentilcore 2013-02-04 14:01:05 PST
Comment on attachment 186189 [details]
Patch

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

>> Source/WebCore/html/parser/CompactHTMLToken.cpp:98
>> +    *m_didBlockScriptRequest = *other.didBlockScriptRequest();
> 
> This doesn't seem right.  OwnPtr isn't copyable.  We need to implement some sort of fancier VectorTraits to teach Vector how to move the CompactHTMLToken.  There should be examples already in the codebase of how to do this.

From RenderGeometryMap.h, it looks like SimpleClassVectorTraits does the trick.

>> Source/WebCore/html/parser/CompactHTMLToken.h:80
>> +    DidBlockScriptRequest* didBlockScriptRequest() const;
> 
> The "did" in this function name is very strange.  Can we rename DidBlockScriptRequest to XSSInfo ?

Done

>>> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:44
>>> +    return isStringSafeToSendToAnotherThread(m_reportURL.string())
>> 
>> I'm skeptical this is going to work but am not sure what to do here. Is it going to be safe to send KURL across threads? How do we ASSERT that?
> 
> Maybe we should add a isSafeToSendToAnotherThread function to KURL?

Done
Comment 6 Adam Barth 2013-02-04 21:32:26 PST
Comment on attachment 186454 [details]
Patch

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

> Source/WebCore/html/parser/CompactHTMLToken.h:102
> +// This is required for a struct with OwnPtr. We know CompactHTMLToken is simple enough that
> +// initializing to 0 and moving with memcpy (and then not destructing the original) will work.
> +template<> struct VectorTraits<WebCore::CompactHTMLToken> : SimpleClassVectorTraits { };

If you have this declaration, do you need the CompactHTMLToken copy constructor?  If not, we should remove it.
Comment 7 Tony Gentilcore 2013-02-05 10:21:25 PST
(In reply to comment #6)
> (From update of attachment 186454 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186454&action=review
> 
> > Source/WebCore/html/parser/CompactHTMLToken.h:102
> > +// This is required for a struct with OwnPtr. We know CompactHTMLToken is simple enough that
> > +// initializing to 0 and moving with memcpy (and then not destructing the original) will work.
> > +template<> struct VectorTraits<WebCore::CompactHTMLToken> : SimpleClassVectorTraits { };
> 
> If you have this declaration, do you need the CompactHTMLToken copy constructor?  If not, we should remove it.

Yes. It doesn't compile without the copy ctor. Even with this declaration we still need a way to teach it to copy the structure that the OwnPtr points to, which is where the copy ctor comes in.
Comment 8 Tony Gentilcore 2013-02-05 10:21:45 PST
Created attachment 186653 [details]
Patch for landing
Comment 9 WebKit Review Bot 2013-02-05 11:08:02 PST
Comment on attachment 186653 [details]
Patch for landing

Clearing flags on attachment: 186653

Committed r141905: <http://trac.webkit.org/changeset/141905>
Comment 10 WebKit Review Bot 2013-02-05 11:08:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Jessie Berlin 2013-02-05 11:35:38 PST
(In reply to comment #10)
> All reviewed patches have been landed.  Closing bug.

This is causing build breakage:

http://build.webkit.org/builders/Apple%20Lion%20Debug%20%28Build%29/builds/11613/steps/compile-webkit/logs/stdio

Undefined symbols for architecture x86_64:
  "__ZNK3WTF6String27isSafeToSendToAnotherThreadEv", referenced from:
      __ZNK7WebCore4KURL27isSafeToSendToAnotherThreadEv in KURL.o
      __ZNK7WebCore7XSSInfo27isSafeToSendToAnotherThreadEv in XSSAuditorDelegate.o

Please fix this soon or we will need to roll it out.
Comment 12 Tony Gentilcore 2013-02-05 11:37:43 PST
(In reply to comment #11)
> (In reply to comment #10)
> > All reviewed patches have been landed.  Closing bug.
> 
> This is causing build breakage:
> 
> http://build.webkit.org/builders/Apple%20Lion%20Debug%20%28Build%29/builds/11613/steps/compile-webkit/logs/stdio
> 
> Undefined symbols for architecture x86_64:
>   "__ZNK3WTF6String27isSafeToSendToAnotherThreadEv", referenced from:
>       __ZNK7WebCore4KURL27isSafeToSendToAnotherThreadEv in KURL.o
>       __ZNK7WebCore7XSSInfo27isSafeToSendToAnotherThreadEv in XSSAuditorDelegate.o
> 
> Please fix this soon or we will need to roll it out.

I'm on it.
Comment 13 WebKit Review Bot 2013-02-05 11:41:12 PST
Re-opened since this is blocked by bug 108963
Comment 14 Tony Gentilcore 2013-02-05 18:12:11 PST
Created attachment 186738 [details]
Patch
Comment 15 WebKit Review Bot 2013-02-06 09:51:44 PST
Comment on attachment 186738 [details]
Patch

Clearing flags on attachment: 186738

Committed r142004: <http://trac.webkit.org/changeset/142004>
Comment 16 WebKit Review Bot 2013-02-06 09:51:49 PST
All reviewed patches have been landed.  Closing bug.