Bug 77007 - [Qt][WK2] Use QVariant for payload data in application URL schemes.
Summary: [Qt][WK2] Use QVariant for payload data in application URL schemes.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zeno Albisser
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-25 06:52 PST by Zeno Albisser
Modified: 2012-01-31 04:37 PST (History)
3 users (show)

See Also:


Attachments
patch for review. (19.86 KB, patch)
2012-01-25 07:27 PST, Zeno Albisser
hausmann: review-
Details | Formatted Diff | Diff
patch for review. - updated as commented previously. (21.81 KB, patch)
2012-01-26 03:13 PST, Zeno Albisser
no flags Details | Formatted Diff | Diff
patch for review. - refactorings as commented before. (6.54 KB, patch)
2012-01-27 09:22 PST, Zeno Albisser
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zeno Albisser 2012-01-25 06:52:18 PST
Application schemes should use QVariant as datatype for payload data. This way data with different types can be passed.
A normal JS string / QString can be passed as well as a QByteArray for binary data.

We favor this solution over a http redirection based solution, because this solution is less intrusive and less prone for security issues.
Therefore we should also remove the http headers exposed in QQuickNetworkReply.
The only necessary fields are contentType and data.
Comment 1 Zeno Albisser 2012-01-25 07:27:54 PST
Created attachment 123935 [details]
patch for review.
Comment 2 WebKit Review Bot 2012-01-25 07:30:21 PST
Attachment 123935 [details] did not pass style-queue:

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

Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.h:40:  m_iso8859_1 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Simon Hausmann 2012-01-25 08:11:29 PST
Comment on attachment 123935 [details]
patch for review.

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

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:59
> +    return QVariant(QByteArray::fromRawData(static_cast<const char*>(m_sharedMemory->data()), m_dataLength));
>  }
>  
> -void QQuickNetworkReply::setData(const QString& data)
> +void QQuickNetworkReply::setData(const QVariant& data)
>  {
>      // This function can be called several times. In this case the previously allocated SharedMemory

Just a quick note before I look deeper into this:

I think is is going into the right direction. However I notice that now setData and data are not symmetric anymore, i.e. what you pass to setData is not what you're going to get back from data() if what you pass in is a QString.

I suggest to make setData/data symmetric, i.e. store the original QVariant and only convert it to the final "byte array" when _sending_ the reply across the "CoreIPC wire".
Comment 4 Simon Hausmann 2012-01-26 00:15:55 PST
Comment on attachment 123935 [details]
patch for review.

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

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:54
> +    return QVariant(QByteArray::fromRawData(static_cast<const char*>(m_sharedMemory->data()), m_dataLength));

This is unfortunately also not a safe operation as it requires the caller to keep the network reply alive while keeping a reference to the bytearray.

> Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.cpp:25
> +    , m_iso8859_1(QByteArray("<html><head><title>this is iso8859-1 content</title></head><body>content</body></html>"))
> +    , m_utf8(QByteArray(QString("<html><head><title>this is utf-8 content</title></head><body>content</body></html>").toUtf8()))

How about including a character in the test strings that is not part of ASCII but ISO-8859-1 such as

    251   169   A9     ©     COPYRIGHT SIGN

Then you could do:

QString text = QStringLiteral("<html><head><title>title with copyright %1");
text = text.arg(QChar::fromLatin1(169));

and then

latin1Data = text.toLatin1();
utf8Data = text.toUtf8();

Q_ASSERT(latin1Data != utf8Data);

> Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.h:32
> +Q_OBJECT
> +Q_PROPERTY(QVariant iso88591data READ iso88591data)
> +Q_PROPERTY(QVariant utf8data READ utf8data)

Missing indentation.

> Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.h:37
> +    QVariant iso88591data();
> +    QVariant utf8data();

Missing const.
Comment 5 Zeno Albisser 2012-01-26 03:11:14 PST
Comment on attachment 123935 [details]
patch for review.

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

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:54
>> +    return QVariant(QByteArray::fromRawData(static_cast<const char*>(m_sharedMemory->data()), m_dataLength));
> 
> This is unfortunately also not a safe operation as it requires the caller to keep the network reply alive while keeping a reference to the bytearray.

Indeed - There is a detach() missing. But it's going away with the change for your next comment anyway. :)

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:59
>>      // This function can be called several times. In this case the previously allocated SharedMemory
> 
> Just a quick note before I look deeper into this:
> 
> I think is is going into the right direction. However I notice that now setData and data are not symmetric anymore, i.e. what you pass to setData is not what you're going to get back from data() if what you pass in is a QString.
> 
> I suggest to make setData/data symmetric, i.e. store the original QVariant and only convert it to the final "byte array" when _sending_ the reply across the "CoreIPC wire".

The idea was to copy as little as possible. But yes, with the change to QVariant I have to change this as well.

>> Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.cpp:20
>> +#include "bytearraytestdata.h"
> 
> Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

I am ignoring that one, because we do not want to include config.h in a qml test.

>> Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.cpp:25
>> +    , m_utf8(QByteArray(QString("<html><head><title>this is utf-8 content</title></head><body>content</body></html>").toUtf8()))
> 
> How about including a character in the test strings that is not part of ASCII but ISO-8859-1 such as
> 
>     251   169   A9     ©     COPYRIGHT SIGN
> 
> Then you could do:
> 
> QString text = QStringLiteral("<html><head><title>title with copyright %1");
> text = text.arg(QChar::fromLatin1(169));
> 
> and then
> 
> latin1Data = text.toLatin1();
> utf8Data = text.toUtf8();
> 
> Q_ASSERT(latin1Data != utf8Data);

I guess that's a good idea, to make sure that the test data actually has the proper encoding.

>> Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.h:32
>> +Q_PROPERTY(QVariant utf8data READ utf8data)
> 
> Missing indentation.

fixed.

>> Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.h:37
>> +    QVariant utf8data();
> 
> Missing const.

fixed.

>> Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.h:40
>> +    QByteArray m_iso8859_1;
> 
> m_iso8859_1 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

I renamed that one to m_latin1Data.
Comment 6 Zeno Albisser 2012-01-26 03:13:51 PST
Created attachment 124091 [details]
patch for review. - updated as commented previously.
Comment 7 WebKit Review Bot 2012-01-26 03:15:22 PST
Attachment 124091 [details] did not pass style-queue:

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

Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.cpp:20:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Simon Hausmann 2012-01-26 03:38:02 PST
Comment on attachment 124091 [details]
patch for review. - updated as commented previously.

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

r=me. One comment below and one more here, they can be ignored or addressed in later changes ;)

