Full Text Bug Listing

Zeno Albisser

Reported 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.

Zeno Albisser

Comment 1 2012-01-25 07:27:54 PST

Created attachment 123935 [details] patch for review.

WebKit Review Bot

Comment 2 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.

Simon Hausmann

Comment 3 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".

Simon Hausmann

Comment 4 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.

Zeno Albisser

Comment 5 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.

Zeno Albisser

Comment 6 2012-01-26 03:13:51 PST

Created attachment 124091 [details] patch for review. - updated as commented previously.

WebKit Review Bot

Comment 7 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.

Simon Hausmann

Comment 8 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.

Zeno Albisser

Comment 9 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>

Zeno Albisser

Comment 10 2012-01-26 04:28:27 PST

All reviewed patches have been landed. Closing bug.

Zeno Albisser

Comment 11 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.

Zeno Albisser

Comment 12 2012-01-27 09:22:41 PST

Created attachment 124328 [details] patch for review. - refactorings as commented before.

Eric Seidel (no email)

Comment 13 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).

Simon Hausmann

Comment 14 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 :)

Simon Hausmann

Comment 15 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 :)

Zeno Albisser

Comment 16 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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Bug 77007