Bug 29322

Sorry, I meant "when trying to land bug 29319" :)

Created attachment 39675 [details] diff of failure

(I've marked this as a regression because I've only seen this start to fail in the last few days. It's possible it's been failing longer, but we don't have a good way to know yet.

Speak of the devil. The most recent build again: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r48448%20(5052)/results.html

CCing folks based on: http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/xmlhttprequest/cross-origin-authorization.html

CC'ing Brady, because he did some major work in this area recently.

Only Leopard?

And do we know when the first failure was? When I last checked in work on this, I watched the bots closely for awhile and saw no Leopard failures.

The first time the commit-queue saw this failure was yesterday: https://bugs.webkit.org/show_bug.cgi?id=27846#c5 It's possible that the buildbots saw the failure before that though.

I saw this test time out on my Snow Leopard machine just now.

Created attachment 39798 [details] Proposed fix to the test to make them more reliable. This change might make them more reliable, and doesn't seem to make things worse - they all continue to pass on my machine.

Comment on attachment 39798 [details] Proposed fix to the test to make them more reliable. r=me!

Yay! Saw this again just now: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r48536%20(5140)/results.html --- layout-test-results/http/tests/xmlhttprequest/cross-origin-authorization-expected.txt 2009-09-18 15:11:01.000000000 -0700 +++ layout-test-results/http/tests/xmlhttprequest/cross-origin-authorization-actual.txt 2009-09-18 15:11:01.000000000 -0700 @@ -6,11 +6,11 @@ SCRIPT SRC='...' Should succeed, since authorization is sent for cross-origin subresource loads. PASS: Loaded, user test Cross-origin XMLHttpRequest (sync), testing stored authorization. -PASS +FAIL: 401 Authorization required Cross-origin XMLHttpRequest (sync), testing cookies. PASS Cross-origin XMLHttpRequest (async), testing stored authorization. -PASS +FAIL: Received error event. Cross-origin XMLHttpRequest (async), testing cookies. PASS Cross-origin XMLHttpRequest (sync), testing authorization with explicitly provided credentials that should be ignored. Hopefully your fix will do the trick!

Committed revision 48541. Not closing the bug, because I've never been able to reproduce this and it's intermittent for those affected, so we'll have to let it bake a little...

http/tests/xmlhttprequest/cross-origin-no-authorization.html -> failed just failed too while trying to land bug 29345. I assume these are related? I'm happy to file a separate bug.

Definitely related, but it might use a different .php script that I didn't cleanup - filing a seperate bug will make sure someone takes a closer look :)

Is there something that can be filed as a CFNetwork bug? "Connection: close" clearly shouldn't be a prerequisite for authentication working properly.

Comment on attachment 39798 [details] Proposed fix to the test to make them more reliable. This was landed as r48541 as noted above. Clearing the r+ so that it doesn't show up in the list-of-stuff-to-be-committed. :) Yay for non-flakey tests!

It looks like cross-origin-authorization.php might be the script missing the close, if Brady's theory is correct. We saw this again on the Leopard Debug bot just now: http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r48636%20(5177)/results.html

Leopard Release bot just now: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r48652%20(5247)/results.html I'm going to test to see if: run-webkit-tests --iterations 100 http/tests/xmlhttprequest/cross-origin-authorization.htm will reproduce this locally on my leopard box.

run-webkit-tests --iterations 100 http/tests/xmlhttprequest/cross-origin-authorization.html was not failing for me. run-webkit-tests --iterations 100 http/tests/xmlhttprequest Saw two failures, related to cookies: http/tests/xmlhttprequest/cookies.html -> failed http/tests/xmlhttprequest/cross-origin-cookie-storage.html -> failed I've not investigated. I expect the cookie things are an issue of test design, unrelated to this bug.

I would like to suggest that we skip http/tests/xmlhttprequest/cross-origin-authorization.html on leopard to keep the builders green. Thoughts? Failed again just now: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r48685%20(5280)/results.html

It seems that at this point, the flakiness primarily affects the commit queue. Could it have its own extended skip list? That's how a human committer would operate.

Flakiness affects us all. :) But yes, we could have a separate skipped list for the commit-queue. I'm not sure what we gain by running flakey tests and having them occasionally cause the tree to fail. It just makes the build bots harder to read/use.

