Bug 83576

Summary: [Qt][WK2] http/tests/navigation/https-in-page-cache.html fails with timeout
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: Michael Brüning <michael.bruning>
Status: RESOLVED FIXED    
Severity: Critical CC: hausmann, menard, michael.bruning, ossy, webkit.review.bot, zoltan
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 79668    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
hausmann: review+, webkit.review.bot: commit-queue-
Patch with fixed reviewer in ChangeLog. none

Csaba Osztrogonác
Reported 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
Attachments
Patch (8.04 KB, patch)
2012-05-14 09:52 PDT, Michael Brüning 		no flags
DetailsFormatted DiffDiff
Patch (8.20 KB, patch)
2012-05-14 10:39 PDT, Michael Brüning 		no flags
DetailsFormatted DiffDiff
Patch (7.43 KB, patch)
2012-05-14 16:49 PDT, Michael Brüning 		no flags
DetailsFormatted DiffDiff
Patch (7.42 KB, patch)
2012-05-15 00:02 PDT, Michael Brüning 		hausmann: review+
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Patch with fixed reviewer in ChangeLog. (7.74 KB, patch)
2012-05-15 01:33 PDT, Michael Brüning 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Csaba Osztrogonác
Comment 1 2012-04-10 06:53:52 PDT
Skipped by http://trac.webkit.org/changeset/113712 Please unskip it with the proper fix.
Michael Brüning
Comment 2 2012-05-14 01:24:23 PDT
Taking a look.
Michael Brüning
Comment 3 2012-05-14 03:45:34 PDT
Seems this is not specific to qt: https://bugs.webkit.org/show_bug.cgi?id=81622
Michael Brüning
Comment 4 2012-05-14 09:52:27 PDT
Created attachment 141744 [details] Patch
Michael Brüning
Comment 5 2012-05-14 10:39:36 PDT
Created attachment 141754 [details] Patch
Simon Hausmann
Comment 6 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.
Simon Hausmann
Comment 7 2012-05-14 12:40:29 PDT
Change status to assigned to make it clear that Michael is working on this one :)
Michael Brüning
Comment 8 2012-05-14 16:49:00 PDT
Created attachment 141811 [details] Patch
Michael Brüning
Comment 9 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)...
Simon Hausmann
Comment 10 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"))
Michael Brüning
Comment 11 2012-05-15 00:02:38 PDT
Created attachment 141868 [details] Patch
Michael Brüning
Comment 12 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 :)
WebKit Review Bot
Comment 13 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
Michael Brüning
Comment 14 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 :)
WebKit Review Bot
Comment 15 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>
Michael Brüning
Comment 16 2012-05-15 08:08:25 PDT
Resolving fixed...
Note You need to log in before you can comment on or make changes to this bug.