Bug 63582 – [Qt] REGRESSION(r87797): Broke KDEWebKit's custom QNAM.

• Component: Page Loading
• Status: RESOLVED FIXED

[Dawit A.]

The fix applied to workaround the QNAM limitation in bug 49650 breaks custom QNAM implementations, namely kdewebkit's, that rely on the unsupportedContent signal being invoked as soon as possible instead of being queued. 

kdewebkit requires this functionality in order to take advantage of KIO's put-slave-on-hold functionality which allows two different applications to share the same network resource. That way a single resource will not be downloaded multiple times: once when the user clicks on the link and a second time when the appropriate application attempts to display the resource.

[Dawit A.]

Created attachment 98998 [details]
proposed patchgit diff

The proposed patch uses Qt's Meta Object system, namely a dynamic property, to address this issue. If the dynamic property "SKIP_QTBUG_18718_WORKAROUND" is set, the unsupportedContent signal will be invoked directly. Otherwise, the signal connection will be queued until control returns to the event loop.

[WebKit Review Bot]

Attachment 98998 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/WebCoreSupport/FrameLoade..." exit_code: 1

Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1699:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dawit A.]

Created attachment 99001 [details]
proporsed patch v2

[Andreas Kling]

Comment on attachment 99001 [details]
proporsed patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=99001&action=review

> Source/WebKit/qt/ChangeLog:5
> +        [Qt] Regression: Fix for 49650 breaks custom QNAM implementations...

Please end your sentences with one period, not three.

> Source/WebKit/qt/ChangeLog:8
> +        This is an improvement of the regression patch committed to address bug 49650.

This adds no information.

> Source/WebKit/qt/ChangeLog:9
> +        It allows for immediate emition of the QWebPage::unsupportedContent signal

emition -> emission

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:215
> +    qRegisterMetaType<QNetworkReply*>("QNetworkRepyPtr");

Typo, should be "QNetworkReplyPtr".
Is this the right place to register meta types?
Note that this code may execute more than once.

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:242
> +    // NOTE: This should be a signal-to-signal connection were it not for bugs.

should -> would

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1699
> +    if (page->property("SKIP_QTBUG_18718_WORKAROUND").toBool()

I don't like the name of this property. Can we make it something that describes what it does instead?
_q_emitUnsupportedContentDirectly perhaps?

[Dawit A.]

(In reply to comment #4)
> > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:215
> > +    qRegisterMetaType<QNetworkReply*>("QNetworkRepyPtr");
> 
> Typo, should be "QNetworkReplyPtr".
> Is this the right place to register meta types?
> Note that this code may execute more than once.

I do not think you need to register the meta-type each time you emit the queued signal. At least that is how I have been using that macro all these years, but I will check it again.

[Dawit A.]

Created attachment 99110 [details]
proposed patch v3

[Kenneth Rohde Christiansen]

Comment on attachment 99110 [details]
proposed patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=99110&action=review

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1698
> +    // A dynamic Qt property, _q_emitUnsupportedContentDirectly, is used here to prevent breakage
> +    // of custom QNAM implementations that assume this signal would not be queued, e.g kdewebkit.

Why not modify kdewebkit?

[Dawit A.]

(In reply to comment #7)
> (From update of attachment 99110 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99110&action=review
> 
> > Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1698
> > +    // A dynamic Qt property, _q_emitUnsupportedContentDirectly, is used here to prevent breakage
> > +    // of custom QNAM implementations that assume this signal would not be queued, e.g kdewebkit.
> 
> Why not modify kdewebkit?

How ? kdewebkit needs the notification as soon as possible and the workaround completely prevents that from happening. The fact that the notification is needed immediately is not a simple choice in kdewebkit. It is a requirement enforced on it by the design of KIO.

The way KIO 's put-slave-on-hold feature is designed, it requires the client to put the ioslave on hold as soon as it informs it of the mime-type if the client cannot handle that content-type. Otherwise, the "put-slave-on-hold" functionality will not work and the same resource would end up being downloaded at least twice: once for the original the request and the second when the application is launched to handle the request attempts to retrieve it. That won't happen if the slave is put on hold because the request the lanuched application make would be assigned to use the already parked (put-on-hold) ioslave. 

Anyhow, if the unsupported content signal is queued, the slot that handles this won't receive the notification on time to pause the ioslave. The ioslave would already been in the process of downloading the request. That not only causes the issue mentioned above, duplicate download of same resource, but since the call to publish the ioslave for reuse is a sync call, it causes a freeze in the application until the ioslave can interrupt itself from downloading the content to respond to our request.

[Ademar Reis]

Any news regarding this problem? Anyone planning to look at it?

[Andreas Kling]

(In reply to comment #9)
> Any news regarding this problem? Anyone planning to look at it?

I think we should roll out the original workaround making the emission queued, as that can be worked around in user code much more easily.

[Dawit A.]

(In reply to comment #10)
> (In reply to comment #9)
> > Any news regarding this problem? Anyone planning to look at it?
> 
> I think we should roll out the original workaround making the emission queued, as that can be worked around in user code much more easily.

Yes, please. Developers connecting to this signal can use Qt::QueuedConnection, if they need to use anything that creates its own local event loop.

[Andreas Kling]

Comment on attachment 99110 [details]
proposed patch v3

Let's roll out the previous workaround instead.

[Ademar Reis]

(In reply to comment #12)
> (From update of attachment 99110 [details])
> Let's roll out the previous workaround instead.

How do we go from here? Should we reopen the original bug or open a new one and attach the patch with the rollout?

[Dawit A.]

Created attachment 102471 [details]
proposed patch

Here is a patch that rolls back out the source of this regression.

[Alexis Menard (darktears)]

(In reply to comment #14)
> Created an attachment (id=102471) [details]
> proposed patch
> 
> Here is a patch that rolls back out the source of this regression.

LGTM. Though the original bug should be re-open if there was any.

[Ademar Reis]

(In reply to comment #15)
> (In reply to comment #14)
> > Created an attachment (id=102471) [details] [details]
> > proposed patch
> > 
> > Here is a patch that rolls back out the source of this regression.
> 
> LGTM. Though the original bug should be re-open if there was any.

Yep, that's bug 49650 (which is also a regression itself). I guess it should be reopen once we land this patch (assuming we still have the problem).

[Andreas Kling]

Comment on attachment 102471 [details]
proposed patch

r=me indeed.
Please re-open the original bug when landing, as mentioned.

[Ademar Reis]

(In reply to comment #17)
> (From update of attachment 102471 [details])
> r=me indeed.
> Please re-open the original bug when landing, as mentioned.

Dawit, anything else to change in the patch? You have to cq+ it so that it lands in the repository (or you can cq? if you're not a commiter).

I would like to include it on this 2.2 weekly release so that it gets proper test exposure.

[WebKit Review Bot]

Comment on attachment 102471 [details]
proposed patch

Clearing flags on attachment: 102471

Committed r92479: <http://trac.webkit.org/changeset/92479>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r92479 cherry-picked into qtwebkit-2.2 with commit 18d52e6 <http://gitorious.org/webkit/qtwebkit/commit/18d52e6>