Closed Bug 558220 Opened 15 years ago Closed 13 years ago

Intermittent test_bug435425.html | Wrong responseText - got Bad request

Categories

(Testing :: General, defect)

Product:
Testing
Component:
General
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox11 affected, firefox15 verified)

Status:
RESOLVED FIXED
Milestone:
mozilla16
Tracking Flags:
Tracking Status
firefox11 --- affected
firefox15 --- verified

People

(Reporter: zpao, Assigned: bzbarsky)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

part 1. Add more debugging spew to the test HTTP server.
9.40 KB, patch
Waldo
: review+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review
part 2. Fix a bug in the test HTTP header that caused it to misread headers when a packet boundary fell between the CR and LF at the end of a header value.
1.16 KB, patch
Waldo
: review+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review
24089 INFO TEST-PASS | /tests/content/base/test/test_bug435425.html | Wrong event! 24090 INFO TEST-PASS | /tests/content/base/test/test_bug435425.html | Wrong event target [[object XMLHttpRequest @ 0xbc72378 (native @ 0xbad4f18)],error]! 24091 INFO TEST-PASS | /tests/content/base/test/test_bug435425.html | Extra or wrong event? 24092 INFO TEST-PASS | /tests/content/base/test/test_bug435425.html | Wrong event! 24093 INFO TEST-PASS | /tests/content/base/test/test_bug435425.html | Wrong event target [[object XMLHttpRequest @ 0xcad3730 (native @ 0xab2c558)],loadstart]! 24094 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug435425.html | Wrong responseText - got (65537 x "A"), expected (65539 x "A") 24095 INFO TEST-PASS | /tests/content/base/test/test_bug435425.html | Extra or wrong event? 24096 INFO TEST-PASS | /tests/content/base/test/test_bug435425.html | Wrong event! http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270768602.1270770067.5257.gz Linux mozilla-central debug test mochitests-1/5 on 2010/04/08 16:16:42
So, um, did something change in responseText handling lately? Or is there something else happening here.
Nothing that I know of....
Summary: mochitest-plain: test_bug435425.html | Wrong responseText → mochitest-plain: intermittent "test_bug435425.html | Wrong responseText - got Bad request"
Summary: mochitest-plain: intermittent "test_bug435425.html | Wrong responseText - got Bad request" → Intermittent test_bug435425.html | Wrong responseText - got Bad request
smaug: Do you have any ideas about this? Currently the #5 orange failure on mozilla-inbound.
Looks like this bug is used for different problems. The original was about events, the latest ones are about bad request.
Is there a bug in httpd.js? Why is it returning "Bad request"?
Attachment #637361 - Flags: review?(jwalden+bmo)
Assignee: nobody → bzbarsky
Component: DOM → httpd.js
Product: Core → Testing
QA Contact: general → httpd.js
Whiteboard: [orange] → [orange][need review]
Comment on attachment 637361 [details] [diff] [review] part 1. Add more debugging spew to the test HTTP server. Review of attachment 637361 [details] [diff] [review]: ----------------------------------------------------------------- Braces in httpd.js are always on their own lines in httpd.js, throughout. (Also equality should be === or !==, but those were preexisting, so no need for you specifically to fix them now specifically.) ::: netwerk/test/httpserver/httpd.js @@ +1665,5 @@ > if (scheme === "http") > port = 80; > else if (scheme === "https") > port = 443; > + else { If the else here is braced, the if-body and else if-body above should be braced. (Braces on their own lines as previously noted.) @@ +1777,5 @@ > // multi-line header if we've already seen a header line > if (!lastName) > { > // we don't have a header to continue! > + dumpn("No header to continue"); Might as well move the comment-text into the dumpn if you're going to do this, and not repeat yourself. @@ +1810,1 @@ > throw HTTP_400; And here.
Attachment #637361 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 637363 [details] [diff] [review] part 2. Fix a bug in the test HTTP header that caused it to misread headers when a packet boundary fell between the CR and LF at the end of a header value. Review of attachment 637363 [details] [diff] [review]: ----------------------------------------------------------------- Please add a test to netwerk/test/httpserver/test/test_linedata.js along these (untested!) lines (don't forget to update the .ini file too): /* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ /* This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ // test that the LineData internal data structure works correctly function run_test() { var data = new LineData(); data.appendBytes(['a', CR]); var out = { value: "" }; do_check_false(data.readLine(out)); data.appendBytes([LF]); do_check_true(data.readLine(out)); do_check_eq(out.value, "a"); } ::: netwerk/test/httpserver/httpd.js @@ +1903,5 @@ > var length = findCRLF(data, this._start); > if (length < 0) > { > this._start = data.length; > + // But if our data ends in a CR, we have to back up one, because Blank line before the start of the comment. You could also common up some, but I wrote it out, and it looks noisier than backing up after the fact, to me at least. @@ +1908,5 @@ > + // the first byte in the next packet might be an LF and if we > + // start looking at data.length we won't find it. > + if (data[data.length - 1] == CR) { > + --this._start; > + } No braces here, it's a single-line condition, single-line consequent. === rather than ==, please. It's entirely possible for data to be zero-length here, I believe. Technically the equality won't hold in that case. But even still, I'd rather see this conditioned on |data.length > 0| than silently depending on underflowing behavior happening to do the right thing.
Attachment #637363 - Flags: review?(jwalden+bmo) → review+
With the review comments applied and the test added: https://hg.mozilla.org/integration/mozilla-inbound/rev/4918f2685b05 https://hg.mozilla.org/integration/mozilla-inbound/rev/72c50592b3a7
Whiteboard: [orange][need review] → [orange]
Target Milestone: --- → mozilla16
<3 bz Thank you so much for taking a look at this :-)
https://hg.mozilla.org/mozilla-central/rev/4918f2685b05 Sorry, this merged yesterday, but I completely forgot to do the bug marking: https://hg.mozilla.org/mozilla-central/rev/72c50592b3a7
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 776945
Comment on attachment 637361 [details] [diff] [review] part 1. Add more debugging spew to the test HTTP server. [Approval Request Comment] Bug caused by (feature/regressing bug #): Intermittent orange cause by a bug in the test HTTP header that caused it to misread headers when a packet boundary fell between the CR and LF at the end of a header value. User impact if declined: More [orange] tbpl noise on mozilla-beta Testing completed (on m-c, etc.): on m-c + aurora Risk to taking this patch (and alternatives if risky): None; test-only change. String or UUID changes made by this patch: None
Attachment #637361 - Flags: approval-mozilla-beta?
Comment on attachment 637363 [details] [diff] [review] part 2. Fix a bug in the test HTTP header that caused it to misread headers when a packet boundary fell between the CR and LF at the end of a header value. [Approval Request Comment] Bug caused by (feature/regressing bug #): Intermittent orange cause by a bug in the test HTTP header that caused it to misread headers when a packet boundary fell between the CR and LF at the end of a header value. User impact if declined: More [orange] tbpl noise on mozilla-beta Testing completed (on m-c, etc.): on m-c + aurora Risk to taking this patch (and alternatives if risky): None; test-only change. String or UUID changes made by this patch: None
Attachment #637363 - Flags: approval-mozilla-beta?
Comment on attachment 637361 [details] [diff] [review] part 1. Add more debugging spew to the test HTTP server. test-only change, approving.
Attachment #637361 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 637363 [details] [diff] [review] part 2. Fix a bug in the test HTTP header that caused it to misread headers when a packet boundary fell between the CR and LF at the end of a header value. test-only change, approving.
Attachment #637363 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [orange]
Component: httpd.js → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: