We want to send only one report when blocking script/pages via XSSAuditor. Currently, we do this by zeroing out the report URL after creating an XSSInfo object. I'd like to move this responsibility to the XSSAuditorDelegate instead, and do it explicitly via a "m_numberOfReportsSent" counter or "m_reportSent" boolean. That seems simpler to understand.
Created attachment 192446 [details] Patch
Comment on attachment 192446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192446&action=review > Source/WebCore/html/parser/XSSAuditor.cpp:-335 > - m_reportURL = KURL(); Won't m_reportURL now be referenced on multiple threads at the same time?
(In reply to comment #2) > (From update of attachment 192446 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192446&action=review > > > Source/WebCore/html/parser/XSSAuditor.cpp:-335 > > - m_reportURL = KURL(); > > Won't m_reportURL now be referenced on multiple threads at the same time? Ugh. Yes. r-. I think it's more elegant to allow the delegate to determine what the reporting policy should be. I'm not sure that elegance is worth making multiple isolated copies of the report URL. Perhaps it would be possible to send the report URL to the delegate separately, rather than passing it around on an XSSInfo object. How would you feel about exposing the report URL via a public method on XSSAuditor, and adding it as a property to the XSSAuditorDelegate? We could populate it from HTMLDocumentParser::pumpTokenizer directly after initializing the Auditor. This would allow us to remove the property from XSSInfo objects entirely, which would also allow us to drop the thread-safe checks, as it would consist entirely of booleans (and a TextPosition, which might or might not be safe to pass across threads? (it isn't being checked in the existing code)). WDYT?
Yeah, there don't seem to be much reason to send the report URL to the background thread. We should give it to the delegate on the main thread somehow.
(In reply to comment #4) > Yeah, there don't seem to be much reason to send the report URL to the background thread. We should give it to the delegate on the main thread somehow. Forked this into https://bugs.webkit.org/show_bug.cgi?id=112179 for clarity. Mind taking a look?
Created attachment 192901 [details] Patch
Comment on attachment 192901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192901&action=review > Source/WebCore/html/parser/XSSAuditorDelegate.cpp:111 > + RefPtr<FormData> report = generateViolationReport(); > + PingLoader::sendViolationReport(m_document->frame(), m_reportURL, report); report.release() Actually, I would just combine these lines.
Created attachment 193098 [details] Patch for landing
Comment on attachment 193098 [details] Patch for landing Clearing flags on attachment: 193098 Committed r145801: <http://trac.webkit.org/changeset/145801>
All reviewed patches have been landed. Closing bug.