Fix assertion failure in XSS Auditor
Created attachment 109108 [details] Patch
Note: the included test doesn't actually work, but I'm not sure why. If I open the .html page in a Debug build of Safari or Chromium, it trips the assertion, as expected. But when run as a test (also in Debug), no dice.
Your change looks fine. A test that triggers the error in the testing harness would be better, of course.
(In reply to comment #3) > Your change looks fine. A test that triggers the error in the testing harness would be better, of course. If you could help me arrive at such a test, I would appreciate it :)
I'm sorry that I don't have time to help you find such a test. I'd probably start out by understanding what's different about the test harness and in-browser.
Comment on attachment 109108 [details] Patch Have you tried using an HTTP test? That might behave differently than a test like this one that uses the file system.
Created attachment 109361 [details] Patch
(In reply to comment #5) > I'm sorry that I don't have time to help you find such a test. I'd probably start out by understanding what's different about the test harness and in-browser. Thanks to a bunch of help from dbates@ (dydx), the test now properly tests the assertion. Hooray!
Comment on attachment 109361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109361&action=review Thanks for taking the time to add a test case that works. > LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:1 > +<html> Nit: We should add the HTML5 doctype, <!DOCTYPE html>, since this page doesn't need quirks mode. > LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:3 > + <script src="../js/resources/js-test-pre.js"></script> As far as I can tell we don't use any functionality that is provided by this external script. Please remove it.
Comment on attachment 109361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109361&action=review > LayoutTests/ChangeLog:10 > + * fast/forms/xss-auditor-doesnt-crash.html: Added. This test name seems a bit vague. Maybe, xss-auditor-doesnt-crash-on-post-submit-of-empty-form? > LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:24 > + This tests that no assertions fire when submitting a form. This text is a bit vague. Maybe "This tests that no assertions is thrown when POST submitting an empty form."?
Created attachment 109365 [details] Patch
Created attachment 109367 [details] Patch
Comment on attachment 109361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109361&action=review >> LayoutTests/ChangeLog:10 >> + * fast/forms/xss-auditor-doesnt-crash.html: Added. > > This test name seems a bit vague. Maybe, xss-auditor-doesnt-crash-on-post-submit-of-empty-form? This previously crashed with non-empty forms as well, so renamed to xss-auditor-doesnt-crash-on-post-submit >> LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:1 >> +<html> > > Nit: We should add the HTML5 doctype, <!DOCTYPE html>, since this page doesn't need quirks mode. Done. >> LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:3 >> + <script src="../js/resources/js-test-pre.js"></script> > > As far as I can tell we don't use any functionality that is provided by this external script. Please remove it. Added testPassed() call. >> LayoutTests/fast/forms/xss-auditor-doesnt-crash.html:24 >> + This tests that no assertions fire when submitting a form. > > This text is a bit vague. Maybe "This tests that no assertions is thrown when POST submitting an empty form."? Done.
Comment on attachment 109367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109367&action=review > LayoutTests/fast/forms/xss-auditor-doesnt-crash-on-post-submit.html:4 > + <script src="../js/resources/js-test-pre.js"></script> This test is good enough as-is. Nit: You're under utilizing js-test-pre.js and the other JS unit test scripts (i.e. js-test-post.js) which could be taken advantage of to reduce the amount of code in this test.
(In reply to comment #13) > >> LayoutTests/ChangeLog:10 > >> + * fast/forms/xss-auditor-doesnt-crash.html: Added. > > > > This test name seems a bit vague. Maybe, xss-auditor-doesnt-crash-on-post-submit-of-empty-form? > > This previously crashed with non-empty forms as well, so renamed to xss-auditor-doesnt-crash-on-post-submit > We should further investigate this.
Comment on attachment 109367 [details] Patch Clearing flags on attachment: 109367 Committed r96440: <http://trac.webkit.org/changeset/96440>
All reviewed patches have been landed. Closing bug.
(In reply to comment #15) > (In reply to comment #13) > > >> LayoutTests/ChangeLog:10 > > >> + * fast/forms/xss-auditor-doesnt-crash.html: Added. > > > > > > This test name seems a bit vague. Maybe, xss-auditor-doesnt-crash-on-post-submit-of-empty-form? > > > > This previously crashed with non-empty forms as well, so renamed to xss-auditor-doesnt-crash-on-post-submit > > > > We should further investigate this. I probably won't have time to investigate further; but if you're interested in doing so, it might help to start from the repro case in https://code.google.com/p/chromium/issues/detail?id=97346.