Bug 29322 – REGRESSION: http/tests/xmlhttprequest/cross-origin-authorization.html is failing/crashing intermittently

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

http/tests/xmlhttprequest/cross-origin-authorization.html is failing intermittently

It failed for the commit-queue when trying to land bug 

and has failed on the bots today as well:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r48438%20(5043)/results.html

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Created attachment 39675 [details]
diff of failure

[Eric Seidel (no email)]

(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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Alexey Proskuryakov]

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

[Brady Eidson]

Only Leopard?

[Brady Eidson]

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.

[Eric Seidel (no email)]

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.

[Darin Adler]

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

[Brady Eidson]

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.

[Alexey Proskuryakov]

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

r=me!

[Eric Seidel (no email)]

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!

[Brady Eidson]

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...

[Eric Seidel (no email)]

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.

[Brady Eidson]

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  :)

[Alexey Proskuryakov]

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

[Eric Seidel (no email)]

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!

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Alexey Proskuryakov]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Alexey Proskuryakov]

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

[Eric Seidel (no email)]

(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. :)

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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. :)

[Alexey Proskuryakov]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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. :(

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Claimed another victim.  bug 28054.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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!

[Alexey Proskuryakov]

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.

[Eric Seidel (no email)]

Thank you Alexey for investigating!

[Eric Seidel (no email)]

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

[Alexey Proskuryakov]

<rdar://problem/7259965>

[Alexey Proskuryakov]

Created attachment 40276 [details]
proposed fix

[Oliver Hunt]

Comment on attachment 40276 [details]
proposed fix

Why are there two copies of encodeBasicAuthorization added? surely one shared
implementation would be superior?

[Brady Eidson]

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.

[Alexey Proskuryakov]

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.

[Eric Seidel (no email)]

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