Bug 83576 – [Qt][WK2] http/tests/navigation/https-in-page-cache.html fails with timeout

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

This test was skipped long long time ago, and was unskipped by 
http://trac.webkit.org/changeset/113427, but unfortunately it
fails on Qt5-WK2 platform (pass with Qt4.8-WK1 and Qt5-WK2)
with notifyDone timeout:


--- /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/http/tests/navigation/https-in-page-cache-expected.txt
+++ /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/http/tests/navigation/https-in-page-cache-actual.txt
@@ -1,7 +1,2 @@
-ALERT: This page is https and has the no-store cache-control directive. It should NOT go in to the page cache.
-ALERT: The page was reloaded on back, not from the page cache. Good job. Running part 2 of the test.
-ALERT: This page is https and has the no-cache cache-control directive. It should NOT go in to the page cache.
-ALERT: The page was reloaded on back, not from the page cache. Good job. Running part 3 of the test.
-ALERT: This page is https and should go in to the page cache.
-ALERT: The page was restored from the page cache. Good job!
+FAIL: Timed out waiting for notifyDone to be called

[Csaba Osztrogonác]

Skipped by http://trac.webkit.org/changeset/113712
Please unskip it with the proper fix.

[Michael Brüning]

Taking a look.

[Michael Brüning]

Seems this is not specific to qt: https://bugs.webkit.org/show_bug.cgi?id=81622

[Michael Brüning]

Created attachment 141744 [details]
Patch

[Michael Brüning]

Created attachment 141754 [details]
Patch

[Simon Hausmann]

Comment on attachment 141754 [details]
Patch

Good catch! I think your patch is correct in principle, the WK1 DRTs do something similar. There's one difference between WK1 and your patch that I think should be corrected: In WK1 (Mac and Qt) only SSL certificates for 127.0.0.1 and localhost are "blindly" accepted.

Also I think the private C++ API can go directly into QQuickWebView, see the "Private C++-only API" comment. This isn't an experimental after all, we know it's private only. I suppose a comment next to the function could indicate that it's used by WTR, and perhaps it should be called setAllowAnyHTTPSCertificateForLocalHost() or so.

[Simon Hausmann]

Change status to assigned to make it clear that Michael is working on this one :)

[Michael Brüning]

Created attachment 141811 [details]
Patch

[Michael Brüning]

Thanks for the review (and the bug state change ;)), Simon. The new version of the patch checks whether the hostname is either localhost or 127.0.0.1 before accepting the certificate. I hope this will also work on the bots (not sure if they are using the localhost/127.0.0.1 config)...

[Simon Hausmann]

Comment on attachment 141811 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141811&action=review

r=me. Only one small nitpick to fix when landing :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:565
> +        && (hostname == QString::fromAscii("127.0.0.1") || hostname == QString::fromAscii("localhost")))

The string comparision should avoid allocating a new QString and instead use QStringLiteral, i.e. if (hostName == QStringLiteral("127.0.0.1"))

[Michael Brüning]

Created attachment 141868 [details]
Patch

[Michael Brüning]

Comment on attachment 141868 [details]
Patch

Thanks for the review, nitpick has bee fixed... 
It seems that webkit-patch uploade cleared the review flag :-/. Could you please r+ again :)

[WebKit Review Bot]

Comment on attachment 141868 [details]
Patch

Rejecting attachment 141868 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit2/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12704341

[Michael Brüning]

Created attachment 141890 [details]
Patch with fixed reviewer in ChangeLog.

Fixed reviewer in ChangeLog to please the commit bot :)

[WebKit Review Bot]

Comment on attachment 141890 [details]
Patch with fixed reviewer in ChangeLog.

Clearing flags on attachment: 141890

Committed r117045: <http://trac.webkit.org/changeset/117045>

[Michael Brüning]

Resolving fixed...