Bug 45523 - [Qt] Inconsistent behavior on a form submit request...
Summary: [Qt] Inconsistent behavior on a form submit request...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Diego Gonzalez
URL:
Keywords: Qt, QtTriaged
Depends on: 46968
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-09 22:53 PDT by Dawit A.
Modified: 2011-11-14 06:54 PST (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Dawit A. 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 ?
Comment 1 Luiz Agostini 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 ?
Comment 2 Diego Gonzalez 2011-05-12 13:21:01 PDT
The timer is firing the wrong frame for the new window. Investigating
Comment 3 Diego Gonzalez 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
Comment 4 Diego Gonzalez 2011-05-26 14:18:03 PDT
Created attachment 95038 [details]
Patch
Comment 5 Antonio Gomes 2011-05-26 23:01:26 PDT
Comment on attachment 95038 [details]
Patch

no test? :)
Comment 6 Simon Hausmann 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.
Comment 7 Diego Gonzalez 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 :)
Comment 8 Diego Gonzalez 2011-06-02 13:32:57 PDT
Created attachment 95798 [details]
Patch with test
Comment 9 Kenneth Rohde Christiansen 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?
Comment 10 Diego Gonzalez 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 :(
Comment 11 Diego Gonzalez 2011-06-10 15:10:00 PDT
Created attachment 96797 [details]
Patch wit NIT changes
Comment 12 Antonio Gomes 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? :)
Comment 13 Diego Gonzalez 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.
Comment 14 Antonio Gomes 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.
Comment 15 Robert Hogan 2011-06-13 07:33:30 PDT
DRT also sets a group name for pages. Maybe that can be removed now?
Comment 16 Diego Gonzalez 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.
Comment 17 Diego Gonzalez 2011-06-14 13:04:11 PDT
Created attachment 97159 [details]
Patch updated changelog
Comment 18 Kenneth Rohde Christiansen 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.
Comment 19 Diego Gonzalez 2011-06-14 16:49:14 PDT
Created attachment 97194 [details]
Patch updated v2
Comment 20 WebKit Review Bot 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.
Comment 21 WebKit Review Bot 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>
Comment 22 WebKit Review Bot 2011-06-15 08:08:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Ademar Reis 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>
Comment 24 andy.shaw 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.
Comment 25 andy.shaw 2011-11-14 06:54:40 PST
Created attachment 114938 [details]
Example to reproduce the regression problem