1759745 - Brotli compressed HTTP chunks larger than 128k not full processed

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[Andy Hochhaus]

Attachment: brotlichunks.go

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:98.0) Gecko/20100101 Firefox/98.0

Steps to reproduce:

• Load https://brotli.hochhaus.us/
• Note that three HTTP chunks will be sent. The chunks are sent 3s apart.
  • Chunk #1 contains the string "start"
  • Chunk #2 ends with the string "...aaa0123456789E" (note this chunk is 131073 bytes long)
  • Chunk #3 contains the string "end"

Source code for the server is attached.

Actual results:

In FF, chunk #2 originally ends with "...aaa0123456789" and then is amended to "...aaa0123456789E" once chunk #3 arrives (3s later).

Note that FF uses a 128k brotli buffer (see nsHTTPCompressConv::BrotliHandler()):

https://hg.mozilla.org/integration/fx-team/file/1aa20bcbb80e1014e4d01057f7d52269b0c2d908/netwerk/streamconv/converters/nsHTTPCompressConv.cpp

const size_t kOutSize = 128 * 1024; // just a chunk size, we call in a loop

It appears that if more than 128k arrives at a time the loop does not "take an additional pass" aggressively enough. Instead, FF waits until more data arrives (eg: chunk #3) and at that point processes the remaining portion of the large chunk.

Expected results:

FF should behave the same as Chrome/Edge/Safari where chunk #2 correctly ends with "...aaa0123456789E" (even before chunk #3 arrives).

Also note that if you opt into gzip compression (eg: https://brotli.hochhaus.us/?gzip=1) or select a chunk size up to 128k (eg: https://brotli.hochhaus.us/?size=131072) all chunks arrive as expected (even in FF).

Comment 1

[Andy Hochhaus]

I built a local copy of Firefox to add some logging and had some additional insights into the problem.

The existing code is written assuming that BrotliDecoderDecompressStream() will return BROTLI_DECODER_RESULT_NEEDS_MORE_OUTPUT if all output has not been consumed. However, this appears not to be the case. Instead, per the docs, BROTLI_DECODER_RESULT_NEEDS_MORE_OUTPUT is only returned when decoding is blocked until more output space is provided. If I am reading the upstream docs correctly, unconsumed output could still be internally buffered by the brotli library even when BROTLI_DECODER_RESULT_NEEDS_MORE_INPUT is returned. Only when the internal buffer is full is BROTLI_DECODER_RESULT_NEEDS_MORE_OUTPUT guaranteed to be returned.

To detect this case, the upstream brotli library provides BrotliDecoderHasMoreOutput() and BrotliDecoderTakeOutput(). I have attached a small patch which resolves this issue in my testing. I would very much appreciate guidance as to if this direction is an acceptable path.

Comment 2

[Andy Hochhaus]

Attachment: nsHTTPCompressConv.cpp.diff

Comment 3

[Valentin Gosu [:valentin]]

Kershaw, Andy has posted a patch. Could you take a look if it's the right approach, and guide them through what's needed for landin this?
It would be nice to have a unit test for this too. Thanks!

Comment 4

[Kershaw Chang [:kershaw]]

Hi Andy,

Thanks for the patch.
Are you willing to put this patch on Phabricator for reviewing? More details can be found in this link.

Comment 5

[Andy Hochhaus]

Attachment: Bug 1759745 - Brotli compressed HTTP chunks larger than 128k not fully processed

Comment 6

[Andy Hochhaus]

Thanks Valentin and Kershaw.

It would be nice to have a unit test for this too.

Absolutely. Valentin, could you please provide a pointer to an example that I could follow? The tricky thing about this unit test is that the data is eventually processed (either after the stream is closed or the next chunk is delivered). I don't want to add a sleep in the unit test for multiple reasons but I'm not sure of how best to write such a test in the firefox codebase.

Are you willing to put this patch on Phabricator for reviewing?

Submitted. Kershaw, are you the best person to review? If so, how do I request the review from you? If not, can you suggest someone else who might be willing?

Comment 7

[Kershaw Chang [:kershaw]]

Sorry for the delay.
I'll take a look by the end of this week.

Comment 8

[Kershaw Chang [:kershaw]]

I've left some comment and I'll try to write a test for this.

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: WIP: Bug 1759745 - test case

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb64951a3a4f
Brotli compressed HTTP chunks larger than 128k not fully processed r=necko-reviewers,kershaw

Comment 11

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/eb64951a3a4f