Closed Bug 852957 Opened 7 years ago Closed 6 years ago

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

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Paolo, Assigned: raymondlee)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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.
Assignee: nobody → marcos
Assignee: marcos → raymond
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
Attachment #750299 - Flags: review?(paolo.mozmail)
Attached patch v2 WIP (obsolete) — Splinter Review
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?
Attachment #750355 - Flags: feedback?(paolo.mozmail)
Attachment #750355 - Attachment is patch: true
Attachment #750355 - Attachment mime type: text/x-patch → text/plain
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.
Attachment #750355 - Flags: feedback?(paolo.mozmail)
Attachment #750299 - Flags: review?(paolo.mozmail)
Attached patch v3 (obsolete) — Splinter Review
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?
Attachment #750299 - Attachment is obsolete: true
Attachment #750355 - Attachment is obsolete: true
Attachment #752693 - Flags: review?(paolo.mozmail)
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).
Attachment #752693 - Flags: review?(paolo.mozmail)
Attached patch v4 (obsolete) — Splinter Review
(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
Attachment #752693 - Attachment is obsolete: true
Attachment #753192 - Flags: review?(paolo.mozmail)
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.
Attachment #753192 - Flags: review?(paolo.mozmail)
Attached patch v5 (obsolete) — Splinter Review
Attachment #753192 - Attachment is obsolete: true
Attachment #757853 - Flags: review?(paolo.mozmail)
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.
Attachment #757853 - Flags: review?(paolo.mozmail) → review+
Attachment #757853 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6fee06d61d20
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.