Another solution would be to use test-expectation files like Chromium does and add both PASS and FAIL as an expectation for this test. http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/test_expectations.txt Sadly, it failed yet again just now: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r48693%20(5287)/results.html I think for now I'll write up a patch to skip the test on Leopard, and if you folks agree we can go ahead an make it so.

I disagree with swiping potential security issues under the carpet. They are supposed to inflict pain until fully investigated.

(In reply to comment #26) > I disagree with swiping potential security issues under the carpet. They are > supposed to inflict pain until fully investigated. I think if we have trouble forgetting to fix security issues, then we have bigger problems. :) In this case, I think tracking bugs is what the bug tracker is for. Telling us if our current build is OK is what the layout tests is for. Flakey tests make the Layout Tests less effective and thus should be turned into bugs and skipped. We should fix the bugs in the tracker(s) and rely on our Layout Tests to tell us if our builds work or not. :)

Created attachment 40034 [details] Proposal to skip tests (to keep the bots green) until the bugs can be fixed

Per Alexey's request, we've taken the discussion to webkit-dev.

Failed again just now. :( http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r48728%20(5319)/results.html

And again. :( http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r48729%20(5322)/results.html I think this one is quickly winning the "most flakey test" award. :)

As far as I can tell, this test wasn't flaky when landed. When did this problem begin to happen?

The earliest record the commit-queue has of this failure is from 11AM on 9/15/09: https://bugs.webkit.org/show_bug.cgi?id=27846#c5 I don't think the buildbot logs go back that far, but I could be wrong.

The commit-queue box was upgrade to 10.5.8 on 8/12/09, so I don't believe that's related.

The commit-bot saw this test CRASH instead of fail this evening: https://bugs.webkit.org/show_bug.cgi?id=29612#c27 That's kinda concerning. :(

I can lookup the crash log for you on Monday when I'm back in the office and post it here.

Claimed another victim. bug 28054.

Created attachment 40268 [details] crash log from crash seen while trying to land bug 29612

Comment on attachment 40268 [details] crash log from crash seen while trying to land bug 29612 if (!delegate->m_initialCredential.isEmpty()) { // Apply basic auth header String unencoded = delegate->m_initialCredential.user() + ":" + delegate->m_initialCredential.password(); CString encoded = unencoded.utf8().base64Encode(); String authHeader = String::format("Basic %s", encoded.data()); requestWithInitialCredentials.addHTTPHeaderField("Authorization", authHeader); } Is the crashing code. That could be related to the failures seen in this test. It's possible that this test is flakey due to memory issues? I'm also happy to file a separate bug for the crash if desired.

Entertainingly, two related failures occurred just now: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r48845%20(5501)/results.html Alexey said he's debugging this as we speak, so here's hoping for successful resolution!

Comment on attachment 40034 [details] Proposal to skip tests (to keep the bots green) until the bugs can be fixed I think I know what causes this; let's not skip the test.

Thank you Alexey for investigating!

Documenting frequency: Another commit-bot failure: https://bugs.webkit.org/show_bug.cgi?id=29784#c4

<rdar://problem/7259965>

Created attachment 40276 [details] proposed fix

Comment on attachment 40276 [details] proposed fix Why are there two copies of encodeBasicAuthorization added? surely one shared implementation would be superior?

Comment on attachment 40276 [details] proposed fix Yah... I'm not gonna make it a deal breaker like Olliej, but if you wanted a shared implementation, no reason that encodeBasicAuthorization couldn't be moved to the Credential class, or at least to Credential.h as a free function. Since you're always encoding the name and password from the same credential, it's logically an operation on the Credential anyways.

Committed revision 48879. I don't think that specifics of credential encoding for HTTP should be part of Credential class - its job is to hold the data, not to transfer it over HTTP. We'll probably want to factor out the code when implementing the same functionality for digest auth. The Basic case is just a few straightforward lines anyway, probably not worth having a separate file for this.

Thank you for taking this on Alexey. Here's hoping for green bots!