Bug 45523

Summary:

[Qt] Inconsistent behavior on a form submit request...

Product:

WebKit

Reporter:

Dawit A. <adawit>

Component:

Page Loading

Assignee:

Diego Gonzalez <diegohcg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ademar, andy.shaw, benjamin, christian.webkit, cmarcelo, diegohcg, hausmann, kenneth, luiz, robert, tonikitoo, webkit.review.bot

Priority:

Keywords:

Qt, QtTriaged

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

46968

Bug Blocks:

Attachments:

Description	Flags
Patch	hausmann: review-
Patch with test	none
Patch wit NIT changes	none
Patch updated changelog	none
Patch updated v2	none
Example to reproduce the regression problem	none

Dawit A.

Reported 2010-09-09 22:53:31 PDT

This bug opened because portions of an issue reported downstream, https://bugs.kde.org/show_bug.cgi?id=210291, can be clearly duplicated using QtTestBrowser from QtWebKit 2.1. The issue that is QtWebKit specific from that report as follows: 1.) Go to http://www.dpd.at/ 2.) Enter 01635001689282H (non working tracking # from downstream report, comment #6). 3.) Click on the '>' button and see what happens in QtTestBrowser... Expected behavior: A single new window with error message should be shown as verified using both Chromium 6.0.472.53 (0) and Firefox 3.6.9. Actual behavior: Two new windows/tabs are created when using QtWebKit based browsers. One shows "about:blank" and the other shows an error message. Actually with QtTestBrowser the second page is completely blank. What is even worse is the fact that if you close these two new windows and repeat the process again, only the "about:blank" window is created. I guess the question is why does repeating the process result in these two incorrect behaviors ?

Attachments
Patch (2.86 KB, patch) 2011-05-26 14:18 PDT, Diego Gonzalez	hausmann: review-	Details Formatted Diff Diff
Patch with test (3.80 KB, patch) 2011-06-02 13:32 PDT, Diego Gonzalez	no flags	Details Formatted Diff Diff
Patch wit NIT changes (3.77 KB, patch) 2011-06-10 15:10 PDT, Diego Gonzalez	no flags	Details Formatted Diff Diff
Patch updated changelog (4.16 KB, patch) 2011-06-14 13:04 PDT, Diego Gonzalez	no flags	Details Formatted Diff Diff
Patch updated v2 (4.94 KB, patch) 2011-06-14 16:49 PDT, Diego Gonzalez	no flags	Details Formatted Diff Diff
Example to reproduce the regression problem (1.59 KB, application/octet-stream) 2011-11-14 06:54 PST, andy.shaw	no flags	Details
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Luiz Agostini

Comment 1 2011-05-06 10:05:46 PDT

> Actual behavior: > Two new windows/tabs are created when using QtWebKit based browsers. One shows "about:blank" and the other shows an error message. Actually with QtTestBrowser the second page is completely blank. After the MIME sniffing (bug 46968) the second screen now shows the expected content in QtTestBrowser. Other issues continue as they were. > What is even worse is the fact that if you close these two new windows and repeat the process again, only the "about:blank" window is created. I guess the question is why does repeating the process result in these two incorrect behaviors ?

Diego Gonzalez

Comment 2 2011-05-12 13:21:01 PDT

The timer is firing the wrong frame for the new window. Investigating

Diego Gonzalez

Comment 3 2011-05-26 14:16:16 PDT

Pages needs to be set at the same group name, to avoid problems with popup windows and link coloration. Will be used only one page group name, making it similar as others port's are doing

Diego Gonzalez

Comment 4 2011-05-26 14:18:03 PDT

Created attachment 95038 [details] Patch

Antonio Gomes

Comment 5 2011-05-26 23:01:26 PDT

Comment on attachment 95038 [details] Patch no test? :)

Simon Hausmann

Comment 6 2011-05-31 15:22:12 PDT

Comment on attachment 95038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95038&action=review r- because of the string literal. With this change all pages are automatically in the same page group, which to some extend this bug is about. This is a behavioural change that we should really verify with a test. I see that the Gtk+ port seems to behave the same, but I don't see the same behaviour in the other ports directly (as they have an API). What's the pattern their callers use? What's the safest default for us? > Source/WebKit/qt/Api/qwebpage.cpp:159 > +const char* QWebPagePrivate::pageGroupName = "QtWebKit"; As a side-note: This declares a variable that is initialized to a read-only string. The variable itself is writable however, which means it will unnecessarily end up in the data section of unshared memory. > Source/WebKit/qt/Api/qwebpage.cpp:343 > + page->setGroupName(QWebPagePrivate::pageGroupName); I think we should use a string literal here, instead of a variable.

Diego Gonzalez

Comment 7 2011-06-02 13:30:15 PDT

