1126559 - UITour is able to show the refresh Firefox dialog even when reset isn't supported

Status: RESOLVED FIXED

(Firefox :: Tours, defect)

Description

[Matthew N. [:MattN]]

UITour.jsm isn't checking ResetProfile.resetSupported and doesn't let the page know when a refresh is supported so it can avoid showing UI to show the refresh dialog.

This is probably not very common but one user hit it from the SUMO refresh page: https://support.mozilla.org/en-US/questions/1043695

Comment 1

[Alex Gibson [:agibson]]

Some additional info in the case of mozorg - we're now also showing a refresh button for up-to-fate users on https://www.mozilla.org/firefox/new/. In this case, we're first calling getConfiguration('appInfo') to make sure the user is on release channel with the most recent version before showing the button [1]. I'm assuming this is enough to avoid the likelihood of users hitting this bug also.

[1] https://github.com/mozilla/bedrock/blob/master/media/js/firefox/new.js#L69

Comment 3

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1126559 - don't allow resetting when that is not possible, r?MattN

Review commit: https://reviewboard.mozilla.org/r/53546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53546/

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8753894 [details]
MozReview Request: Bug 1126559 - don't allow resetting when that is not possible, r?MattN

https://reviewboard.mozilla.org/r/53546/#review50528

Thanks

Comment 5

[Matthew N. [:MattN]]

Alex, can you have mozorg not show the refresh button if the version is 49.0a1 or higher and getConfiguration("canReset") is false?

I'm not sure who can make the SUMO change.

Comment 6

[Alex Gibson [:agibson]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #5)
> Alex, can you have mozorg not show the refresh button if the version is
> 49.0a1 or higher and getConfiguration("canReset") is false?
> 
> I'm not sure who can make the SUMO change.

Sure, but on mozorg we're already calling getConfiguration('appinfo') to make sure the user is on release channel before showing the button. In that case, is calling 'canReset' still needed? Just trying to understand if there are edge cases I'm not aware of where reset might not be supported still.

I can see about filing a SUMO bug to get their page fixed.

Comment 7

[:Gijs (he/him)]

(In reply to Alex Gibson [:agibson] from comment #6)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #5)
> > Alex, can you have mozorg not show the refresh button if the version is
> > 49.0a1 or higher and getConfiguration("canReset") is false?
> > 
> > I'm not sure who can make the SUMO change.
> 
> Sure, but on mozorg we're already calling getConfiguration('appinfo') to
> make sure the user is on release channel before showing the button. In that
> case, is calling 'canReset' still needed? Just trying to understand if there
> are edge cases I'm not aware of where reset might not be supported still.

Yes - if a user starts with particular commandline options to use profiles stored in non-default directories, or starts with a non-default profile (on 48 and below), we can't (couldn't) always reset the browser. I'll see about uplifting this because of that constraint.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8753894 [details]
MozReview Request: Bug 1126559 - don't allow resetting when that is not possible, r?MattN

Approval Request Comment
[Feature/regressing bug #]: Firefox refresh
[User impact if declined]: users can be offered refresh when it won't work properly
[Describe test coverage new/current, TreeHerder]: additional test coverage is part of the patch
[Risks and why]: low risk, minor change specific to UITour
[String/UUID change made/needed]: nope

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/957edd85c736

Comment 10

[Alex Gibson [:agibson]]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Alex Gibson [:agibson] from comment #6)
> > (In reply to Matthew N. [:MattN] (behind on reviews) from comment #5)
> > > Alex, can you have mozorg not show the refresh button if the version is
> > > 49.0a1 or higher and getConfiguration("canReset") is false?
> > > 
> > > I'm not sure who can make the SUMO change.
> > 
> > Sure, but on mozorg we're already calling getConfiguration('appinfo') to
> > make sure the user is on release channel before showing the button. In that
> > case, is calling 'canReset' still needed? Just trying to understand if there
> > are edge cases I'm not aware of where reset might not be supported still.
> 
> Yes - if a user starts with particular commandline options to use profiles
> stored in non-default directories, or starts with a non-default profile (on
> 48 and below), we can't (couldn't) always reset the browser. I'll see about
> uplifting this because of that constraint.

Got it thanks, I'll file a mozorg bug so we can update as necessary. Thanks for the info!

Comment 11

[Alex Gibson [:agibson]]

Filed Bug 1274207 for bedrock.

Comment 12

[Alex Gibson [:agibson]]

Filed Bug 1274210 for SUMO.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/957edd85c736

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8753894 [details]
MozReview Request: Bug 1126559 - don't allow resetting when that is not possible, r?MattN

Let's uplift to Aurora48. Given how old this issue is, I would prefer to not uplift to Beta47 as we are a week away from build RC. 

At this point, I would like to only accept uplifts for critical recent regressions, severe stability, security, perf, mlk issues only. This is to minimize code churn and get ready to ship this thing in 2 weeks.

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

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

I had to back this out in https://hg.mozilla.org/releases/mozilla-aurora/rev/c40d4d7f4bb2 for breaking a test:
https://treeherder.mozilla.org/logviewer.html#?job_id=2647292&repo=mozilla-aurora

Comment 18

[:Gijs (he/him)]

Relanded with a test change (tested locally, should pass on aurora now).

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/a6138313bed9

Comment 19

[Carsten Book [:Tomcat]]

(In reply to :Gijs Kruitbosch from comment #18)
> Relanded with a test change (tested locally, should pass on aurora now).
> 
> remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/a6138313bed9

setting flags