When I send a url of the format http://user@server/path into the webkit (using the QWebView::load() method), when the code gets to ResourceHandle::start(), the method uses the class's internal "d" pointer instead of using getInternal(). See ResourceHandleQt.cpp, lines 134-147 in Qt 4.6.1. At line 147 the method calls getInternal() to get a locally defined "d", but above it is using the object's "d", which is not initialized. See also loadResourceSynchronously(), which properly sets a local "d" pointer from which to get the user/password. The net result is that by the time the network reply handler gets the URL, it does not have authentication data.
Bugs relating to the Qt port of WebKit should have the Qt keyword. See http://trac.webkit.org/wiki/QtWebKitBugs
AFAICS d and the return value of getInternal() point to the same thing: ResourceHandle's private data pointer. I agree the last call to getInternal() is redundant, but otherwise I don't see the bug. What am I missing?
(In reply to comment #2) > AFAICS d and the return value of getInternal() point to the same thing: > ResourceHandle's private data pointer. > > I agree the last call to getInternal() is redundant, but otherwise I don't see > the bug. What am I missing? That I'm getting mislead by my debugger. I think that the actual bug might be the if statement on 134: if (!(d->m_user.isEmpty() || d->m_pass.isEmpty()) If a username is provided but not a password (which is my case), if (!(false || true)) == if (!(true)) == if (false). Should this and other user/password checks be: if (!(d->m_user.isEmpty() && d->m_pass.isEmpty())
(In reply to comment #3) > (In reply to comment #2) > > AFAICS d and the return value of getInternal() point to the same thing: > > ResourceHandle's private data pointer. > > > > I agree the last call to getInternal() is redundant, but otherwise I don't see > > the bug. What am I missing? > > That I'm getting mislead by my debugger. I think that the actual bug might be > the if statement on 134: > > if (!(d->m_user.isEmpty() || d->m_pass.isEmpty()) > > If a username is provided but not a password (which is my case), if (!(false || > true)) == if (!(true)) == if (false). Should this and other user/password > checks be: > > if (!(d->m_user.isEmpty() && d->m_pass.isEmpty()) Yes, exactly! :) Or slightly more readable: if (!d->m_user.isEmpty() || !d->m_pass.isEmpty()) { Want to make a patch with ChangeLog and testcase/unit test in WebKit/qt/tests?
Can't this be tested with cross-platform tests? I'm actually surprised that we don't have an XMLHttpRequest test for empty password.
Created attachment 74709 [details] Patch
Note that the case where you have no username but do have a password is broken, this is due to a bug in Qt which has been separately reported in http://bugreports.qt.nokia.com/browse/QTBUG-15566. The test case for this has been added to the Skipped file in the proposed patch for the time being.
what about gtk and chromium port? no need to skip? if so, what is the point of adding a test that do not run on any DRT?
+# mac presents an authentication dialog when no username or no password is present +http/tests/xmlhttprequest/basic-auth-nouser.html +http/tests/xmlhttprequest/basic-auth-nopassword.html I'm not quite sure what behavior you are after, but it's possible to simulate authentication dialog handling in DumpRenderTree with layoutTestController.setAuthenticationUsername()/setAuthenticationPassword().
(In reply to comment #8) > what about gtk and chromium port? no need to skip? if so, what is the point of adding a test that do not run on any DRT? Works fine on chromium. I just built gtk and that one seems to have the same issue as on Mac, meaning that this test implementation would have to be skipped there as well.
(In reply to comment #9) > I'm not quite sure what behavior you are after, but it's possible to simulate authentication dialog handling in DumpRenderTree with layoutTestController.setAuthenticationUsername()/setAuthenticationPassword(). Ah right, that's probably what I should do then. I don't need to test the XMLHttpRequest behavior in particular. I'll update the test and propose a new patch. Thanks.
Created attachment 74792 [details] Patch
(In reply to comment #12) > Created an attachment (id=74792) [details] > Patch I'm not entirely sure if it's safe to use http://localhost:8000/ in a test, will the http server always listen on that port? Works fine on mac, qt and chromium with the default settings at least.
Created attachment 74798 [details] Patch
For future reference, see Jan Erik's comment in <http://bugreports.qt.nokia.com/browse/QTBUG-15566>: "certain routers (at least recent Cisco models like the E2100L) require you to log in with username blank and password 'admin' in order to change the username / password to something else. This won't work using QtWebKit." > I'm not entirely sure if it's safe to use http://localhost:8000/ in a test Theoretically, there is a way to change the port in run-webkit-tests, but a lot of existing tests just hardcode 8000. So yes, it's OK. + * platform/mac-leopard/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added. + * platform/mac-snowleopard/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added. + * platform/mac-tiger/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added. I don't see any difference between these results, are there any? Otherwise, the -expected file should just go to platform/mac/http/tests/xmlhttprequest. + layoutTestController.queueLoad("http://:password@localhost:8000/xmlhttprequest/resources/basic-auth-nouserpass/basic-auth-nouserpass.php"); I don't know what queueLoad is - but the explanatory text says "Tests for XMLHttpRequest authentication", so maybe you should actually use XMLHttpRequest? + layoutTestController.setHandlesAuthenticationChallenges(true); + layoutTestController.setAuthenticationPassword("password"); Since we're really after empty username, perhaps it would be cleaner to call setAuthenticationUsername(""), too.
(In reply to comment #15) > + * platform/mac-leopard/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added. > + * platform/mac-snowleopard/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added. > + * platform/mac-tiger/http/tests/xmlhttprequest/basic-auth-nopassword-expected.txt: Added. > > I don't see any difference between these results, are there any? Otherwise, the -expected file should just go to platform/mac/http/tests/xmlhttprequest. Yes, that's what I thought as well. However, when doing that then Chromium picks up that expected file for some reason and the test then fails. This is as of revision 72696. > + layoutTestController.queueLoad("http://:password@localhost:8000/xmlhttprequest/resources/basic-auth-nouserpass/basic-auth-nouserpass.php"); > > I don't know what queueLoad is - but the explanatory text says "Tests for XMLHttpRequest authentication", so maybe you should actually use XMLHttpRequest? I had some issues with XMLHttpRequest on mac, it looked like the initial request would not get the username picked up, I had to schedule two of them. With queueLoad() the test passes on all platforms except GTK when just using one request. > + layoutTestController.setHandlesAuthenticationChallenges(true); > + layoutTestController.setAuthenticationPassword("password"); > > Since we're really after empty username, perhaps it would be cleaner to call setAuthenticationUsername(""), too. Yes, I can do that.
(In reply to comment #16) > (In reply to comment #15) > > I don't see any difference between these results, are there any? Otherwise, the -expected file should just go to platform/mac/http/tests/xmlhttprequest. > > Yes, that's what I thought as well. However, when doing that then Chromium picks up that expected file for some reason and the test then fails. This is as of revision 72696. Looks like I can work around this by putting the default expected file into platform/chromium/http/tests/xmlhttprequest/
> I had some issues with XMLHttpRequest on mac, it looked like the initial request would not get the username picked up, Nate (CC'ed) might have a comment about that. Generally, for a non-trivial test like this, it's best to make it work outside DumpRenderTree as an option, so that we could compare behavior with other browsers. > Looks like I can work around this by putting the default expected file into platform/chromium/http/tests/xmlhttprequest/ That's the right thing to do when Chromium results don't match Mac.
(In reply to comment #18) > > I had some issues with XMLHttpRequest on mac, it looked like the initial request would not get the username picked up, > > Nate (CC'ed) might have a comment about that. > > Generally, for a non-trivial test like this, it's best to make it work outside DumpRenderTree as an option, so that we could compare behavior with other browsers. Right. I had a look at this again and I still can't make XMLHttpRequest work the way I want on mac. If I don't call layoutTestController.setAuthenticationUsername() then no requests will get the username picked up, but if I do call that function then the first one still fails but subsequent ones succeeds. Interrestingly enough the password-only case succeeds on mac, even with no calls to setAuthenticationPassword().
Comment on attachment 74798 [details] Patch LGTM.
Comment on attachment 74798 [details] Patch Rejecting attachment 74798 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--no-update', '--non-interactive', 74798]" exit_code: 2 Last 500 characters of output: xmlhttprequest/basic-auth-nopassword-expected.txt patching file LayoutTests/platform/qt/Skipped Hunk #1 succeeded at 275 with fuzz 2 (offset -14 lines). patching file WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebCore/platform/network/qt/ResourceHandleQt.cpp Hunk #1 succeeded at 124 (offset 1 line). Hunk #2 succeeded at 186 (offset -6 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/7239143
Created attachment 77390 [details] Patch Fix the patch not applying problem
Manually landed in r74609 http://trac.webkit.org/changeset/74609
Comment on attachment 77390 [details] Patch Clearing the r and cq flags.
http://trac.webkit.org/changeset/74609 might have broken GTK Linux 64-bit Debug
GTK bot should be fixed in r74613: http://trac.webkit.org/changeset/74613