Bug 83576 - [Qt][WK2] http/tests/navigation/https-in-page-cache.html fails with timeout
Summary: [Qt][WK2] http/tests/navigation/https-in-page-cache.html fails with timeout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Critical
Assignee: Michael Brüning
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 79668
  Show dependency treegraph
 
Reported: 2012-04-10 06:45 PDT by Csaba Osztrogonác
Modified: 2012-05-15 08:08 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.04 KB, patch)
2012-05-14 09:52 PDT, Michael Brüning
no flags Details | Formatted Diff | Diff
Patch (8.20 KB, patch)
2012-05-14 10:39 PDT, Michael Brüning
no flags Details | Formatted Diff | Diff
Patch (7.43 KB, patch)
2012-05-14 16:49 PDT, Michael Brüning
no flags Details | Formatted Diff | Diff
Patch (7.42 KB, patch)
2012-05-15 00:02 PDT, Michael Brüning
hausmann: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch with fixed reviewer in ChangeLog. (7.74 KB, patch)
2012-05-15 01:33 PDT, Michael Brüning
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2012-04-10 06:45:10 PDT
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
Comment 1 Csaba Osztrogonác 2012-04-10 06:53:52 PDT
Skipped by http://trac.webkit.org/changeset/113712
Please unskip it with the proper fix.
Comment 2 Michael Brüning 2012-05-14 01:24:23 PDT
Taking a look.
Comment 3 Michael Brüning 2012-05-14 03:45:34 PDT
Seems this is not specific to qt: https://bugs.webkit.org/show_bug.cgi?id=81622
Comment 4 Michael Brüning 2012-05-14 09:52:27 PDT
Created attachment 141744 [details]
Patch
Comment 5 Michael Brüning 2012-05-14 10:39:36 PDT
Created attachment 141754 [details]
Patch
Comment 6 Simon Hausmann 2012-05-14 12:39:15 PDT
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.
Comment 7 Simon Hausmann 2012-05-14 12:40:29 PDT
Change status to assigned to make it clear that Michael is working on this one :)
Comment 8 Michael Brüning 2012-05-14 16:49:00 PDT
Created attachment 141811 [details]
Patch
Comment 9 Michael Brüning 2012-05-14 16:51:34 PDT
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)...
Comment 10 Simon Hausmann 2012-05-14 22:58:12 PDT
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"))
Comment 11 Michael Brüning 2012-05-15 00:02:38 PDT
Created attachment 141868 [details]
Patch
Comment 12 Michael Brüning 2012-05-15 00:05:35 PDT
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 :)
Comment 13 WebKit Review Bot 2012-05-15 01:05:31 PDT
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
Comment 14 Michael Brüning 2012-05-15 01:33:07 PDT
Created attachment 141890 [details]
Patch with fixed reviewer in ChangeLog.

Fixed reviewer in ChangeLog to please the commit bot :)
Comment 15 WebKit Review Bot 2012-05-15 02:38:27 PDT
Comment on attachment 141890 [details]
Patch with fixed reviewer in ChangeLog.

Clearing flags on attachment: 141890

Committed r117045: <http://trac.webkit.org/changeset/117045>
Comment 16 Michael Brüning 2012-05-15 08:08:25 PDT
Resolving fixed...