Bug 72329 - [Qt] Uploading images to Google+ using QtWebKit does not work
Summary: [Qt] Uploading images to Google+ using QtWebKit does not work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Major
Assignee: Allan Sandfeld Jensen
URL:
Keywords: Qt
: 68581 (view as bug list)
Depends on: 98749
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-14 16:50 PST by Dawit A.
Modified: 2012-10-15 03:51 PDT (History)
4 users (show)

See Also:


Attachments
Patch (11.29 KB, patch)
2012-10-08 08:21 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (11.57 KB, patch)
2012-10-08 09:07 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (12.57 KB, patch)
2012-10-09 02:52 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (11.92 KB, patch)
2012-10-09 05:36 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dawit A. 2011-11-14 16:50:07 PST
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.
Comment 1 Dawit A. 2011-11-25 06:49:28 PST
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.
Comment 2 Zeno Albisser 2011-11-28 04:26:07 PST
*** Bug 68581 has been marked as a duplicate of this bug. ***
Comment 3 Allan Sandfeld Jensen 2012-10-08 02:48:17 PDT
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.
Comment 4 Allan Sandfeld Jensen 2012-10-08 08:21:13 PDT
Created attachment 167541 [details]
Patch
Comment 5 Allan Sandfeld Jensen 2012-10-08 09:07:07 PDT
Created attachment 167544 [details]
Patch

Handle expected modification time.
Comment 6 Jocelyn Turcotte 2012-10-08 10:51:49 PDT
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?
Comment 7 Allan Sandfeld Jensen 2012-10-08 11:42:27 PDT
(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).
Comment 8 Allan Sandfeld Jensen 2012-10-09 02:52:36 PDT
Created attachment 167719 [details]
Patch
Comment 9 Allan Sandfeld Jensen 2012-10-09 03:21:43 PDT
Landed in r130746, but webkit-path land crashed before closing bug.
Comment 10 Csaba Osztrogonác 2012-10-09 04:16:31 PDT
(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
Comment 11 Allan Sandfeld Jensen 2012-10-09 05:36:54 PDT
Created attachment 167742 [details]
Patch
Comment 12 Allan Sandfeld Jensen 2012-10-09 06:02:27 PDT
Committed r130753: <http://trac.webkit.org/changeset/130753>