RESOLVED FIXED69050
Fix assertion failure in XSS Auditor 
https://bugs.webkit.org/show_bug.cgi?id=69050
Summary Fix assertion failure in XSS Auditor
Ilya Sherman
Reported 2011-09-28 18:01:45 PDT
Fix assertion failure in XSS Auditor
Attachments
Patch (4.17 KB, patch)
2011-09-28 18:13 PDT, Ilya Sherman 		no flags
DetailsFormatted DiffDiff
Patch (4.61 KB, patch)
2011-09-30 15:51 PDT, Ilya Sherman 		no flags
DetailsFormatted DiffDiff
Patch (4.66 KB, patch)
2011-09-30 16:10 PDT, Ilya Sherman 		no flags
DetailsFormatted DiffDiff
Patch (4.81 KB, patch)
2011-09-30 16:16 PDT, Ilya Sherman 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Ilya Sherman
Comment 1 2011-09-28 18:13:31 PDT
Created attachment 109108 [details] Patch
Ilya Sherman
Comment 2 2011-09-28 18:14:36 PDT
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.
Adam Barth
Comment 3 2011-09-28 19:48:05 PDT
Your change looks fine. A test that triggers the error in the testing harness would be better, of course.
Ilya Sherman
Comment 4 2011-09-28 19:48:51 PDT
(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 :)
Adam Barth
Comment 5 2011-09-28 19:52:58 PDT
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.
Adam Barth
Comment 6 2011-09-28 19:53:38 PDT
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.
Ilya Sherman
Comment 7 2011-09-30 15:51:22 PDT
Created attachment 109361 [details] Patch
Ilya Sherman
Comment 8 2011-09-30 15:52:24 PDT
(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!
Daniel Bates
Comment 9 2011-09-30 15:58:30 PDT
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.
Daniel Bates
Comment 10 2011-09-30 16:09:32 PDT
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."?
Ilya Sherman
Comment 11 2011-09-30 16:10:28 PDT
Created attachment 109365 [details] Patch
Ilya Sherman
Comment 12 2011-09-30 16:16:17 PDT
Created attachment 109367 [details] Patch
Ilya Sherman
Comment 13 2011-09-30 16:17:28 PDT
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.
Daniel Bates
Comment 14 2011-09-30 16:19:19 PDT
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.
Daniel Bates
Comment 15 2011-09-30 16:26:35 PDT
(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.
WebKit Review Bot
Comment 16 2011-09-30 17:24:17 PDT
Comment on attachment 109367 [details] Patch Clearing flags on attachment: 109367 Committed r96440: <http://trac.webkit.org/changeset/96440>
WebKit Review Bot
Comment 17 2011-09-30 17:24:22 PDT
All reviewed patches have been landed. Closing bug.
Ilya Sherman
Comment 18 2011-10-03 12:54:51 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.