Since enabling fatal criticals for layout tests, test http/tests/local/blob/send-hybrid-blob-using-open-panel.html has been crashing somewhere in the network process (there is no backtrace unfortunately) with libsoup-CRITICAL **: soup_message_body_append_buffer: assertion 'buffer->length > 0' failed. Updating expectations accordingly.
Created attachment 346763 [details] Patch
Comment on attachment 346763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346763&action=review > Source/WebCore/ChangeLog:3 > + [SOUP] Check length before calling soup_message_body_append_buffer. Use the bug title here, either change it here or change the bug title. > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:54 > + if (soupBuffer->length) The thing is, is this expected to be 0, or is it 0 because of another bug? I'm afraid we might be hiding the actual bug with this patch.
(In reply to Carlos Garcia Campos from comment #2) > Comment on attachment 346763 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346763&action=review > > > Source/WebCore/ChangeLog:3 > > + [SOUP] Check length before calling soup_message_body_append_buffer. > > Use the bug title here, either change it here or change the bug title. Done. > > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:54 > > + if (soupBuffer->length) > > The thing is, is this expected to be 0, or is it 0 because of another bug? > I'm afraid we might be hiding the actual bug with this patch. I think ending up with an empty buffer here is just the logical result of reading an empty file.
Comment on attachment 346763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346763&action=review >>> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:54 >>> + if (soupBuffer->length) >> >> The thing is, is this expected to be 0, or is it 0 because of another bug? I'm afraid we might be hiding the actual bug with this patch. > > I think ending up with an empty buffer here is just the logical result of reading an empty file. It make sense. If it's expected, then I guess there's no point in creating an empty SoupBuffer just to check that is empty. I think we could do something like: if (blobItem.length() == BlobDataItem::toEndOfFile) return 0; GUniquePtr<SoupBuffer> soupBuffer(buffer->createSoupBuffer(blobItem.offset(), blobItem.length())); soup_message_body_append_buffer(soupMessage->request_body, soupBuffer.get()); return soupBuffer->length;
(In reply to Carlos Garcia Campos from comment #4) > Comment on attachment 346763 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346763&action=review > > >>> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:54 > >>> + if (soupBuffer->length) > >> > >> The thing is, is this expected to be 0, or is it 0 because of another bug? I'm afraid we might be hiding the actual bug with this patch. > > > > I think ending up with an empty buffer here is just the logical result of reading an empty file. > > It make sense. If it's expected, then I guess there's no point in creating > an empty SoupBuffer just to check that is empty. I think we could do > something like: > > if (blobItem.length() == BlobDataItem::toEndOfFile) > return 0; > > GUniquePtr<SoupBuffer> > soupBuffer(buffer->createSoupBuffer(blobItem.offset(), blobItem.length())); > soup_message_body_append_buffer(soupMessage->request_body, soupBuffer.get()); > return soupBuffer->length; That's not what a zero size argument to createSoupBuffer means; it uses > size ? size : this->size() for some odd reason.
(In reply to Ms2ger from comment #5) > (In reply to Carlos Garcia Campos from comment #4) > > Comment on attachment 346763 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=346763&action=review > > > > >>> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:54 > > >>> + if (soupBuffer->length) > > >> > > >> The thing is, is this expected to be 0, or is it 0 because of another bug? I'm afraid we might be hiding the actual bug with this patch. > > > > > > I think ending up with an empty buffer here is just the logical result of reading an empty file. > > > > It make sense. If it's expected, then I guess there's no point in creating > > an empty SoupBuffer just to check that is empty. I think we could do > > something like: > > > > if (blobItem.length() == BlobDataItem::toEndOfFile) > > return 0; > > > > GUniquePtr<SoupBuffer> > > soupBuffer(buffer->createSoupBuffer(blobItem.offset(), blobItem.length())); > > soup_message_body_append_buffer(soupMessage->request_body, soupBuffer.get()); > > return soupBuffer->length; > > That's not what a zero size argument to createSoupBuffer means; it uses > > > size ? size : this->size() > > for some odd reason. This is because you can create a sub-buffer, that's why it receives an offset and a size. If the size is 0, then the actual buffer size is used. This was added before std::optional. In this case, if the file is empty the buffer size will be 0 in any case. We can change the condition to: if (buffer->isEmpty()) return 0; That way we are actually checking that it's an empty file. Previous check was indeed wrong.
Created attachment 347488 [details] Patch
Comment on attachment 347488 [details] Patch Clearing flags on attachment: 347488 Committed r235026: <https://trac.webkit.org/changeset/235026>
All reviewed patches have been landed. Closing bug.