(In reply to comment #6) > (From update of attachment 95038 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95038&action=review > Thanks Simon :) > With this change all pages are automatically in the same page group, which to some extend this bug is about. This is a behavioural change that we should really verify with a test. Test added > I see that the Gtk+ port seems to behave the same, but I don't see the same behaviour in the other ports directly (as they have an API). What's the pattern their callers use? What's the safest default for us? Chromium also uses the same way, but mac and window uses an API for it. > > Source/WebKit/qt/Api/qwebpage.cpp:159 > > +const char* QWebPagePrivate::pageGroupName = "QtWebKit"; > > As a side-note: This declares a variable that is initialized to a read-only string. The variable itself is writable however, which means it will unnecessarily end up in the data section of unshared memory. > > > Source/WebKit/qt/Api/qwebpage.cpp:343 > > + page->setGroupName(QWebPagePrivate::pageGroupName); > > I think we should use a string literal here, instead of a variable. using a string now :)

Diego Gonzalez

Comment 8 2011-06-02 13:32:57 PDT

Created attachment 95798 [details] Patch with test

Kenneth Rohde Christiansen

Comment 9 2011-06-05 09:21:34 PDT

Comment on attachment 95798 [details] Patch with test View in context: https://bugs.webkit.org/attachment.cgi?id=95798&action=review > Source/WebKit/qt/Api/qwebpage.cpp:347 > + // similar as others port's are doing Add dot :-D > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:402 > + QTest::qWait(500); Can we avoid the qWait?

Diego Gonzalez

Comment 10 2011-06-06 07:28:10 PDT

(In reply to comment #9) > (From update of attachment 95798 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95798&action=review > > > Source/WebKit/qt/Api/qwebpage.cpp:347 > > + // similar as others port's are doing > > Add dot :-D > > > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:402 > > + QTest::qWait(500); > > Can we avoid the qWait? Hummmm, is necessary a little time to the form be updated :(

Diego Gonzalez

Comment 11 2011-06-10 15:10:00 PDT

Created attachment 96797 [details] Patch wit NIT changes

Antonio Gomes

Comment 12 2011-06-10 21:43:25 PDT

Comment on attachment 96797 [details] Patch wit NIT changes Just curious, what is the inconsistency the patch is fixing? :)

Diego Gonzalez

Comment 13 2011-06-13 06:03:51 PDT

(In reply to comment #12) > (From update of attachment 96797 [details]) > Just curious, what is the inconsistency the patch is fixing? :) Following the steps reported, the inconsistency is when a occurs a form submission requesting a new window, two windows are opened (instead of only) and the first window is opened with a blank page.

Antonio Gomes

Comment 14 2011-06-13 06:31:53 PDT

(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 96797 [details] [details]) > > Just curious, what is the inconsistency the patch is fixing? :) > > Following the steps reported, the inconsistency is when a occurs a form submission requesting a new window, two windows are opened (instead of only) and the first window is opened with a blank page. Nice! I'd have added it to the ChangeLog.

Robert Hogan

Comment 15 2011-06-13 07:33:30 PDT

DRT also sets a group name for pages. Maybe that can be removed now?

Diego Gonzalez

Comment 16 2011-06-14 13:02:08 PDT

(In reply to comment #15) > DRT also sets a group name for pages. Maybe that can be removed now? Hummm maybe but I don't knoew if it is really necessary, because when DRT sets the group name it's overlap the group name already set.

Diego Gonzalez

Comment 17 2011-06-14 13:04:11 PDT

Created attachment 97159 [details] Patch updated changelog

Kenneth Rohde Christiansen

Comment 18 2011-06-14 15:23:39 PDT

Comment on attachment 97159 [details] Patch updated changelog View in context: https://bugs.webkit.org/attachment.cgi?id=97159&action=review > Source/WebKit/qt/Api/qwebpage.cpp:350 > + // Pages needs to be set at the same group name, to avoid problems with popup > + // windows and link coloration. Will be used only one page group name, making > + // it is similar as others port's are doing. This needs better explanation. Let me help you out here. // My default each page is put into their own unique page group, which affects popup windows and visited links. // Page groups (per process only) is a feature making it possible to use separate settings for each group, so that // for instance an integrated browser/email reader can use different settings for displaying HTML pages and HTML email. // To make QtWebKit work as expected out of the box, we use a default group similar to what other ports are doing. Someone might be able to make this even more clear and short :-) > Source/WebKit/qt/Api/qwebpage.cpp:351 > + page->setGroupName("org.webkit.qt.QtWebKit"); Given the above description I am not sure why such a complicated name is needed. Why not just "Default Group" or so.

Diego Gonzalez

Comment 19 2011-06-14 16:49:14 PDT

Created attachment 97194 [details] Patch updated v2

WebKit Review Bot

Comment 20 2011-06-15 08:07:17 PDT

The commit-queue encountered the following flaky tests while processing attachment 97194 [details]: inspector/inspected-objects-not-overriden.html bug 62729 (authors: pfeldman@chromium.org and yurys@chromium.org) The commit-queue is continuing to process your patch.

WebKit Review Bot

Comment 21 2011-06-15 08:08:38 PDT

Comment on attachment 97194 [details] Patch updated v2 Clearing flags on attachment: 97194 Committed r88933: <http://trac.webkit.org/changeset/88933>

WebKit Review Bot

Comment 22 2011-06-15 08:08:44 PDT

All reviewed patches have been landed. Closing bug.

Ademar Reis

Comment 23 2011-06-20 06:32:46 PDT

Revision r88933 cherry-picked into qtwebkit-2.2 with commit a2ce773 <http://gitorious.org/webkit/qtwebkit/commit/a2ce773>

andy.shaw

Comment 24 2011-11-14 06:52:16 PST

Unfortunately the fix for this caused a problem in a related area with dismissing javascript alert windows that are shown from a hidden QWebView as it will crash on the 2nd dismissal. This was working in the Webkit version with Qt 4.7.x but broken in Qt 4.8.0. Reverting the change in the patch above was confirmed to solve the problem. A complete testcase for Qt is attached.

andy.shaw

Comment 25 2011-11-14 06:54:40 PST

Created attachment 114938 [details] Example to reproduce the regression problem

Note You need to log in before you can comment on or make changes to this bug.