1113163 - Loop throttle in fx35 appears not to work -- loop button always appears

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

Setting this in /etc/hosts:
127.0.0.0 soft-start.loop.services.mozilla.com
This shows 0% using the throttling extension check (when I create a new profile using Fx35) after setting the hosts file).

The Loop/Hello icon appears in the URLbar as soon as the new profile is created.
There is no loop.soft_start_ticket_number pref in about:config
loop.throttled2 is false (with the above /etc/hosts).

See also bug 1113138

Comment 1

[Adam Roach [:abr]]

The patch that landed this functionality is in Bug 1073215

Comment 2

[Maire Reavy [:mreavy]]

Assigning to Mike for investigation

Comment 3

[Mike de Boer [:mikedeboer]]

So, what's going on here is that in Fx35 we're specifying loop-button-throttled to be in the navbar _by default_ for a fresh profile or when the user clicks the 'Restore Defaults' button in Customize Mode, regardless of throttling being active.

Now for users that upgrade from 34 -> 35 and are using the profile default configuration (they did _not_ customize the navbar with one or more addons/ buttons), loop-button-throttled will also appear, regardless of throttling being active.

In other words: fresh profiles, new users, will _always_ see the loop button in the navbar from the first time they start up.


For existing Fx users who've installed at least one addon that placed itself in the toolbar once or customized the navbar layout, the loop-button-throttled will _not_ appear. Its visibility is entirely dependent on the throttling mechanism.


If this is not the desired flow, please comment on this bug and I'll see if it's technically feasible to implement (RDF is not my friend).

Comment 4

[Adam Roach [:abr]]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> So, what's going on here is that in Fx35 we're specifying
> loop-button-throttled to be in the navbar _by default_ for a fresh profile
> or when the user clicks the 'Restore Defaults' button in Customize Mode,
> regardless of throttling being active.
> 
> Now for users that upgrade from 34 -> 35 and are using the profile default
> configuration (they did _not_ customize the navbar with one or more addons/
> buttons), loop-button-throttled will also appear, regardless of throttling
> being active.
> 
> In other words: fresh profiles, new users, will _always_ see the loop button
> in the navbar from the first time they start up.
> 
> 
> For existing Fx users who've installed at least one addon that placed itself
> in the toolbar once or customized the navbar layout, the
> loop-button-throttled will _not_ appear. Its visibility is entirely
> dependent on the throttling mechanism.
> 
> 
> If this is not the desired flow, please comment on this bug and I'll see if
> it's technically feasible to implement (RDF is not my friend).

Okay, no, that's not what I expected. In particular, I was under the impression that the following code in CustomizableWidgets.jsm would keep the button in the customize palette until unthrottled:

> defaultArea: !Services.prefs.getBoolPref("loop.throttled2") && CustomizableUI.AREA_NAVBAR,

What should happen is:

 - Fresh profiles in 35 should see the button in the navbar if and only if
   they are unthrottled; it should otherwise be in the customize palette.

 - Users who have upgraded from 34 to 35 without having customized their
   UI should see the button in the navbar if and only if they are unthrottled;
   it should otherwise be in the customize palette.

 - Users who have upgraded from 34 to 35 and *have* customized their UI, but not
   in a way that moved the Loop button out of the customization palette, should
   see the button in the navbar if and only if they are unthrottled; it should
   otherwise be in the customize palette.

 - Users who have upgraded from 34 to 35 and *have* customized their UI in a way
   that *did* move the Loop button out of the customization palette should
   see the button exactly where they put it in 34.

I'm okay treating users who have moved the button to be visible and then put it
back in the customization palette as a rare corner case. They can exhibit any of the
behaviors described above.

Comment 5

[Adam Roach [:abr]]

To be clear: I called these out as four different cases even though the first three have the same user-visible behavior because I want to make sure that all four cases are tested, at least manually, before we consider this fixed.

Comment 6

[Maire Reavy [:mreavy]]

Hey Mike -- What would it take to implement the minimal change in order to satisfy Abr's requirements in Comment 4?  We will be uplifting this to Beta to control server load, and the server folks are counting on us to control the load hitting their system all at once.  

I'm checking the telemetry numbers that we have, but I believe relatively few users have either installed at least one addon that placed itself in the toolbar once or customized the navbar layout.  If I'm correct, then the majority of our users would see the Hello loop bar on release day (which could crush our servers).

So a safe, minimal patch is really important.   I'll make sure it gets well tested.

Comment 7

[Maire Reavy [:mreavy]]

It probably also makes sense to remove loop.throttled2 as an exposed variable -- though I realize anything touched in about:config "voids our warranty".

Comment 8

[Mike de Boer [:mikedeboer]]

I think I know what needs to be done here; should result in a very simple patch. More tomorrow morning.

Comment 9

[Marco Mucci [:MarcoM]]

Added to IT 37.2

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Patch V1: make sure the loop button doesn't get placed in the navbar before throttling is off

