GNOME Bugzilla – Bug 755976
Seeking with GDataDownloadStream can be broken if a range was set in the HTTP request
Last modified: 2018-09-21 16:25:08 UTC
Created attachment 312555 [details] Testcase that shows the difference between seeking file://, davs:// and google-drive:// URIs I can spot two issues: (a) GDataDownloadStream:content-length doesn't take into account the range that we might have set in the request header. If a range was set, it only reports the size of response received from the server, which will be smaller than the size of the actual file. Since GDataDownloadStream:content-length is documented as: "The length (in bytes) of the file being downloaded. This will initially be -1, and will be populated as soon as the appropriate header is received from the server. Its value will never change after this." I believe this is a bug in GDataDownloadStream. (b) Similarly when popping bytes from the GDataBuffer, we don't take into account the range that might have been set when sending the request.
Created attachment 312557 [details] [review] core: GDataDownloadStream:content-length should consider Content-Range
Review of attachment 312557 [details] [review]: ::: gdata/gdata-download-stream.c @@ +827,3 @@ self->priv->content_length = soup_message_headers_get_content_length (message->response_headers); + if (soup_message_headers_get_content_range (message->response_headers, &start, &end, NULL)) { + self->priv->content_length += (gssize) start; I am not sure about, but I think that content_length might be unknown in some cases and consequently you will set content length to wrong value this way... It might be better to use following, mightn't be? if (soup_message_headers_get_content_range (message->response_headers, &start, &end, &total_length)) self->priv->content_length = total_length;
Created attachment 312566 [details] [review] core: Don't duplicate the received chunks in GDataDownloadStream
Review of attachment 312557 [details] [review]: ::: gdata/gdata-download-stream.c @@ +827,3 @@ self->priv->content_length = soup_message_headers_get_content_length (message->response_headers); + if (soup_message_headers_get_content_range (message->response_headers, &start, &end, NULL)) { + self->priv->content_length += (gssize) start; Ondrej’s suggestion looks correct to me — total_length will either be valid, or -1 for unknown — and we want to set priv->content_length to -1 if it’s unknown.
Review of attachment 312566 [details] [review]: Nice catch.
Created attachment 312570 [details] [review] core: GDataDownloadStream:content-length should consider Content-Range
Comment on attachment 312566 [details] [review] core: Don't duplicate the received chunks in GDataDownloadStream Pushed to master.
There is one thing though, soup_message_headers_get_content_length returns 0, not -1, when Content-Length is not found, while GDataDownloadStream::content_length assumes that the unknown value is -1, not 0. For example, this will break the condition in gdata_download_stream_seek. I wonder if this needs fixing.
Review of attachment 312570 [details] [review]: ++
(In reply to Debarshi Ray from comment #8) > There is one thing though, soup_message_headers_get_content_length returns > 0, not -1, when Content-Length is not found, while > GDataDownloadStream::content_length assumes that the unknown value is -1, > not 0. For example, this will break the condition in > gdata_download_stream_seek. > > I wonder if this needs fixing. I think it probably does. Can you also please extend the streams unit tests in gdata/tests to cover these situations?
And with this, LibreOffice can open ODFs from Drive, but it is painfully slow. I think because of the way we restart the network thread and re-download when seeking back over data that has already been read. My understanding is that very recent LO locally downloads remote files and does the seeks on the local copy, so this might not be that big of an issue.
(In reply to Philip Withnall from comment #10) > Can you also please extend the streams unit tests in gdata/tests to cover > these situations? Sure.
(In reply to Debarshi Ray from comment #11) > And with this, LibreOffice can open ODFs from Drive, but it is painfully > slow. I think because of the way we restart the network thread and > re-download when seeking back over data that has already been read. > > My understanding is that very recent LO locally downloads remote files and > does the seeks on the local copy, so this might not be that big of an issue. Indeed, http://cgit.freedesktop.org/libreoffice/core/commit/?id=93a0696e74e96e5a5ca821f33cb791b87e876f49
(In reply to Debarshi Ray from comment #12) > (In reply to Philip Withnall from comment #10) > > Can you also please extend the streams unit tests in gdata/tests to cover > > these situations? > > Sure. Any progress on this?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libgdata/issues/18.