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

VERIFIED FIXED in Firefox 38.0.5

Status

()

P2
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: gcp, Assigned: Gijs)

Tracking

unspecified
Firefox 41
x86
Windows
Points:
2
Bug Flags:
firefox-backlog +
in-testsuite -

Firefox Tracking Flags

(firefox38.0.5+ fixed, firefox39 verified, firefox40 verified, firefox41 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Observed with recently Nightlies, once installing in Windows 7, once in Windows 8.1
(Reporter)

Comment 2

4 years ago
Reproduced again, without step 3. Should be trivial to verify now.
(Assignee)

Updated

4 years ago
Blocks: 1164302
(Assignee)

Comment 3

4 years ago
Created attachment 8608686 [details]
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/
Attachment #8608686 - Flags: review?(jaws)
(Assignee)

Comment 4

4 years ago
I don't understand why QA didn't see this.

Also, ffffffffffff.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 41.1 - May 25
Points: --- → 2
Flags: qe-verify+
Flags: in-testsuite-
Flags: firefox-backlog+
(Assignee)

Comment 5

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

Updated

4 years ago
Summary: Pocket Icon reappears in the toolbar after removal. → Pocket Icon reappears in the toolbar after removal on a clean profile (or at least one with no saved state)
status-firefox38.0.5: --- → affected
status-firefox39: --- → affected
status-firefox40: --- → affected
status-firefox41: --- → affected
tracking-firefox38.0.5: --- → +
(Assignee)

Comment 8

4 years ago
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
Attachment #8608686 - Flags: approval-mozilla-release?
Attachment #8608686 - Flags: approval-mozilla-beta?
Attachment #8608686 - Flags: approval-mozilla-aurora?
(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?
(Assignee)

Comment 10

4 years ago
(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?
Flags: needinfo?(sledru)
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
Flags: needinfo?(sledru)
Attachment #8608686 - Flags: approval-mozilla-release?
Attachment #8608686 - Flags: approval-mozilla-release+
Attachment #8608686 - Flags: approval-mozilla-beta?
Attachment #8608686 - Flags: approval-mozilla-beta+
Attachment #8608686 - Flags: approval-mozilla-aurora?
Attachment #8608686 - Flags: approval-mozilla-aurora+
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/f5a31d211e41
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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.
status-firefox39: fixed → verified
Removing qe-verify+ since verification on Firefox 39 Beta should suffice.
Flags: qe-verify+
(Assignee)

Comment 18

4 years ago
Comment on attachment 8608686 [details]
MozReview Request: bz://1167096/Gijs
Attachment #8608686 - Attachment is obsolete: true
Attachment #8620344 - Flags: review+
(Assignee)

Comment 19

4 years ago
Created attachment 8620344 [details]
MozReview Request: Bug 1167096 - flip introductory prefs if there's no saved state, r?jaws
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]

Updated

3 years ago
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
status-firefox41: fixed → verified
You need to log in before you can comment on or make changes to this bug.