468467 - If onInputStreamReady doesn't process all of the Request-Line the first time, httpd.js asserts

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch and a couple tests [Checkin: Comment 7]

I noticed this during an intermediate state in writing the first patch for bug 468087.  The browser usually spews the entire request at once, but in that case I mangled a Request-Line to be super-super-long, and it had to be split when the server got it, resulting in an assertion failure that the state at the start of _processHeaders was correct.  It's easy to verify patch correctness -- just look at the API docs for _processRequestLine!  :-)

This is a mistake from earlier iterations of the patch in bug 462864 when I hadn't quite grasped exactly what the meaning of the return value would be, for what it's worth.

(Ted says Core:Testing is dead, but it still seems the most accurate component for httpd.js bugs; the server's used in too many places to qualify for any one component in Testing, really.  A new component in Testing would be a great solution [and an even greater way to stroke own my ego!], but I don't think httpd.js bugs are quite common enough to warrant it.)

Comment 1

[Honza Bambas (:mayhemer)]

Comment on attachment 351918 [details] [diff] [review]
Patch and a couple tests
[Checkin: Comment 7]

Oooops, I overlooked that... 

Is there any platform with MTU larger then 16k? CentOS 5.2 gives me 16436, Mac 10.4 about 8160, WinXP 1492. The blankline test could not be sufficient in some limit cases, probably raise from 13 to 14? 

Also, adding '[diff] unified=10' or so and hg 1.0.1 could help produce patches with larger context.

Comment 2

[Jeff Walden [:Waldo]]

Sure, I can raise it.  Frankly, as long as it trips *somewhere*, I'm not especially worried (cf. the constant 250 in the patch for bug 465921).

No worries about overlooking, either; I'm still probably the only person who really understands the entire server (although I hope someone could get to the same point if they really had to), and people I try to get up to speed on it are inevitably going to miss things occasionally.

Re context, I have it set up for diff that way but not for mq diffs, and I upload directly from .hg/patches.  I'm sure I've seen stuff about this in #hg at some point, but the flip was avoiding unnecessary conflicts as much as possible, and I'm unlikely to change that in the near future.

Comment 3

[Jeff Walden [:Waldo]]

Er, that should have been the constant 250 in bug 463254.

Comment 4

[Jeff Walden [:Waldo]]

Moving httpd.js bugs to the new Testing :: httpd.js component; filter out this bugmail by searching for "ICanHasHttpd.jsComponent".

Comment 5

[Jeff Walden [:Waldo]]

...and changing the QA contact as well.  Filter on the string "BugzillaQAContactHandlingIsStupid".

Comment 6

[Jeff Walden [:Waldo]]

Fixed on trunk, waiting for tree to cycle to land on 191 to keep things sync'd, as this is test-only code and I'd like to avoid divergence while both are undergoing active development...

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 351918 [details] [diff] [review]
Patch and a couple tests
[Checkin: Comment 7]

(In reply to comment #6)
> Fixed on trunk, waiting for tree to cycle to land on 191 to keep things sync'd,

http://hg.mozilla.org/mozilla-central/rev/469f2ddc1df5
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8c8869f150d2

Comment 8

[Jeff Walden [:Waldo]]

Serge, please don't modify bug statuses and such things on my bugs when it's not clear I've completely forgotten about them.  In particular, it hasn't been that long since this was committed to 191, and I was still watching just to be sure nothing adverse was going to happen.

Comment 9

[Jeff Walden [:Waldo]]

The changes to head_utils.js introduced a buglet that strangely doesn't result in test failures (or at least not enough for anyone to have reported it as a bug or pinged me).  See bug 508128 comment 59, to be fixed there fairly soon.