Bug 32521 - [Qt] LayoutTests/http/tests/xmlhttprequest/access-control-basic-denied-preflight-cache.html fails in manual test
Summary: [Qt] LayoutTests/http/tests/xmlhttprequest/access-control-basic-denied-prefli...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2009-12-14 08:37 PST by Robert Hogan
Modified: 2014-02-03 03:13 PST (History)
6 users (show)

See Also:


Attachments
move to skipped list (2.57 KB, patch)
2009-12-14 08:41 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (3.63 KB, patch)
2009-12-20 13:42 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Change HTTP Method in test (3.84 KB, patch)
2009-12-21 13:42 PST, Robert Hogan
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2009-12-14 08:37:12 PST
LayoutTests/http/tests/xmlhttprequest/access-control-basic-denied-preflight-cache.html

This passes the automatic layout tests in Qt due to a bug with notifyDone. (see https://bugs.webkit.org/show_bug.cgi?id=32437)

Performing the test manually in QtLauncher shows that an exception is received.
Comment 1 Robert Hogan 2009-12-14 08:41:55 PST
Created attachment 44799 [details]
move to skipped list
Comment 2 WebKit Review Bot 2009-12-14 08:46:24 PST
style-queue ran check-webkit-style on attachment 44799 [details] without any errors.
Comment 3 Eric Seidel (no email) 2009-12-14 11:21:13 PST
Comment on attachment 44799 [details]
move to skipped list

OK.
Comment 4 WebKit Commit Bot 2009-12-14 12:28:31 PST
Comment on attachment 44799 [details]
move to skipped list

Clearing flags on attachment: 44799

Committed r52109: <http://trac.webkit.org/changeset/52109>
Comment 5 WebKit Commit Bot 2009-12-14 12:28:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Robert Hogan 2009-12-14 14:10:53 PST
Needs to stay open until fix is identified.
Comment 7 Robert Hogan 2009-12-20 13:40:40 PST
try {
        xhr.open("FOO", "http://localhost:8000/xmlhttprequest/resources/access-control-basic-denied.cgi");
        xhr.send();
    } catch (e) {
        log("Got exception.");
    }

All DRTs, except Qt, return a simple 'PASS' for this test, but Qt throws an exception and the result includes 'Got exception'.

The reason Qt throws an exception is the following code in  start() in QtNetworkReplyHandler.cpp:

        case QNetworkAccessManager::UnknownOperation: {
            m_reply = 0;
            ResourceHandleClient* client = m_resourceHandle->client();
            if (client) {
                ResourceError error(url.host(), 400 /*bad request*/,
                                    url.toString(),
                                    QCoreApplication::translate("QWebPage", "Bad HTTP request"));
                client->didFail(m_resourceHandle, error);
            }
            return;
        }
    }

The HTTP method used in the test ("FOO") causes the request to enter the above clause and Qt to register an exception with client->didFail.

So Qt's behaviour is valid on its own terms and the test passes - Qt also returns a 'PASS' in common with the other DRTs.

