[Webkit-unassigned] [Bug 85880] [SOUP] Allow sending URI request data in chunks

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-05-09 23:52:45 PST ---
(In reply to comment #7)
> (From update of attachment 140735 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140735&action=review
> 
> This looks pretty good to me! I have a few outstanding questions though, before I feel comfortable giving an r+.

Thanks!

> > Source/WebKit2/ChangeLog:7
> > +        Reviewed by NOBODY (OOPS!).
> > +
> 
> It's probably a good idea to write a couple sentences describing this change in general, for the sake of future readers.

Fair enough

> > Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:78
> >      if (!m_client.didReceiveURIRequest(this, WebURL::create(uriString).get(), requestID))
> > -        handleURIRequest(WebData::create(0, 0).get(), String(), requestID);
> > +        handleURIRequest(WebData::create(0, 0).get(), 0, String(), requestID);
> 
> Hrm. Perhaps this should return an error?

No, this is handled in the web process, and indeed an error is shown, the same error that if you type foo:bar and foo is not handled.

> > Source/WebKit2/WebProcess/soup/WebKitSoupRequestInputStream.cpp:45
> > +    uint64_t bytesAdded;
> 
> bytesReceivedFromWebProcess or bytesReceived? bytesAdded could easily refer to "bytes added to the output."

Ok, I'll think a better name.

> > Source/WebKit2/WebProcess/soup/WebKitSoupRequestInputStream.cpp:48
> > +    Mutex readLock;
> 
> I'm a little confused as to what exactly this mutex is protecting. Is it protecting just the data in this private structure or any operations at all?

GLib will run read_async in a thread, so webkitSoupRequestInputStreamReadAsync() and webkitSoupRequestInputStreamAddData() can be run at the same time. We need to protect bytesAdded, asyncReadData, etc.

> > Source/WebKit2/WebProcess/soup/WebKitSoupRequestInputStream.cpp:81
> > +        stream->priv->asyncReadData = adoptPtr(new AsyncReadData(result.get(), buffer, count, cancellable));
> 
> Won't this break if g_input_stream_read_async is called twice in a row?

You are not supposed to call read_async twice in a row, in such case, our read_async is not called because g_input_stream_read_async() returns early with the error G_IO_ERROR_PENDING.

> > Source/WebKit2/WebProcess/soup/WebKitSoupRequestInputStream.cpp:135
> > +    if (webkitSoupRequestInputStreamFinished(stream))
> > +        return;
> 
> Does this need to be protected by the mutex or can the mutex be locked after this early return¿

That reads bytesAdded that is only written by this method, so it can probably be moved after the early return.

> > Source/WebKit2/WebProcess/soup/WebKitSoupRequestInputStream.cpp:137
> > +    if (stream->priv->bytesAdded + dataLength > stream->priv->contentLength)
> > +        dataLength = stream->priv->contentLength - stream->priv->bytesAdded;
> 
> Dan should confirm, but is it really an error to pass more data to soup than specified by the content length?

That's not actually an error, the data is simply truncated.

> What about the case where contentLength = -1 that he mentioned above?

We are not supporting such case, we need to now when more data is expected. We could implement that case by always sending a 0 bytes data message when contentLength is -1 and all data has been read from the input stream.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.