One aspect I'm not too happy about is that the _uiprocess_ decides to store input _strings_ as-is (utf-16) encoded and the _web process_ side - in the QtNetworkReply constructor - makes the assumption that the encoding of the provided data is utf-16 unless otherwise specified. I think it would be much cleaner if this could all be handled on the ui process side without assumptions across process boundaries.

That means that ::send() should look like this:

if (data.type() == QVariant::String) {
    contentType = "text/html; charset=utf-16";
    QString string = data.toString();
    memcpy(blah, string.constData(), ...);
} else if (data.canConvert<QByteArray>()) {
    // copy bytes
}

if (contentType.isEmpty()) {
   abort and print a warning or something
}

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:91
> +        QObject* schemeParent = parent()->parent();
> +        if (schemeParent) {
> +            QQuickWebViewExperimental* webViewExperimental = qobject_cast<QQuickWebViewExperimental*>(schemeParent->parent());

parent()->parent()->parent() is quite a hack. I think it would be nicer if QQuickNetworkReply kept a QWeakPointer to the QQuickWebView and created the QQuickWebViewExperimental on the fly for sending.
Comment 9 Zeno Albisser 2012-01-26 04:28:16 PST
Comment on attachment 124091 [details]
patch for review. - updated as commented previously.

Clearing flags on attachment: 124091

Committed r105991: <http://trac.webkit.org/changeset/105991>
Comment 10 Zeno Albisser 2012-01-26 04:28:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Zeno Albisser 2012-01-27 09:21:26 PST
Thank you very much for reviewing. :)

(In reply to comment #8)
> (From update of attachment 124091 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124091&action=review
> 
> r=me. One comment below and one more here, they can be ignored or addressed in later changes ;)
> 
> One aspect I'm not too happy about is that the _uiprocess_ decides to store input _strings_ as-is (utf-16) encoded and the _web process_ side - in the QtNetworkReply constructor - makes the assumption that the encoding of the provided data is utf-16 unless otherwise specified. I think it would be much cleaner if this could all be handled on the ui process side without assumptions across process boundaries.
> 
> That means that ::send() should look like this:
> 
> if (data.type() == QVariant::String) {
>     contentType = "text/html; charset=utf-16";
>     QString string = data.toString();
>     memcpy(blah, string.constData(), ...);
> } else if (data.canConvert<QByteArray>()) {
>     // copy bytes
> }
> 
> if (contentType.isEmpty()) {
>    abort and print a warning or something
> }
> 
> > Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:91
> > +        QObject* schemeParent = parent()->parent();
> > +        if (schemeParent) {
> > +            QQuickWebViewExperimental* webViewExperimental = qobject_cast<QQuickWebViewExperimental*>(schemeParent->parent());
> 
> parent()->parent()->parent() is quite a hack. I think it would be nicer if QQuickNetworkReply kept a QWeakPointer to the QQuickWebView and created the QQuickWebViewExperimental on the fly for sending.

I will propose a patch to address these issues.
Unfortunately I cannot just pass a pointer to the WebView in the constructor, because the QQuickNetworkReply is created in the constructor of the QQuickUrlSchemeDelegate and that one is created in QML. So at the time of instantiation it does not even have a parent. So calling a setter when the UrlScheme is added to the ListProperty is the best i could come up with.
Comment 12 Zeno Albisser 2012-01-27 09:22:41 PST
Created attachment 124328 [details]
patch for review. - refactorings as commented before.
Comment 13 Eric Seidel (no email) 2012-01-30 16:03:23 PST
Comment on attachment 124328 [details]
patch for review. - refactorings as commented before.

Cleared review? from attachment 124328 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment 14 Simon Hausmann 2012-01-31 03:38:13 PST
Comment on attachment 124328 [details]
patch for review. - refactorings as commented before.

Perhaps you should open a new bug report and attach this patch with r? set :)
Comment 15 Simon Hausmann 2012-01-31 03:40:33 PST
Comment on attachment 124328 [details]
patch for review. - refactorings as commented before.

Patch looks good to me otherwise :)
Comment 16 Zeno Albisser 2012-01-31 04:37:56 PST
(In reply to comment #15)
> (From update of attachment 124328 [details])
> Patch looks good to me otherwise :)

https://bugs.webkit.org/show_bug.cgi?id=77417