Closed Bug 1273547 Opened 5 years ago Closed 4 years ago

Disable Brotli for Firefox 45

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: Harald, Assigned: mcmanus)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 2 obsolete files)

Firefox 45 is still 5% of Firefox users (via partner, doesn't recognize minor versions to filter out ESR releases). 45 ships with a broken Brotli implementation (not enabled in ESR) with an issue for decoding multiple brotli files in parallel (hangs the browser).

With FF 45 still being out there for some weeks, we could consider a hotfix that prefs off brotli.
:mcmanaus, who has the technical details and was thinking about the hotfix.
Flags: needinfo?(mcmanus)
I wrote lawrence about this.. not sure quite how to make the call, but the fix is trivial if you wanted to do it - change the pref network.http.accept-encoding.secure to be "gzip, deflate"

I don't believe this hangs the browser fwiw - just corrupts the page. (or maybe I haven't seen the bug?)

this is going to be facebook related - and right now they are only doing brotli on dynamic resources which they can filter out FF45 for. So they aren't seeing breakage. They would like to "soon" (as I understand it in a few weeks) doing this on the CDN which wouldn't be able to do filtering by version..

so the question might be what would non-esr 45 usage look like at that time?


-
Flags: needinfo?(mcmanus)
If launch is a few weeks out FF 45 should be negligible. ni? Ben for more details on the impact.
Flags: needinfo?(ben.maurer)
Yeah, so we're still seeing ~5% of firefox traffic to FB coming from FF 45. Of that traffic essentially all of it is sending us accept-encoding:br headers.

My understanding when we did some internal testing on facebook with brotli enabled for all resources on ff45 was that it would cause the browser tab that was opening FB to hang.
Flags: needinfo?(ben.maurer)
Patrick seem to know what to do here.
Assignee: nobody → mcmanus
Whiteboard: [necko-active]
Any news here? We're still seeing similar levels of brotli-requesting FF45 users. We're getting pretty close to being able to fully launch brotli for static resources which is difficult to gate to these users. I totally understand that there's a risk of the hotfix not getting to those users but I'd like to do everything in our collective powers to give them a good experience.
comment 6
Flags: needinfo?(lmandel)
Patrick, you are asking us to run an hotfix for 45 users (not ESR), Correct?

