Bug 72329 – [Qt] Uploading images to Google+ using QtWebKit does not work

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Dawit A.]

As the summary states, you won't be able to upload images to your Google+ account using QtTestBrowser from QtWebKit 2.2 branch. The reason seems to be the QIODevice parameter that points to the POST data in QNetworkAccesssManager::createRequest, is simply invalid and accessing it causes a crash if you do not first check for the "QNetworkRequest::Content-Length" attribute.

[Dawit A.]

The cause of this bug turns out to be the lack of support for BlobData in QtNetworkReplyHandler. Actually, the fact that QtNetworkReplyHandler does not even check for BlobData when form upload data type is actually a bug since it would be attempting to access a non-existent iodevice. In any case, I am not sure how to implement support for this functionality, but the lack of such support means, no image upload support in Google+ and worst yet a crash if you attempt to read from the POST iodevice in QNetworkAccessManager::createRequest.

[Zeno Albisser]

*** Bug 68581 has been marked as a duplicate of this bug. ***

[Allan Sandfeld Jensen]

We have enabled BLOBs but are not handling them at all in network request. What is worse is that we treat the formdata types as if they were a binary choice of either Data or File.

[Allan Sandfeld Jensen]

Created attachment 167541 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 167544 [details]
Patch

Handle expected modification time.

[Jocelyn Turcotte]

Comment on attachment 167544 [details]
Patch

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

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:81
> +            qint64 fileEnd = fi.size();
> +#if ENABLE(BLOB)
> +            if (element.m_fileLength != BlobDataItem::toEndOfFile)
> +                fileEnd = qMin<qint64>(fi.size(), element.m_fileStart + element.m_fileLength);
> +            m_fileSize += qMax<qint64>(0, fileEnd - element.m_fileStart);
> +#else
> +            m_fileSize += fileEnd;
> +#endif

I think this would be slightly easier to follow if you keep the fileEnd declaration inside the #if ENABLE(BLOB) and keep using fi.size() directly outside.

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:110
> +    default:
> +        ASSERT_NOT_REACHED();

This could be worth a comment stating that you expect encodedBlob types to have been converted already.

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:169
> +            else if (element.m_fileLength != BlobDataItem::toEndOfFile && m_currentDelta == element.m_fileLength)
> +                moveToNextElement();

If I'm following all this, m_fileLength is provided by JS and might be larger than the actual file, in that case is there any protection to prevent looping forever or cutting the end of the last elements?

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:647
> +#if ENABLE(BLOB)
> +    // Check if there is a blob in the form data.
> +    bool hasBlob = false;
> +

It would be nice to put all the blob logic added to getIODevice in a function named something like convertBlobFormData instead.

> LayoutTests/platform/qt/TestExpectations:587
>  http/tests/local/blob/send-hybrid-blob.html

Are any of those other skipped blob tests easy to cover?

[Allan Sandfeld Jensen]

(In reply to comment #6)
> > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:169
> > +            else if (element.m_fileLength != BlobDataItem::toEndOfFile && m_currentDelta == element.m_fileLength)
> > +                moveToNextElement();
> 
> If I'm following all this, m_fileLength is provided by JS and might be larger than the actual file, in that case is there any protection to prevent looping forever or cutting the end of the last elements?
> 
The condition just prior to this one checks for endOfFile, this is only to terminate if we are before end-of-file but at the end of the file-length specifier.

> > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:647
> > +#if ENABLE(BLOB)
> > +    // Check if there is a blob in the form data.
> > +    bool hasBlob = false;
> > +
> 
> It would be nice to put all the blob logic added to getIODevice in a function named something like convertBlobFormData instead.
> 
Right.

> > LayoutTests/platform/qt/TestExpectations:587
> >  http/tests/local/blob/send-hybrid-blob.html
> 
> Are any of those other skipped blob tests easy to cover?

The rest of the blobdata tests all depend on either DragAndDrop logic which we don't support in layout tests, or the FILE_SYSTEM api, which don't have enabled (and which would require additional support in QNetworkRequestHandler).

[Allan Sandfeld Jensen]

Created attachment 167719 [details]
Patch

[Allan Sandfeld Jensen]

Landed in r130746, but webkit-path land crashed before closing bug.

[Csaba Osztrogonác]

(In reply to comment #9)
> Landed in r130746, but webkit-path land crashed before closing bug.

Reopen, because it made 45 tests crash - http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r130746%20%2843587%29/results.html

[Allan Sandfeld Jensen]

Created attachment 167742 [details]
Patch

[Allan Sandfeld Jensen]

Committed r130753: <http://trac.webkit.org/changeset/130753>