Closed Bug 1126559 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox :: Tours, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox35 --- affected
firefox36 --- affected
firefox37 --- affected
firefox38 --- affected
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: MattN, Assigned: Gijs)

References

()

Details

Attachments

(1 file)

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
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
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
Duplicate of this bug: 1269669
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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
Attachment #8753894 - Flags: review?(MattN+bmo) → review+
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.
Flags: needinfo?(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.
Flags: needinfo?(agibson) → needinfo?(MattN+bmo)
(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.
Flags: needinfo?(MattN+bmo)
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
Attachment #8753894 - Flags: approval-mozilla-beta?
Attachment #8753894 - Flags: approval-mozilla-aurora?
(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!
Filed Bug 1274207 for bedrock.
Filed Bug 1274210 for SUMO.
https://hg.mozilla.org/mozilla-central/rev/957edd85c736
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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.
Attachment #8753894 - Flags: approval-mozilla-beta?
Attachment #8753894 - Flags: approval-mozilla-beta-
Attachment #8753894 - Flags: approval-mozilla-aurora?
Attachment #8753894 - Flags: approval-mozilla-aurora+
Relanded with a test change (tested locally, should pass on aurora now).

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/a6138313bed9
Flags: needinfo?(gijskruitbosch+bugs)
(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
You need to log in before you can comment on or make changes to this bug.