1254411 - Rendering of chunked responses compressed with brotli is delayed until entire request is downloaded

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Misha Shneerson]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36

Steps to reproduce:

Constructed a page simulating brotli compression. The page renders 5 chunks with 1 second interval between them. There are 3 variants of this page:

1. chunks are rendered without any compression 
http://beta.facebook.com/brotli/test.php?orig

2. chunks are rendered with brotli compression enabled
http://beta.facebook.com/brotli/test.php

3. chunks were compressed on the server then decompressed. render w/o compression enabled.
http://beta.facebook.com/brotli/test.php?decompressed

Tests were done on Mac, FireFox build 44.0.1


Actual results:

This page renders all at the same time, with 4 seconds delay. i.e. all chunks are downloaded before browser renders the content
http://beta.facebook.com/brotli/test.php


Expected results:

The content should be rendered similarly to this:
http://beta.facebook.com/brotli/test.php?decompressed

(well, ideally it should be rendered like this page - http://beta.facebook.com/brotli/test.php?orig - but there is an outstanding bug in brotli that prevents this from happening. See https://github.com/google/brotli/issues/325)

Comment 1

[Misha Shneerson]

Ooops, just noticed an issue with the URLs - these all should be https.
https://beta.facebook.com/brotli/test.php?orig
https://beta.facebook.com/brotli/test.php
https://beta.facebook.com/brotli/test.php?decompressed

Comment 2

[Daniel Holbert [:dholbert]]

Thanks for reporting!

Good news - this seems to work correctly in current Nightly. (If I load https://beta.facebook.com/brotli/test.php there, I immediately see "Sending data chunk 1 of 1000" -- there's no delay.)

So this is likely a duplicate of a bug that's already been fixed. The only question is, which bug, and how far has the fix made it down our release pipes.

Comment 3

[Misha Shneerson]

Awesome! I downloaded nightly and it works as advertised now. Thanks for looking into this.

Comment 4

[Daniel Holbert [:dholbert]]

Partially-narrowed fix range, using mozregression:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=76733110704b975154ac0fa779445e6eae5da559&tochange=ac39fba33c6daf95b2cda71e588ca18e2eb752ab

That includes exactly one commit that mentions Brotli:
>ad43c7344bdb Frédéric Wang — Bug 1242904 - Update Brotli to latest upstream revision ;
>                               now at 33aa40220b96cf95ad2b9ba61dc8d7fd2f964f2c. r=mcmanus
http://hg.mozilla.org/mozilla-central/rev/ad43c7344bdb

The bug is currently marked as security-sensitive, so you probably can't see it. It looks like the full patch was backported to Aurora46, so this should definitely be fixed in Firefox 46 (which I can confirm locally usinG Firefox Developer Edition).

A tiny, extremely-targeted patch (a cherrypicked fix from upstream brotli) was backported to Beta45 for that bug, and that patch does not seem to have fixed this bug here. So looks like this will be broken in Firefox 45 (which I believe is released this week).

But, it should be fixed in 6 weeks when Firefox 46 is released.

Hence, I'm resolving this as FIXED by bug 1242904, with firefox45 marked as affected, but all later branches marked as FIXED.  (This isn't quite a duplicate of bug 1242904, because as noted above, bug 1242904's beta backport didn't help here.)

Comment 6

[Patrick McManus [:mcmanus]]

brotli content is becoming a bit more popular on the internet and will likely increase (a cdn just added it e.g.).. since this bug impacts esr 45 and the fix (which is the uplift of the upstream library) isn't appropriate to uplift we should disable brotli on esr 45.

< 45 is not impacted (brotli is not implemented there).. > 45 is already fixed.

Comment 7

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1ca3d40803e

Comment 8

[Patrick McManus [:mcmanus]]

Attachment: disable brotli by pref for esr45

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 8734104 [details] [diff] [review]
disable brotli by pref for esr45

This patch disables brotli compression over https via a pref change for ESR 45 due to upstream library bugs creating interop problems. The library is fixed in 46.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: Fixed on 46, but this is a disable patch for ESR 45
Risk to taking this patch (and alternatives if risky): very low - disables a transparent feature for stability.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 10

[Patrick McManus [:mcmanus]]

I believe all the test coverage explicitly sets the pref to what it needs so we shouldn't need corresponding test changes.. but there is a try run in comment 7 to be sure.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8734104 [details] [diff] [review]
disable brotli by pref for esr45

Improve the ESR quality, taking it in esr.
Should be in 45.1.0

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/c01d81ef2f5481d29295270599bec5d7c6db70ec (its finally on esr45)