Closed Bug 675616 Opened 10 years ago Closed 10 years ago

Trivial fix in layout/style/test/test_css_cross_domain.html

Categories

(Testing :: General, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
mozilla8

People

(Reporter: mayhemer, Unassigned)

Details

(Whiteboard: [inbound])

Attachments

(1 file)

Attached patch v1Splinter Review
As part of the work to make httpd.js support keep-alive connections this test needs a little fix: change using of localhost:8888 to proper mochi.test:8888.

Otherwise the test cause assertions in httpd.js and fails.
Attachment #549781 - Flags: review?(zackw)
It does what it says, but could you please explain why localhost is no longer usable?
Actually I start thinking I have to rerun the test w/o this fix.  According to my notes this test had a problem but it had been fixed by another change in the tweaked httpd.js.

I will let you know and potentially close this bug as INVALID.  However, I think fixing this is anyway good think to do.
Yes, this fix is not needed to allow httpd.js to support keep-alive connections.  Two passes of all layout/style tests and the test is OK w/o the proposed fix.

However I can see on the server following line in the log:

*** unrecognized hostname (localhost:8888) in Host header, 400 time

for GET /tests/layout/style/test/ccd.sjs?JD2ls (and similar) request.  It means, the server returns 400 Bad request as a response.  If this is intentional, then let's close this bug as INVALID.
According to recheck on this fix, this no longer blocks bug 469228.
No longer blocks: 469228
Comment on attachment 549781 [details] [diff] [review]
v1

(In reply to comment #3)
> Yes, this fix is not needed to allow httpd.js to support keep-alive
> connections.  Two passes of all layout/style tests and the test is OK w/o
> the proposed fix.
> 
> However I can see on the server following line in the log:
> 
> *** unrecognized hostname (localhost:8888) in Host header, 400 time
> 
> for GET /tests/layout/style/test/ccd.sjs?JD2ls (and similar) request.  It
> means, the server returns 400 Bad request as a response.  If this is
> intentional, then let's close this bug as INVALID.

I don't believe that's intentional, and it might explain the mysterious intermittent failures seen in bug 536603.  It sounds like a bug in httpd.js, though - why should name resolution of 'localhost' ever fail?

Independent of all that, I am going to r+ your patch simply because there's no good reason for ccd-standards.html to be inconsistent with ccd-quirks.html.
Attachment #549781 - Flags: review?(zackw) → review+
... unless the server is deliberately throwing 400 Bad Request for *all* requests that specify Host: localhost because at some point there was a decision that tests should always use mochi.test instead.  (I am unaware of any such decision, but I have not been paying close attention to test-writing policy lately.)  In which case your patch fixes a bug rather than a mere inconsistency, the only thing wrong with httpd.js is it isn't clear enough about its diagnostics, and I'm a bit mystified why the test doesn't fail all the time.
It's been awhile, but I do believe the intent was to completely switch to mochi.test and not use localhost at all.  The problem is that Mochitest on mobile systems runs the web server on a remote machine, not the local machine, so "localhost" is a quite inaccurate moniker in that situation.  (localhost also requires extra-special work to proxy, since it usually should resolve to 127.0.0.1.)  The 400 error is the proper response when the Host included in the incoming request is unrecognized.  httpd.js includes some special logic to make localhost always work, at least by default; it might be the case that that logic also kicks in for Mochitest and that we should do extra work to disable it.
...or we could just ask the guy who made the localhost->mochi.test switch, of course.  :-)
we switches localhost->mochi.test for windowsCE because there was no localhost and we were running into problems (i.e. special handling of localhost and PAC settings).  All mobile mochitests run on a remote server instead of on the device.
Hm, ok, in that case test_css_cross_domain definitely should change, and maybe httpd.js's special casing for localhost should change too (to always spit an error).

I realized why this doesn't make the test fail: the test can't tell the difference between "successfully downloaded the style sheet and then discarded it because it had the wrong MIME type" and "failed to download the style sheet."  Probably not worth trying to fix that.
(In reply to comment #10)
> I realized why this doesn't make the test fail: the test can't tell the
> difference between "successfully downloaded the style sheet and then
> discarded it because it had the wrong MIME type" and "failed to download the
> style sheet."  Probably not worth trying to fix that.

It is doable: register "examine-response" observer and check the result/status of the channel for you CSS.

But really, probably not worth to do this ;)


To confirm: patch is actually r+'ed and free to land?
> To confirm: patch is actually r+'ed and free to land?

Yes.
http://hg.mozilla.org/mozilla-central/rev/95e5db0e9798
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Component: httpd.js → General
You need to log in before you can comment on or make changes to this bug.