For this reason, propose to create platform-specific expected results for this test and remove from the skipped list.
Comment 8 Robert Hogan 2009-12-20 13:42:02 PST
Created attachment 45294 [details]
Patch
Comment 9 WebKit Review Bot 2009-12-20 13:43:17 PST
style-queue ran check-webkit-style on attachment 45294 [details] without any errors.
Comment 10 Eric Seidel (no email) 2009-12-21 00:24:38 PST
I do not understand why Qt's behavior should diverge here.
Comment 11 Robert Hogan 2009-12-21 13:41:41 PST
(In reply to comment #10)
> I do not understand why Qt's behavior should diverge here.

The test uses "FOO" as the HTTP method - which QNetworkReplyHandler.cpp rejects as invalid and throws an exception. I had a look at the Gtk client and it uses libsoup, which as far as I can tell does not attempt to validate the HTTP method.

Changing the method to "GET" in the test does not appear to alter the result for GTK. If I understand the test correctly, the request method isn't pertinent so instead propose to change the test to use "GET".
Comment 12 Robert Hogan 2009-12-21 13:42:38 PST
Created attachment 45348 [details]
Change HTTP Method in test
Comment 13 WebKit Review Bot 2009-12-21 13:44:14 PST
style-queue ran check-webkit-style on attachment 45348 [details] without any errors.
Comment 14 Alexey Proskuryakov 2009-12-21 14:56:01 PST
Comment on attachment 45348 [details]
Change HTTP Method in test

GET is a "simple" method as far as cross-origin XMLHttpRequest is concerned, so its behavior is different from what we'd get with FOO.

This just looks like a QNetworkReplyHandler bug to me - all HTTP method names are equally valid.
Comment 15 Robert Hogan 2009-12-22 12:14:57 PST
(In reply to comment #14)
> (From update of attachment 45348 [details])
> GET is a "simple" method as far as cross-origin XMLHttpRequest is concerned, so
> its behavior is different from what we'd get with FOO.
> 
> This just looks like a QNetworkReplyHandler bug to me - all HTTP method names
> are equally valid.

Mmm, then I think this test is not testing what it thinks it's testing. Judging by the title I think it's expecting to reach:

        if (CrossOriginPreflightResultCache::shared().canSkipPreflight(document->securityOrigin()->toString(), request.url(), m_options.allowCredentials, request.httpMethod(), request.httpHeaderFields()))
            preflightSuccess();
        else
            makeCrossOriginAccessRequestWithPreflight(request);

in the DocumentThreadableLoader constructor and call makeCrossOriginAccessRequestWithPreflight(request);

However the request is marked as m_sameOriginRequest because allowUniversalAccessFromFileURLs() is set to true in WebKit by default - as a result it is creating the request in:

    if (m_sameOriginRequest || m_options.crossOriginRequestPolicy == AllowCrossOriginRequests) {
        loadRequest(request, DoSecurityCheck);
        return;
    }

in the DocumentThreadableLoader constructor instead.

So the test needs to set UniversalAccessFromFileUrls to false in order to work as intended. However, even this won't work without a patch because calling layoutTestController.setUniversalAccessFromFileUrls() gets called after the securityOrigin() of the document has been initialized, so the new value doesn't get used for the test. This requires a patch to SecurityOrigin.cpp to provide a method for the DRT to revoke universal access immediately the setting is updated.
Comment 16 Robert Hogan 2009-12-22 14:10:58 PST
(In reply to comment #14)
> (From update of attachment 45348 [details])
> GET is a "simple" method as far as cross-origin XMLHttpRequest is concerned, so
> its behavior is different from what we'd get with FOO.
> 
> This just looks like a QNetworkReplyHandler bug to me - all HTTP method names
> are equally valid.

OK, so even with the changes in place from my previous response Qt will still throw an exception due to the unrecognized HTTP method - which in makeCrossOriginAccessRequestWithPreflight() is 'OPTIONS'. QtWebKit doesn't support anything apart from GET/POST/PUT/HEAD/DELETE. I guess this test should remain skipped in Qt until it explicitly supports the OPTIONS method - until it does it won't hit the code this test is meant to validate.

All that said, the more I look at the code the more I think the proper result should include a 'Got exception', though for different reasons to the one Qt is producing it. The test is meant to hit preflightFailure() at some point and that will cause the test to catch an exception. Presumably libsoup in GTK can handle OPTIONS appropriately, I'll fix the gtk port to disable universalAccessFromFiles during the test run and see what happens there.
Comment 17 Alexey Proskuryakov 2009-12-22 15:45:49 PST
This test is in http subdirectory, which means that run-webkit-tests runs it from a local Apache instance (127.0.0.1:8000). Thus, the universal access from file URLs setting won't affect it in any way.
Comment 18 Tor Arne Vestbø 2010-03-10 06:27:25 PST
Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should only be used for bugs/features in the
    public QtWebKit API layer, not to signify that the bug is specific to
    the Qt port of WebKit

      http://trac.webkit.org/wiki/QtWebKitBugs#Component

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords
Comment 19 Jocelyn Turcotte 2014-02-03 03:13:16 PST
=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.