Bug 32521 – [Qt] LayoutTests/http/tests/xmlhttprequest/access-control-basic-denied-preflight-cache.html fails in manual test

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Robert Hogan]

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.

[Robert Hogan]

Created attachment 44799 [details]
move to skipped list

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44799 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 44799 [details]
move to skipped list

OK.

[WebKit Commit Bot]

Comment on attachment 44799 [details]
move to skipped list

Clearing flags on attachment: 44799

Committed r52109: <http://trac.webkit.org/changeset/52109>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Robert Hogan]

Needs to stay open until fix is identified.

[Robert Hogan]

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.

[Robert Hogan]

Created attachment 45294 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45294 [details] without any errors.

[Eric Seidel (no email)]

I do not understand why Qt's behavior should diverge here.

[Robert Hogan]

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

[Robert Hogan]

Created attachment 45348 [details]
Change HTTP Method in test

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45348 [details] without any errors.

[Alexey Proskuryakov]

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.

[Robert Hogan]

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

[Robert Hogan]

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

[Alexey Proskuryakov]

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.

[Tor Arne Vestbø]

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

[Jocelyn Turcotte]

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