Bug 45523 – [Qt] Inconsistent behavior on a form submit request...

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Dawit A.]

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 ?

[Luiz Agostini]

> 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]

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

[Diego Gonzalez]

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]

Created attachment 95038 [details]
Patch

[Antonio Gomes]

Comment on attachment 95038 [details]
Patch

no test? :)

[Simon Hausmann]

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]

(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]

Created attachment 95798 [details]
Patch with test

[Kenneth Rohde Christiansen]

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]

(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]

Created attachment 96797 [details]
Patch wit NIT changes

[Antonio Gomes]

Comment on attachment 96797 [details]
Patch wit NIT changes

Just curious, what is the inconsistency the patch is fixing? :)

[Diego Gonzalez]

(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]

(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]

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

[Diego Gonzalez]

(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]

Created attachment 97159 [details]
Patch updated changelog

[Kenneth Rohde Christiansen]

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]

Created attachment 97194 [details]
Patch updated v2

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 97194 [details]:

inspector/inspected-objects-not-overriden.html bug 62729 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 97194 [details]
Patch updated v2

Clearing flags on attachment: 97194

Committed r88933: <http://trac.webkit.org/changeset/88933>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

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

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.

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