Bug 69050 - Fix assertion failure in XSS Auditor
Summary: Fix assertion failure in XSS Auditor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ilya Sherman
URL:
Keywords: XSSAuditor
Depends on:
Blocks:
 
Reported: 2011-09-28 18:01 PDT by Ilya Sherman
Modified: 2011-10-03 12:54 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.17 KB, patch)
2011-09-28 18:13 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (4.61 KB, patch)
2011-09-30 15:51 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (4.66 KB, patch)
2011-09-30 16:10 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff
Patch (4.81 KB, patch)
2011-09-30 16:16 PDT, Ilya Sherman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Sherman 2011-09-28 18:01:45 PDT
Fix assertion failure in XSS Auditor
Comment 1 Ilya Sherman 2011-09-28 18:13:31 PDT
Created attachment 109108 [details]
Patch
Comment 2 Ilya Sherman 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.
Comment 3 Adam Barth 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.
Comment 4 Ilya Sherman 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 :)
Comment 5 Adam Barth 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.
Comment 6 Adam Barth 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.
Comment 7 Ilya Sherman 2011-09-30 15:51:22 PDT
Created attachment 109361 [details]
Patch
Comment 8 Ilya Sherman 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!
Comment 9 Daniel Bates 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.
Comment 10 Daniel Bates 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."?
Comment 11 Ilya Sherman 2011-09-30 16:10:28 PDT
Created attachment 109365 [details]
Patch
Comment 12 Ilya Sherman 2011-09-30 16:16:17 PDT
Created attachment 109367 [details]
Patch
Comment 13 Ilya Sherman 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.
Comment 14 Daniel Bates 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.
Comment 15 Daniel Bates 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.
Comment 16 WebKit Review Bot 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>
Comment 17 WebKit Review Bot 2011-09-30 17:24:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Ilya Sherman 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.