After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 755976 - Seeking with GDataDownloadStream can be broken if a range was set in the HTTP request
Seeking with GDataDownloadStream can be broken if a range was set in the HTTP...
Status: RESOLVED OBSOLETE
Product: libgdata
Classification: Platform
Component: General
0.17.x
Other All
: Normal normal
: ---
Assigned To: libgdata-maint
libgdata-maint
Depends on:
Blocks:
 
 
Reported: 2015-10-02 10:04 UTC by Debarshi Ray
Modified: 2018-09-21 16:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Testcase that shows the difference between seeking file://, davs:// and google-drive:// URIs (3.91 KB, text/x-csrc)
2015-10-02 10:04 UTC, Debarshi Ray
  Details
core: GDataDownloadStream:content-length should consider Content-Range (1.56 KB, patch)
2015-10-02 10:48 UTC, Debarshi Ray
none Details | Review
core: Don't duplicate the received chunks in GDataDownloadStream (1.09 KB, patch)
2015-10-02 14:03 UTC, Debarshi Ray
committed Details | Review
core: GDataDownloadStream:content-length should consider Content-Range (1.60 KB, patch)
2015-10-02 15:13 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2015-10-02 10:04:23 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.
Comment 1 Debarshi Ray 2015-10-02 10:48:41 UTC
Created attachment 312557 [details] [review]
core: GDataDownloadStream:content-length should consider Content-Range
Comment 2 Ondrej Holy 2015-10-02 10:58:49 UTC
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;
Comment 3 Debarshi Ray 2015-10-02 14:03:25 UTC
Created attachment 312566 [details] [review]
core: Don't duplicate the received chunks in GDataDownloadStream
Comment 4 Philip Withnall 2015-10-02 14:41:33 UTC
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.
Comment 5 Philip Withnall 2015-10-02 14:44:13 UTC
Review of attachment 312566 [details] [review]:

Nice catch.
Comment 6 Debarshi Ray 2015-10-02 15:13:09 UTC
Created attachment 312570 [details] [review]
core: GDataDownloadStream:content-length should consider Content-Range
Comment 7 Debarshi Ray 2015-10-02 15:13:34 UTC
Comment on attachment 312566 [details] [review]
core: Don't duplicate the received chunks in GDataDownloadStream

Pushed to master.
Comment 8 Debarshi Ray 2015-10-02 15:17:40 UTC
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.
Comment 9 Philip Withnall 2015-10-02 15:44:24 UTC
Review of attachment 312570 [details] [review]:

++
Comment 10 Philip Withnall 2015-10-02 15:45:17 UTC
(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?
Comment 11 Debarshi Ray 2015-10-02 16:27:05 UTC
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.
Comment 12 Debarshi Ray 2015-10-02 16:27:27 UTC
(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.
Comment 13 Ondrej Holy 2015-10-05 06:29:33 UTC
(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
Comment 14 Philip Withnall 2015-11-19 12:24:27 UTC
(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?
Comment 15 GNOME Infrastructure Team 2018-09-21 16:25:08 UTC
-- 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.