By the way, do you have the bug number in which we disabled Brotli in 45 ESR? Thanks
Flags: needinfo?(lmandel)
(In reply to Sylvestre Ledru [:sylvestre] from comment #8)
> Patrick, you are asking us to run an hotfix for 45 users (not ESR), Correct?
>

I am asking you to respond (and decide) to Ben from Facebook who is asking about a hotfix for ESR 45 users. I don't think that's generally something we do, but its a decision that belongs with release management imo. The tradeoff is clear.

(In reply to Sylvestre Ledru [:sylvestre] from comment #8)

> 
> By the way, do you have the bug number in which we disabled Brotli in 45
> ESR? Thanks

https://bugzilla.mozilla.org/show_bug.cgi?id=1254411#c11
I am assuming that the user on 45 who has not going to 46 (and soon 47) is going to be correlated with the user who won't pick of a 45 hotfix. Maybe that's not true?
We did hotfixes in the past for this kind of things. If you are happy with it, let's do it.
If I understand correct, we want:
* an hotfix targeting only Firefox 45.* users (not ESR)
* once the user updates to 46, we want to turn back on brotli? (I will have to double check how to do that)

About ESR, it seems it is NOT disabled in esr (because by setting the value to fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1254411#c6 the sheriff didn't see it)
Could you double check that?
(to see if we have to disable it in esr 45.2.0 too)
(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> About ESR, it seems it is NOT disabled in esr (because by setting the value
> to fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=1254411#c6 the
> sheriff didn't see it)
> Could you double check that?

oh man. you're right - this is tip of esr 45 https://hg.mozilla.org/releases/mozilla-esr45/file/064d2636b91b7b8a8bbd220e44510af95fbf79e9/modules/libpref/init/all.js#l1352 and it has brotli enabled.

that sheriff thing is weird. The other approval flags (aurora/beta) are routinely hoovered up by the sheriff after going to FIXED, right? (FIXED generally applies to m-c state and often the approvals don't happen until something is already FIXED.) Just trying to understand - don't like losing patches.

so it seems to me that 45 ESR just needs to land the approved 
https://bugzilla.mozilla.org/show_bug.cgi?id=1254411#c11

and 45-regular needs a hotfix that deals allows the default to rule when >=46 is installed. If I'm the one that needs to put that together I need a pointer on how to make it happen (all we're doing is twiddling the above mentioned pref).
I will have a look, I just unsure how to make sure that the flag is back on once a user updated from 45 to 46.
Depends on: 1254411
Ben, I wonder how much of the 45 traffic you have is native 45 vs ESR. We had been assuming it wasn't ESR because of the mistake about whether brotli was already disabled there or not - which I'm certainly glad this discussion has surfaced!

This matters because those users will get the change from different streams.

I believe its reflected in the user-agent string if you have that data..
Flags: needinfo?(ben.maurer)
I think our stats are saying the vast majority is non-ESR (like 99%+). That said, we haven't really looked at this stuff before and it's possible there's an issue with our UA parsing. Do you guys have any stats on your end here?
Flags: needinfo?(ben.maurer)
(In reply to Patrick McManus [:mcmanus] from comment #12)

> 
> that sheriff thing is weird. The other approval flags (aurora/beta) are
> routinely hoovered up by the sheriff after going to FIXED, right? (FIXED
> generally applies to m-c state and often the approvals don't happen until
> something is already FIXED.) Just trying to understand - don't like losing
> patches.
> 

just to clear up some confusion.

Yes, you are right, the general Bug Status is set to Fixed when something land on m-c. And normally a uplift nomination and approval happen when something is on m-c 

So the general Bug status in https://bugzilla.mozilla.org/show_bug.cgi?id=1254411#c4 was ok.

What happened then is that status-firefox-esr45: --- → fixed was set and this indicate that also esr45 was fixed in some ways.

The way uplift queries happen is that they search for bugs that have approval (in example Firefox48) and where Firefox48 is not fixed (so a uplift needs to be happen). 

So because it was assumed esr45 is fixed (and not affected or something other than fixed or verified) that bug was missed. 

We as sheriffs only set something like status-firefox-esr45: --- → fixed  when the uplift checkin has landed. 

> so it seems to me that 45 ESR just needs to land the approved 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1254411#c11
> 

thats done now
thanks carsten
How long will it take for ESR users to get this update?
https://wiki.mozilla.org/RapidRelease/Calendar looks like a june 7 release date is planned atm
Sylvestre, I think the next step is yours.. if its mine I guess that's a valuable outcome of this ni :)
Flags: needinfo?(sledru)
We discussed with Benjamin Smedberg and it seems that we should use a system addon (go faster) instead of a hotfix. I haven't find the time to do it myself. So, you are motivated in trying, this would be really appreciated. Sorry about the lag.
Flags: needinfo?(sledru)
We are starting to see more FF45 users that have brotli disabled since the new ESR version came out. However, the majority of FF45 users are still sending us brotli headers. Overall 2% of firefox users seem to still be at risk here

This week we are going to be launching Brotli support for all JS/CSS on the site. Because this is served by the CDN it's not really possible to gate this based on user agent. Is there anything we can do to further reduce the number of users impacted.
Hey Ben - the concept of "pushing out non critical stuff to old releases" isn't a common one around here so this has gone slowly. I did have a meeting on Friday where Felipe helped me learn what I need to know to get it done - its not particularly hard.

It's currently #2 on my todo list. So stay tuned.
Attached patch disable brotli on 45 (obsolete) — Splinter Review
Comment on attachment 8763937 [details] [diff] [review]
disable brotli on 45

Hey Stephen, Hey Mark -

I'm trying to push out a change to firefox 45 to disable brotli via a pref. Bsmedberg pointed us at a system add on and I had a chat on friday with Felipe about how that works (its all new to me).

It seems pretty easy but I'm not really sure how it rolls out in practice. I tested this code by
1] installing a fresh 45.0.2 and fresh profile.. confirming brotli was on
2] copied the directory with the add on to browser/extensions
3] confirmed that that brotli was off after the install method ran
4] upgraded (which went right to 47)
5] confirmed that brotli was back on

The diff is against the 45.0.2 tag of mozilla-release. I presumed there would be some kind of xpi involved but none was made when I compiled the tree.

Felipe is on PTO this week but he said you guys would be able to help me figure out the next step on how to get this reviewed and deployed - that's totally foreign territory for me. There is a bit of a time element here - thanks!
Attachment #8763937 - Flags: review?(standard8)
Attachment #8763937 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8763937 [details] [diff] [review]
disable brotli on 45

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

::: browser/extensions/brotli/bootstrap.js
@@ +10,5 @@
> +
> +function startup() {
> +}
> +
> +function install() {

My understanding is that |install()| is only run when the addon is first installed. I was going to say that this should be in |startup()|, but I'm confused by the fact that you say the pref is reverted once you update Firefox. This makes it seem like either |install()| is run again after an update, or you're re-downloading and installing the system addon. Could you find out which one it is?

Note: When a system addon is installed, it's supposed to run |install()| followed by |startup()|. On subsequent starts of Firefox, only |startup()| is run.
Attachment #8763937 - Flags: review?(spohl.mozilla.bugs)
For testing: You can override the URL to the update.xml in prefs ("extensions.systemAddon.update.url") and make it point to a test server of yours (such as people.mozilla.org). Here's a sample update.xml that you can modify[1]. You can then test the download and install of the addon.

To get the .xpi, simply zip up the system addon directory and change the file extension to .xpi. Then place it on the server and point the update.xml to it.


[1] https://aus4-dev.allizom.org/update/3/SystemAddons/32.0/20140728123855/default/en-US/nightlytest/default/default/default/update.xml
interesting that the test described in comment 25 definitely ran install() when firefox was upgraded, but not between restarts on the same version - which is optimal here. but if that's not reliable, I won't rely on it :) Maybe upgrading firefox counts as upgrading a system addon?

thanks for the note on the xpi - the update.url was the info I was missing.
The two other questions I had:
1. Should this addon be uninstalled at some point? If yes, how is this done in the system addons world?
2. Would this addon benefit from telemetry?

I don't have answers to these questions.
thanks for your help.. I'm having a lot of trouble loading the xpi.

I simply get:
1466542074232	addons.xpi	DEBUG	Cancelling download of https://www.ducksong.com/misc/brotli1.xpi
1466542074233	addons.xpi	DEBUG	removeTemporaryFile: https://www.ducksong.com/misc/brotli1.xpi does not own temp file

is there a way to get more info on what the problem is?

I'm using 45.0b10 (a beta channel build) with xpinstall.signatures.required set to false
Flags: needinfo?(spohl.mozilla.bugs)
oh crap - docs say aurora or nightly for signatures off. let me recheck.
Flags: needinfo?(spohl.mozilla.bugs)
same problem - 45.0a2
Flags: needinfo?(spohl.mozilla.bugs)
I'm assuming you changed the checksum, size and version info in the xml file to match your .xpi?
Flags: needinfo?(spohl.mozilla.bugs)
Attached patch TP2 (obsolete) — Splinter Review
Attachment #8763937 - Attachment is obsolete: true
Attachment #8763937 - Flags: review?(standard8)
(In reply to Stephen A Pohl [:spohl] from comment #33)
> I'm assuming you changed the checksum, size and version info in the xml file
> to match your .xpi?

If you're avail on irc to drive this thing home, I'm mcmanus - can't find you.

so I'm actually just trying to give it the xpi directly. It seems to take a verrrrry long time on aurora for it to fetch the update.xml. Is that a problem?
Ben, I thought maybe you'd have some insight here..

as you can see - I'm having some trouble testing this system addon.

1466545660609	addons.xpi	INFO	Starting system add-on update check from https://www.ducksong.com/misc/updateurl.xml.
1466545660609	addons.productaddons	INFO	sending request to: https://www.ducksong.com/misc/updateurl.xml
1466545660765	addons.productaddons	INFO	Completed downloading document
1466545660777	addons.productaddons	INFO	Downloading from https://www.ducksong.com/misc/brotli1.xpi to /tmp/tmpaddon
1466545660833	addons.xpi	WARN	System add-on brotli@mozilla.org isn't compatible with the application.

this is with 45.0a2 and xpinstall.signatures.required set to false

does it just have to be signed to be tested?
Flags: needinfo?(felipc)
Flags: needinfo?(bhearsum)
The addon returns false for both [1] and [2]. I've confirmed that the .xpi gets installed properly if we make it past those two returns. I have not had time yet to figure out exactly why we return early, but signing seems to be involved.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#706
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#718
Flags: needinfo?(felipc)
Flags: needinfo?(bhearsum)
(In reply to Stephen A Pohl [:spohl] from comment #37)
> The addon returns false

s/addon/XPIProvider/
A few comments:

- System add-ons automatically get uninstalled on the next Firefox upgrade where there's a version bump. You can't rely on it being there unless you're going to offer it (and have it by default in the offer versions as well).

I would strongly suggest seeing if you can make this just compatible with 45.0.2.

I'm not sure if uninstall will get called on Firefox upgrade, Rob Helmer might know, but this would be a better way of sorting out the preferences.

- I'm not sure if the signature preference will disable the signing requirement for system add-ons. So you'd probably need to get it signed fully to test it. Though Rob might know as well.
Flags: needinfo?(rhelmer)
Attached patch brotli-fixupSplinter Review
Attachment #8764049 - Attachment is obsolete: true
(In reply to Stephen A Pohl [:spohl] from comment #29)
> The two other questions I had:
> 1. Should this addon be uninstalled at some point? If yes, how is this done
> in the system addons world?

it can be uninstalled any point an impacted user has moved on from 45.

> 2. Would this addon benefit from telemetry?

I don't think we would learn anything we don't know from our existing version data.

> 
> I'm not sure if uninstall will get called on Firefox upgrade, Rob Helmer
> might know, but this would be a better way of sorting out the preferences.

I've added a uninstall path to the addon re-enable things on upgrade. I'm eager to test that but can't seem to do so without a signature. If for some reason the population that received this system addon did manage to upgrade (and unistall the addon) without reverting the preference changes it wouldn't be the end of the world.

> 
> - I'm not sure if the signature preference will disable the signing
> requirement for system add-ons. So you'd probably need to get it signed
> fully to test it. Though Rob might know as well.

I guess I'm blocked on Rob then... thanks!

Rob: an xpi is available at https://www.ducksong.com/misc/brotli1.xpi
(In reply to Patrick McManus [:mcmanus] from comment #36)
> Ben, I thought maybe you'd have some insight here..
> 
> as you can see - I'm having some trouble testing this system addon.
> 
> 1466545660609	addons.xpi	INFO	Starting system add-on update check from
> https://www.ducksong.com/misc/updateurl.xml.
> 1466545660609	addons.productaddons	INFO	sending request to:
> https://www.ducksong.com/misc/updateurl.xml
> 1466545660765	addons.productaddons	INFO	Completed downloading document
> 1466545660777	addons.productaddons	INFO	Downloading from
> https://www.ducksong.com/misc/brotli1.xpi to /tmp/tmpaddon
> 1466545660833	addons.xpi	WARN	System add-on brotli@mozilla.org isn't
> compatible with the application.
> 
> this is with 45.0a2 and xpinstall.signatures.required set to false
> 
> does it just have to be signed to be tested?

I'm not terribly familiar with the guts of client side, but it looks https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#699 ends up returning false, which causes https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#8248 to spit out that error. Hard to pin it down past that, perhaps "*" is not a valid maxVersion though? If not that, I'd guess that it's failing in the second or third block of that function. Rob Helmer is probably who you need, though.
(In reply to Ben Hearsum (:bhearsum) from comment #42)
> I'm not terribly familiar with the guts of client side, but it looks

I was trying to save you time by clearing the n-i request. ;-) The answers are in comment 37. I built locally to confirm.
I'll help take a look here using the XPI from comment 41. 

Turning on debug logging in the addons manager can sometimes help: extensions.logging.enabled

The compatibility check can fail for a few reasons, we may need to improve the logging here so it's clear.
Flags: needinfo?(rhelmer)
(In reply to Mark Banner (:standard8) from comment #39)
> - I'm not sure if the signature preference will disable the signing
> requirement for system add-ons. So you'd probably need to get it signed
> fully to test it. Though Rob might know as well.

System add-ons should honor the same signature pref.
Is the reason 45.0a1 being tested because of the signing pref (we had messaged in the past that it was going to be removed for release+beta)? We still have not shipped a release that does not honor that pref, so we should be able to use the actual 45.0 release to test here.

If there's something I am missing here please let me know! I am going to test on 45.0 assuming the above is correct.
right - 45.0a2 was because I thought the pref was nightly/aurora only.. would prefer to test on release 45.0.2

thanks!
(In reply to Robert Helmer [:rhelmer] from comment #45)
> (In reply to Mark Banner (:standard8) from comment #39)
> > - I'm not sure if the signature preference will disable the signing
> > requirement for system add-ons. So you'd probably need to get it signed
> > fully to test it. Though Rob might know as well.
> 
> System add-ons should honor the same signature pref.

OK sorry I am wrong ^ we do require these to be signed and don't honor the pref:

https://dxr.mozilla.org/mozilla-central/rev/51377a64158941f89ed73f388ae437cfa494c030/toolkit/mozapps/extensions/internal/XPIProvider.jsm#704

SIGNEDSTATE_SYSTEM means that the add-on is signed with the "Mozilla Components" OU, and it isn't optional when installed via this mechanism.

If I disable that in my local build, then I am able to install the brotli add-on in 45.0 just fine.

In the meantime I think you'll need to get this signed to test the delivery method. Sorry for the confusion!

If you want to test the functionality of the add-on, you can do that by disabling signature check and installing it normally (e.g. via about:addons or clicking on a XPI link).

I am going to file a bug to add logging for each of the cases that the isUsableAddon function can fail, so this is actually debuggable in the future.
(In reply to Robert Helmer [:rhelmer] from comment #48)
> I am going to file a bug to add logging for each of the cases that the
> isUsableAddon function can fail, so this is actually debuggable in the
> future.

Bug 1281547
Thanks Robert!
> In the meantime I think you'll need to get this signed to test the delivery
> method. Sorry for the confusion!
> 

Ben, are you the person I see to get this signed to verify it (and then talk to about deployment, presumably.. but one thing at a time :))?
Flags: needinfo?(bhearsum)
(In reply to Patrick McManus [:mcmanus] from comment #51)
> > In the meantime I think you'll need to get this signed to test the delivery
> > method. Sorry for the confusion!
> > 
> 
> Ben, are you the person I see to get this signed to verify it (and then talk
> to about deployment, presumably.. but one thing at a time :))?

I don't think so; it looks like CloudOps holds the cert that they need to be signed by. Here's an example bug where Hello was signed: https://bugzilla.mozilla.org/show_bug.cgi?id=1256671.
Flags: needinfo?(bhearsum)
Depends on: 1281600
(In reply to Ben Hearsum (:bhearsum) from comment #52)
> 
> I don't think so; it looks like CloudOps holds the cert that they need to be
> signed by. Here's an example bug where Hello was signed:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1256671.

Thank you.. I'll be back when I'm ready to figure out how to deploy :)
(In reply to Patrick McManus [:mcmanus] from comment #53)
> (In reply to Ben Hearsum (:bhearsum) from comment #52)
> > 
> > I don't think so; it looks like CloudOps holds the cert that they need to be
> > signed by. Here's an example bug where Hello was signed:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1256671.
> 
> Thank you.. I'll be back when I'm ready to figure out how to deploy :)

You should be able to file a bug in "Cloud Services :: Operations" to do one-off uploads of the XPI.
They'll want to upload to: https://ftp.mozilla.org/pub/system-addons/

The final step is to configure the update service (AUS aka Balrog) to point to this, bhearsum should be able to help with that bit.

Also just for future reference we want to automate the signing+uploading bits of this process in bug 1281571 and bug 1281578 respectively but unfortunately probably won't be in time for you to use it for this fix. Next time!
alright I can confirm that

https://bugzilla.mozilla.org/attachment.cgi?id=8764381
from 
https://bugzilla.mozilla.org/show_bug.cgi?id=1281600

is a signed version of this XPI that tests fine for me as a system addon in 45.0.2... i.e. I update the system addon url to point at xml that references it and the preference gets updated after a little while.. and updating to 47 from the auto updater reverts the pref.
Depends on: 1281842
ploy :)
> 
> You should be able to file a bug in "Cloud Services :: Operations" to do
> one-off uploads of the XPI.
> They'll want to upload to: https://ftp.mozilla.org/pub/system-addons/
> 

https://bugzilla.mozilla.org/show_bug.cgi?id=1281842
(In reply to Patrick McManus [:mcmanus] from comment #53)
> (In reply to Ben Hearsum (:bhearsum) from comment #52)
> > 
> > I don't think so; it looks like CloudOps holds the cert that they need to be
> > signed by. Here's an example bug where Hello was signed:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1256671.
> 
> Thank you.. I'll be back when I'm ready to figure out how to deploy :)

alright - the signed system add on is available here
https://ftp.mozilla.org/pub/system-addons/brotli/brotli@mozilla.org-1.0.0-ff45.xpi

ben, can you help me take it from here to get it deployed to our remaining 45's?
Flags: needinfo?(bhearsum)
We also need to ensure that this gets tracked for any upcoming updates to ESR I believe, so that any updates that go out don't wipe out the change that this system addon made. Or am I wrong on that?
(In reply to Johnny Stenback  (:jst, jst@mozilla.com) from comment #58)
> We also need to ensure that this gets tracked for any upcoming updates to
> ESR I believe, so that any updates that go out don't wipe out the change
> that this system addon made. Or am I wrong on that?

The ESR channel has brotli disabled in its prefs - which is essentially what the addon does.. so it should be ok to avoid shipping this to ESR and tracking it.
(In reply to Patrick McManus [:mcmanus] from comment #57)
> (In reply to Patrick McManus [:mcmanus] from comment #53)
> > (In reply to Ben Hearsum (:bhearsum) from comment #52)
> > > 
> > > I don't think so; it looks like CloudOps holds the cert that they need to be
> > > signed by. Here's an example bug where Hello was signed:
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=1256671.
> > 
> > Thank you.. I'll be back when I'm ready to figure out how to deploy :)
> 
> alright - the signed system add on is available here
> https://ftp.mozilla.org/pub/system-addons/brotli/brotli@mozilla.org-1.0.0-
> ff45.xpi
> 
> ben, can you help me take it from here to get it deployed to our remaining
> 45's?

I'm not sure I'm the best person to help at this point, actually. Mark, I think you're the person who understands the state of the state of the rules best - can you help out here?
Flags: needinfo?(bhearsum) → needinfo?(standard8)
Patrick, did you ever get a review for the code in this addon? If not, I think it would be good to get a review from Felipe (or someone that he recommends). I'm not able to r+ system addons myself.
Flags: needinfo?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #59)
> (In reply to Johnny Stenback  (:jst, jst@mozilla.com) from comment #58)
> > We also need to ensure that this gets tracked for any upcoming updates to
> > ESR I believe, so that any updates that go out don't wipe out the change
> > that this system addon made. Or am I wrong on that?
> 
> The ESR channel has brotli disabled in its prefs - which is essentially what
> the addon does.. so it should be ok to avoid shipping this to ESR and
> tracking it.

Excellen, thanks!
I believe you'll also want this system addon to live in the mozilla GitHub repo somewhere. As an example, the system addon for bug 1280378 will live here: https://github.com/mozilla/outofdate-notifications-system-addon
(In reply to Stephen A Pohl [:spohl] from comment #63)
> I believe you'll also want this system addon to live in the mozilla GitHub
> repo somewhere. As an example, the system addon for bug 1280378 will live
> here: https://github.com/mozilla/outofdate-notifications-system-addon

Given this is a one-off add-on, I think leaving it in these bugs is enough.
(In reply to Ben Hearsum (:bhearsum) from comment #60)
> (In reply to Patrick McManus [:mcmanus] from comment #57)
> > ben, can you help me take it from here to get it deployed to our remaining
> > 45's?
> 
> I'm not sure I'm the best person to help at this point, actually. Mark, I
> think you're the person who understands the state of the state of the rules
> best - can you help out here?

So first of all, you'll need to co-ordinate with release-drivers and for QA testing & release dates. Since this is 45, it shouldn't interfere with too much else, but we should check anyway. 

To do this, the best way is probably to send an intent to ship email to release drivers. See https://docs.google.com/document/d/1x27I7hAmWDWiqk3o3YC3fklhE3N59bdgHCQHF5p_lkU/edit#heading=h.x1jyf4e7lno7 for background.

Explain that it is for 45 only. We'll also be able to do release channel only as well (i.e. not esr).

Once there's an ok, I can help you get Balrog setup (the update system) on a test channel, and then we can get QA to verify it. Then ship it.

It would also be good to get a clear statement on this bug of what QA should test before shipping.
Flags: needinfo?(standard8)
thanks everyone for the help. especially mark - I have a meeting filled week but I'll try and catch up. We do need the review - I was waiting for felipe to come back from pto so 'll do that now as the next step.
Flags: needinfo?(mcmanus)
Attachment #8764231 - Flags: review?(felipc)
hey felipe - I know there is a big comment trail here - feel free to reach out to me with any questions on how this has gone since the work week.

There is a signed version of this suitable for QA if this passes muster. It passes my local tests.
Flags: needinfo?(felipc)
Comment on attachment 8764231 [details] [diff] [review]
brotli-fixup

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

Code looks good!

::: browser/extensions/brotli/install.rdf.in
@@ +17,5 @@
> +    <em:targetApplication>
> +      <Description>
> +        <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> +        <em:minVersion>45.0a1</em:minVersion>
> +        <em:maxVersion>*</em:maxVersion>

We try to avoid infinite things like this, so how about setting this to 47.* just to have a reasonable value?
Attachment #8764231 - Flags: review?(felipc) → review+
I've read through the comments and it looks like everything is understood now.. I'll clear the needinfo and if I missed something please re-add the needinfo and I'll look again
Flags: needinfo?(felipc)
Hey guys -- so we actually figured out a workaround to not launch this for ff45 and so we're currently sending brotli to all ff46+ users. Somebody internally reported that they are using FF44 on Ubuntu and it seems like botli is breaking FF for them. Is there any chance this addon also needs to be deployed for FF44 users?
The current version in Ubuntu's repo is 47. Is that internal user who's seeing brotli in 44 a single isolated case or do you have evidence of there being lots of users who are on Ubuntu with Firefox 44?
hey ben -  i guess the obvious question is why your 45 workaround doesn't work for 44 the ubuntu 44 user too? Or maybe I misunderstand..

I suspect we wouldn't ship the addon at all, with the current release being 47, if FB would be unimpacted while that population drifts to ~0.

If you think we should still ship it then yes, its easy enough to add in 46 (which is the earliest version with the feature) - OS platform shouldn't be relevant.
So it looks like right now of all Firefox users using FB:

1.1% have FF 44 and send brotli
1.7% have FF 45 and send brotli

For FF 44 (both with and without brotli) users heres a breakdown by OS/Accept Encoding

Hits ↑ 	OS	Accept Encoding
44.30%	Linux	deflate,gzip
36.00%	Linux	*
14.80%	Windows	br,deflate,gzip
2.20%	Windows 10	br,deflate,gzip
1.60%	Linux	br,deflate,gzip
0.50%	Mac OS X	br,deflate,gzip

Same for FF45
Hits ↑ 	OS	Accept Encoding
55.20%	Windows	br,deflate,gzip
20.00%	Windows	deflate,gzip
8.00%	Windows 10	br,deflate,gzip
7.40%	Linux	br,deflate,gzip

So I think this is affecting more than just internal users. IDK why Linux shows up so high for FF44 and with Accept-Encoding = *. It's possible that is some sort of spoofed user agent.

In any case I think the population on both FF44 and FF45 is high enough to justify deploying the addon.
both of those non-br linux 44's are probably some kind of fork - so there's a good chance they don't take hotfixes from our servers either. I don't know how we would determine that other than experimentally after deploying. hard to tell - but they're not getting brotli anyhow I would hope. (* can't possibly mean *, that's not sensical.)

anyhow - I don't think it will be any harder/easier to get this approved for 44,45 than for just 45. (though I do have to change it and get it resigned and uploaded :( )

Can you tell me more about the workaround you have in place - that's what I really want to assess. Whether the workaround is sufficient for this problem or the addon is required too. Thanks.

Oh - and in some other bug we should talk about immutable!
The workaround we have in place is that we've hardcoded our internal CDN to look at the UA of the request and not serve Brotli to FF45 (by stripping the br encoding from any such UA). We can extend this to ff44, but I'd really like to get as much of a workaround as possible into firefox itself. Because we control the code for our CDN we're able to do stuff like this, but most people don't control their CDN and wouldn't be able to apply this type of fix. I really worry that the number of FF users who are affected will delay the widespread adoption of brotli.
Depends on: 1284976
Depends on: 1284997
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.