Gijs, I hope you're comfortable reviewing this.

I made the mistake to add 'loop-button-throttled' to the defaultset whereas that's actually bypassing the throttling mechanism. Not nice. This patch fixes that and QA will verify the scenarios Adam listed in comment 4.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8539193 [details] [diff] [review]
Patch V1: make sure the loop button doesn't get placed in the navbar before throttling is off

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

Assuming the code in comment #4 is the only thing that is meant to unthrottle people, this will break the following situation:

 - Fresh profiles in 35 should see the button in the navbar if [...]
   they are unthrottled


as the defaultArea property on the widget on its own is not enough here (see https://bugzilla.mozilla.org/show_bug.cgi?id=1031019 - it seems like gavin never did file a bug to investigate why defaultArea by itself isn't enough. :-(

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #11)
> Assuming the code in comment #4 is the only thing that is meant to
> unthrottle people, this will break the following situation:
> 
>  - Fresh profiles in 35 should see the button in the navbar if [...]
>    they are unthrottled
> 
> 
> as the defaultArea property on the widget on its own is not enough here (see
> https://bugzilla.mozilla.org/show_bug.cgi?id=1031019 - it seems like gavin
> never did file a bug to investigate why defaultArea by itself isn't enough.
> :-(

No the actual throttling/ unthrottling and putting the button on the navbar is managed here: http://mxr.mozilla.org/mozilla-beta/source/browser/base/content/browser.js#1284

Comment 13

[:Gijs (he/him)]

Comment on attachment 8539193 [details] [diff] [review]
Patch V1: make sure the loop button doesn't get placed in the navbar before throttling is off

r+ if and only if it's OK that the button will appear on unthrottling, but disappear again if you use restore defaults, even though you've been unthrottled, and the button won't be appearing again (despite being unthrottled) until we make more code changes that will likely require a new pref etc.

Caveat emptor.

Comment 14

[Maire Reavy [:mreavy]]

Hi Gijs -- When say, "if it's OK that the button will appear on unthrottling, but disappear again if you use restore defaults, even though you've been unthrottled, and the button won't be appearing again (despite being unthrottled) until we make more code changes that will likely require a new pref etc."   -- what will happen when it "disappears" after a restore to defaults?   Will the button go back to the palette such that the user will have to drag it back to the toolbar if he/she wants to use it?  (Or will something else happen?)

Comment 15

[:Gijs (he/him)]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #14)
> what will happen when it "disappears" after a restore
> to defaults?   Will the button go back to the palette such that the user
> will have to drag it back to the toolbar if he/she wants to use it?

Yes.

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Patch V2: make sure the loop button doesn't get placed in the navbar before throttling is off

Comment 17

[:Gijs (he/him)]

Comment on attachment 8539234 [details] [diff] [review]
Patch V2: make sure the loop button doesn't get placed in the navbar before throttling is off

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

Alright, let's do this. But let's QA the heck out of it. :-)

Also, can you file a followup bug to investigate getting rid of the defaultset attribute?

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8539234 [details] [diff] [review]
Patch V2: make sure the loop button doesn't get placed in the navbar before throttling is off

Approval Request Comment
[Feature/regressing bug #]: Loop
[User impact if declined]: The Loop/ Hello button will always appear for new and/ or un-customized Fx profiles. This is wrong and will potentially push too much load onto the Loop server.
[Describe test coverage new/current, TBPL]: None, this is Beta only.
[Risks and why]: minor
[String/UUID change made/needed]: n/a.

Pushed to try to verify this doesn't break Beta badly: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d2975dbb9a8a

Comment 19

[Mike de Boer [:mikedeboer]]

Try shows me green.

Comment 20

[Maire Reavy [:mreavy]]

Note: This patch can land on Fx35 ONLY due to how we're doing the throttling for Hello. (We will not have Hello throttled in Fx36 or Fx37.)

Comment 21

[Mike de Boer [:mikedeboer]]

Thanks!

Landed on beta as: https://hg.mozilla.org/releases/mozilla-beta/rev/a476f2afa8f9

Comment 22

[Maire Reavy [:mreavy]]

This fix was needed for Fx35 only, and it has landed on Fx35. So I'm marking this resolved/fixed.

Comment 23

[Paul Silaghi, QA [:pauly]]

Apparently, I don't see anything throttled in FF 35b8, new profile - the Hello icon is in the navbar and loop.throttled2=false.
Thoughts?

Comment 24

[Maire Reavy [:mreavy]]

Pauly, This is working as expected.  Nothing is throttled at the moment in release or on beta. We will turn the throttle down this week in prep for release next week.  That is when you can verify that the throttle is working -- unless you want to tweak the DNS results locally to simulate throttling.

Comment 25

[Paul Silaghi, QA [:pauly]]

Throttle works fine now on FF 35 RC due to bug 1118312.