1167096 - Pocket Icon reappears in the toolbar after removal on a clean profile (or at least one with no saved state)

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P2)

Description

[Gian-Carlo Pascutto [:gcp]]

STR:

1) Launch fresh Firefox installation, fresh profile.
2) Customize->Remove Pocket icon from toolbar.
3) Set up sync, change some preferences (I don't sync prefs), potentially get a Firefox update. <- Not sure which parts here are critical.
4) Relaunch Firefox

Expected behavior:
Customization stays, Pocket is gone.

Actual behavior:
Pocket reappears.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Observed with recently Nightlies, once installing in Windows 7, once in Windows 8.1

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Reproduced again, without step 3. Should be trivial to verify now.

Comment 3

[:Gijs (he/him)]

Attachment: MozReview Request: bz://1167096/Gijs

/r/9183 - Bug 1167096 - flip introductory prefs if there's no saved state, r?jaws

Pull down this commit:

hg pull -r 74f7a3070718c4a018bb43f69cfcd73968d26ccb https://reviewboard-hg.mozilla.org/gecko/

Comment 4

[:Gijs (he/him)]

I don't understand why QA didn't see this.

Also, ffffffffffff.

Comment 5

[:Gijs (he/him)]

(the good news being, if you remove it *a second time* it'll stay removed, even on current code)

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8608686 [details]
MozReview Request: bz://1167096/Gijs

https://reviewboard.mozilla.org/r/9181/#review7825

Ship It!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f5a31d211e41

Comment 8

[:Gijs (he/him)]

Comment on attachment 8608686 [details]
MozReview Request: bz://1167096/Gijs

Approval Request Comment
[Feature/regressing bug #]: new button introduction code / pocket feature
[User impact if declined]: new profiles will have trouble removing their pocket button
[Describe test coverage new/current, TreeHerder]: no :-(
[Risks and why]: medium-low. Change is specific to the "no saved state" (ie new profile / deleted prefs.js) case, but we all know that we're doing an RC build and that that's not a great time to land pretty much anything...
[String/UUID change made/needed]: nope

Comment 9

[Sylvestre Ledru [:Sylvestre]]

(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 8608686 [details]
> MozReview Request: bz://1167096/Gijs
> 
> Approval Request Comment
> [Feature/regressing bug #]: new button introduction code / pocket feature
> [User impact if declined]: new profiles will have trouble removing their
> pocket button
It is a sensitive feature. So, we should be careful with the user perception of this bug.


> [Describe test coverage new/current, TreeHerder]: no :-(
It is not the first time that we have a bug like this. We should have testing for these cases.


> [Risks and why]: medium-low. Change is specific to the "no saved state" (ie
> new profile / deleted prefs.js) case, but we all know that we're doing an RC
> build and that that's not a great time to land pretty much anything...
What is the worst case scenario here?

Comment 10

[:Gijs (he/him)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > Comment on attachment 8608686 [details]
> > MozReview Request: bz://1167096/Gijs
> > 
> > Approval Request Comment
> > [Feature/regressing bug #]: new button introduction code / pocket feature
> > [User impact if declined]: new profiles will have trouble removing their
> > pocket button
> It is a sensitive feature. So, we should be careful with the user perception
> of this bug.
> 
> 
> > [Describe test coverage new/current, TreeHerder]: no :-(
> It is not the first time that we have a bug like this. We should have
> testing for these cases.

There is testing for this feature in general (automatically inserting buttons) but it is not really easily possible to test all the different cases of profile use (or lack thereof) in an automated test, especially not when they require restarts, which is why the test did not catch this. The tests are already pretty hacky, I don't think it's worth adding another hack to simulate a new profile or a restart on something that clearly isn't a new profile.

> > [Risks and why]: medium-low. Change is specific to the "no saved state" (ie
> > new profile / deleted prefs.js) case, but we all know that we're doing an RC
> > build and that that's not a great time to land pretty much anything...
> What is the worst case scenario here?

I don't really know how to answer this question. I ran a build on a clean profile with this change (obviously) and the pocket button still shows up. The change only affects what happens when you start the browser with a clean profile (which then itself influences what happens if you start it again later and the profile is no longer "clean").

I expect that the worst case scenario is that the patch doesn't fix the bug or that the button disappears automatically on (second/third) startup. Those are easily testable, though, and I did, and didn't see this happening. Of course, it's possible my logic is wrong and/or my testing was deficient...

Does this help?

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8608686 [details]
MozReview Request: bz://1167096/Gijs

It does.
I think it is break in an obvious way, we will see it quickly.

Anyway, I am taking it because:
* pocket is a sensitive project and I would be sad to see on HN a thread "Firefox prevents you to remove the Pocket icon"
* I trust your judgment & skills
* Jaws reviewed it

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/3ef925962765

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c2ae95e45f9f

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3d87851ef903

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 16

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using Firefox 38.0.5 beta 1 build 2 on Windows 7 64-bit, verified that the issue is fixed using Firefox 39 beta 1 build 2 on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit.

Comment 17

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing qe-verify+ since verification on Firefox 39 Beta should suffice.

Comment 19

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1167096 - flip introductory prefs if there's no saved state, r?jaws

Comment 20

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 41.0a1 (Build ID: 20150521030204) on 
windows 8.1 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Nightly 43.0a1(Build ID: 20150908030203)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0

Verified as fixed with Firefox beta 41.0b8 (Build ID: 20150907144446)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0

Verified as fixed with Firefox aurora 42.0a2 (Build ID: 20150908004020)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0

[testday-20150904]