852957 - Add tests for channel decoding when the "Content-Encoding" header is present

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

When downloading a file from a channel that has a valid "Content-Encoding"
header, the byte progress events, content length, and byte range requests all
refer to the HTTP entity-body. If automatic decoding is enabled, those byte
indications may not match what is received by the channel stream, and thus
what is written into the target file.

The new JavaScript API for downloads should include tests for those cases.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v2 WIP

This patch adds one more test test_download_cancel_midway_restart_with_content_encoding.

Hoever, it doesn't return any progress until it reaches 100 so the download can't be canceled midway.  I can't figure out why.  Is it something to do with content-encoding: gzip won't return any progress?

Comment 3

[:Paolo Amadini]

Comment on attachment 750355 [details] [diff] [review]
v2 WIP

(In reply to Raymond Lee [:raymondlee] from comment #2)
> Hoever, it doesn't return any progress until it reaches 100 so the download
> can't be canceled midway.  I can't figure out why.  Is it something to do
> with content-encoding: gzip won't return any progress?

The reason is that the entire body of the response is sent in one go, while the
body should be divided in two parts, like the handlers registered in head.js do.

You may add a registerInterruptibleHandler call to test_common_initialize, and
use that handler in a similar way to other test cases that handle responses in
two phases.

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v3

I have added some code to test_common_initialize.  However, the writeByteArray is only written to the target file once, even it has been executed in both first and second phrase.  The target file only contains TEST_DATA_SHORT instead of TEST_DATA_SHORT+TEST_DATA_SHORT.  Is this expected?

Comment 5

[:Paolo Amadini]

Comment on attachment 752693 [details] [diff] [review]
v3

There are a couple of things. Firstly, I'm not sure that you can just
concatenate the encoded GZIP data twice, you should probably split the array
of bytes in two parts and send each part once. Secondly, to avoid concurrency
issues, you should create a different instance of nsIBinaryOutputStream for
every function call, because the firstPart and secondPart functions can be
called for multiple requests at the same time, in no particular order (for
example, firstPart for request 1, firstPart for request 2, secondPart for
request 1, and it's also possible that secondPart for request 2 is not called).

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v4

(In reply to Paolo Amadini [:paolo] from comment #5)
> Comment on attachment 752693 [details] [diff] [review]
> v3
> 
> There are a couple of things. Firstly, I'm not sure that you can just
> concatenate the encoded GZIP data twice, you should probably split the array
> of bytes in two parts and send each part once. Secondly, to avoid concurrency
> issues, you should create a different instance of nsIBinaryOutputStream for
> every function call, because the firstPart and secondPart functions can be
> called for multiple requests at the same time, in no particular order (for
> example, firstPart for request 1, firstPart for request 2, secondPart for
> request 1, and it's also possible that secondPart for request 2 is not
> called).

OK, done

Comment 7

[:Paolo Amadini]

Comment on attachment 753192 [details] [diff] [review]
v4

Review of attachment 753192 [details] [diff] [review]:
-----------------------------------------------------------------

This is almost ready, I've just a few comments before the final review.

::: toolkit/components/jsdownloads/test/unit/head.js
@@ +365,5 @@
> +    function firstPart(aRequest, aResponse) {
> +      aResponse.setHeader("Content-Type", "text/plain", false);
> +      aResponse.setHeader("Content-Encoding", "gzip", false);
> +      aResponse.setHeader("Content-Length", "" + TEST_DATA_SHORT_GZIP_ENCODED.length);
> +      aResponse.processAsync();

We call processAsync and "finish" in our infrastructure already, no need to repeat.

@@ +369,5 @@
> +      aResponse.processAsync();
> +
> +      let bos = Cc["@mozilla.org/binaryoutputstream;1"].
> +                  createInstance(Ci.nsIBinaryOutputStream);
> +      bos.setOutputStream(aResponse.bodyOutputStream);

You may add a Components.constructor declaration to the file header, with setOutputStream as the initialization call:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/test/unit/head.js#39

::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
@@ +761,5 @@
> +
> +  function cleanup() {
> +    gHttpServer.registerPathHandler(source_path, null);
> +  }
> +  

nit: whitespace at end of line.

@@ +767,5 @@
> +
> +  gHttpServer.registerPathHandler(source_path, function (aRequest, aResponse) {
> +    aResponse.setHeader("Content-Type", "text/plain", false);
> +    aResponse.setHeader("Content-Encoding", "gzip", false);
> +    aResponse.setHeader("Content-Length", "" + TEST_DATA_SHORT_GZIP_ENCODED.length, false);

indentation nit:

    aResponse.setHeader("Content-Length",
                        "" + TEST_DATA_SHORT_GZIP_ENCODED.length, false);

@@ +775,5 @@
> +    bos.setOutputStream(aResponse.bodyOutputStream);
> +    aResponse.processAsync();
> +    bos.writeByteArray(TEST_DATA_SHORT_GZIP_ENCODED,
> +                       TEST_DATA_SHORT_GZIP_ENCODED.length);
> +    aResponse.finish();

You can avoid processAsync and "finish" also here. In this case, it's because we write the entire response before returning from the function.

@@ +789,5 @@
> +  do_check_true(download.stopped);
> +  do_check_true(download.succeeded);
> +  do_check_false(download.canceled);
> +  do_check_eq(download.progress, 100);
> +  do_check_eq(download.totalBytes, TEST_DATA_SHORT_GZIP_ENCODED.length);

Please check only "progress" and totalBytes.

In general, when we are pretty sure that an object behaves in a certain way, based on other test cases in the suite and our knowledge of the code, there is no need to verify that again.

In this case, start() succeeding is enough for avoiding the state tests. We are checking progress and totalBytes because we want to assert their behavior, since we know there are different data lengths involved.

@@ +810,5 @@
> +    let deferCancel = Promise.defer();
> +    download.onchange = function () {
> +      if (!download.stopped && !download.canceled && download.progress >= 50,
> +           download.progress < 100) {
> +        deferCancel.resolve(download.cancel());

There's something strange here... maybe you could use a condition based on download.currentBytes equal to the data length of the first part, as the progress percentage is less precise here.

@@ +837,5 @@
> +  do_check_eq(download.progress, 0);
> +  do_check_eq(download.totalBytes, 0);
> +  do_check_eq(download.currentBytes, 0);
> +
> +  yield promiseAttempt;

Also here, all this block from "let promiseAttempt = download.start();" to "yield promiseAttempt;" can just become "yield download.start()";

@@ +843,5 @@
> +  do_check_true(download.stopped);
> +  do_check_true(download.succeeded);
> +  do_check_false(download.canceled);
> +  do_check_eq(download.progress, 100);
> +  do_check_eq(download.totalBytes, TEST_DATA_SHORT_GZIP_ENCODED.length);

And check only "progress" and totalBytes here.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v5

Comment 9

[:Paolo Amadini]

Comment on attachment 757853 [details] [diff] [review]
v5

Review of attachment 757853 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! r+ with the change below.

::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
@@ +851,5 @@
> +    let deferCancel = Promise.defer();
> +    download.onchange = function () {
> +      // The data length of first part is 25.
> +      if (!download.stopped && !download.canceled &&
> +          download.currentBytes == 25) {

Just use TEST_DATA_SHORT_GZIP_ENCODED_FIRST.length, no comment needed.

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 11

[Raymond Lee [:raymondlee]]

https://tbpl.mozilla.org/?tree=Try&rev=7c971dd76687
Passed try

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fee06d61d20

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fee06d61d20