Bug 108394

Created attachment 185605 [details] Patch

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

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

Created attachment 185830 [details] Patch

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

Created attachment 185860 [details] Patch

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 on attachment 185860 [details] Patch Clearing flags on attachment: 185860 Committed r141494: <http://trac.webkit.org/changeset/141494>

All reviewed patches have been landed. Closing bug.