Bug 108726 – Call XSSAuditor's didBlockScript() for the threaded HTML parser

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

Call XSSAuditor's didBlockScript() for the threaded HTML parser

[Tony Gentilcore]

Created attachment 186189 [details]
Patch

[Tony Gentilcore]

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?

[Adam Barth]

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?

[Tony Gentilcore]

Created attachment 186454 [details]
Patch

[Tony Gentilcore]

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

[Adam Barth]

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.

[Tony Gentilcore]

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

[Tony Gentilcore]

Created attachment 186653 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 186653 [details]
Patch for landing

Clearing flags on attachment: 186653

Committed r141905: <http://trac.webkit.org/changeset/141905>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jessie Berlin]

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

[Tony Gentilcore]

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

[WebKit Review Bot]

Re-opened since this is blocked by bug 108963

[Tony Gentilcore]

Created attachment 186738 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186738 [details]
Patch

Clearing flags on attachment: 186738

Committed r142004: <http://trac.webkit.org/changeset/142004>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.