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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mshneer, Assigned: mcmanus)

Tracking

44 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 wontfix, firefox46 fixed, firefox47 fixed, firefox-esr45 fixed)

Details

Attachments

(1 attachment)

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)
Component: Untriaged → Networking
Product: Firefox → Core
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.
Awesome! I downloaded nightly and it works as advertised now. Thanks for looking into this.
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.)
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Depends on: 1242904
Resolution: --- → FIXED
Duplicate of this bug: 1255810
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.
Assignee: nobody → 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.
Attachment #8734104 - Flags: approval-mozilla-esr45?
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 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
Attachment #8734104 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
You need to log in before you can comment on or make changes to this bug.