1475974 - The "New Windows and Tabs" section is only updated after you open a new window if the "Restore Defaults" option is used

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[Ciprian Muresan [:cmuresan], Ecosystem QA]

Attachment: restore defaults wrongly defaults to blank page.gif

[Affected versions]:
- Nightly v63.0a1, Build ID 20180715220110

[Affected Platforms]:
- All Windows
- All Mac
- All Linux

[Steps to reproduce]:
1. Open the browser with a clean new profile.
2. Navigate to about:preferences#home and set the "Homepage and new windows" option to "Blank Page".
3. Click the "Restore Defaults" button and observe the "Homepage and new windows" option.
4. Open a New Window and navigate to the about:preferences#home page.
5. Set the "Homepage and new windows" option to "Custom URLs..." and enter a valid URL.
6. Open a New Window and navigate to about:preferences#home.
7. Click the "Restore Defaults" button and observe the "Homepage and new windows" option.

[Expected results]:
- Step 3 & 6: The option resets to "Firefox Home (Default)".

[Actual results]:
- Step 3 & 6: The option is set to "Blank Page".

[Regression window]:
 5:51.04 INFO: Last good revision: c17b3006ad3203c5ded7aaec0e6dfe1e073c3afe
 5:51.04 INFO: First bad revision: bb08bc2f29568bb81840e45115107dd2fcd22c67
 5:51.04 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c17b3006ad3203c5ded7aaec0e6dfe1e073c3afe&tochange=bb08bc2f29568bb81840e45115107dd2fcd22c67

It looks like Bug 721211 has caused this behavior.

[Notes]:
- The setting is correctly displayed only after you open a new window.
- Attached a screen recording of the issue.

Comment 1

[Ciprian Muresan [:cmuresan], Ecosystem QA]

@Axel, it looks like bug 721211 has caused this behavior, could you please take a look at this?

Comment 2

[Axel Hecht [:Pike]]

Some clarification on the STR:

Do you have three windows open with the pref#home tab loaded?

Comment 3

[Ciprian Muresan [:cmuresan], Ecosystem QA]

By the time you finish the STR, yes, you have 3 different windows with about:preferences#home. But you can also use just one instead of navigating to a new one in each new window.

Adding that it's enough to change the homepage from "Firefox Home" to "Blank Page" and click the "Restore Defaults" button to reproduce the issue. Or set a custom URL and click the "Restore Defaults" button.

Comment 4

[Axel Hecht [:Pike]]

I'll take a stab at it, but I'm not sure how to actually test this.

Comment 5

[Axel Hecht [:Pike]]

Attachment: bug 1475974, preferences should treat homepage as string pref,

This is a follow-up to bug 721211.

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

Comment 6

[Axel Hecht [:Pike]]

Pushed this to try.

Jaws, I've got no idea how to test this.

My local tests reproduced the problem, and then I changed the pref handling from wstring to string, which makes sense.

I also tested this locally with a localized pref hacked in, and things still seemed to work.

Having some automated tests for this would of course be very beneficial.

Ciprian, thanks for finding this.

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

Tracking for 63

Comment 8

[Pascal Chevrel:pascalc (PTO until April 26)]

for real this time

Comment 9

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

Comment on attachment 8992372 [details]
bug 1475974, preferences should treat homepage as string pref,

https://reviewboard.mozilla.org/r/257230/#review264720

It looks like bug 1472599 is fixing part of this problem too but both patches can be used.

Comment 10

[Axel Hecht [:Pike]]

This fails tests.

Mark, I'm a bit lost as to what the code should do, could you adopt this? You wrote the initial part that this is breaking, but I'm not sure how deep this rat hole is.

Here's what's currently happening:

browser_extension_controlled.js tests that we can set the default prefs to explicit non-localizable URLs (yahoo.com in the test).

It then tests that that URL shows up in the edit box for the homepage.

Which it does right now because the code to get the defaultValue for a wstring pref tries to load a localized default pref, raises and exception and returns null. Which makes pref.value !== pref.defaultValue for a default pref.

Which makes us show the URL in the edit field.

I don't know what the right fix is here. Fix the test (or tests, only drilled in to one), fix the code, update the editbox for default values, convert more of the preference code to HomePage.jsm?

I guess that the behavior of the failing test and this regression are somewhat related, too.

Full disclosure, I shouldn't spend a lot more time on this. As a stop gap, we could also back out bug 721211.

Comment 11

[Ciprian Muresan [:cmuresan], Ecosystem QA]

It looks like this scenario was fixed by bug 1472599. 
Jared, should we consider this issue as resolved fixed? Or is there more work to be done?

Comment 12

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

The problem won't be visible after bug 1472599 but Axel has some good questions here that we should still answer so let's keep this open.

Comment 13

[Mark Striemer [:mstriemer]]

I'm not sure what the questions are here now that this bug is fixed. Are we still wanting the change attached in this patch and need to get the tests passing?

I see that if there's an extension controlling the homepage Restore Defaults is disabled instead of allowing it to be used to reset the Activity Stream settings, although maybe those should be disabled if there's an add-on controlling the new tab, too.

Should there be a more specific bug filed instead of this?

Comment 14

[Axel Hecht [:Pike]]

It might be good to have a more on-purpose bug, yeah.

But filing that would probably be best done once we know what to actually do. Nothing I can really help with.

Jared?

As the functionality is restored, I'm re-triaging this to P3, and un-assigning myself.

Comment 15

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

We should just close this bug.

Mark, could you file a bug for the situation you're describing here?
> I see that if there's an extension controlling the homepage Restore Defaults
> is disabled instead of allowing it to be used to reset the Activity Stream
> settings, although maybe those should be disabled if there's an add-on
> controlling the new tab, too.

Comment 16

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 63.0a1 (2018-07-16) on Windows 10, 64 Bit!
This bug's fix is verified with latest Nightly!

Build ID 	20180811220142
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Comment 17

[Ciprian Muresan [:cmuresan], Ecosystem QA]

Marking this as Verified - Fixed based on comment 16 and bug 1472599.

Comment 18

[Justin Dolske [:Dolske]]

Fixed in 63, so clearing tracking.

Comment 19

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

(In reply to Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #15)

We should just close this bug.

Mark, could you file a bug for the situation you're describing here?

I see that if there's an extension controlling the homepage Restore Defaults
is disabled instead of allowing it to be used to reset the Activity Stream
settings, although maybe those should be disabled if there's an add-on
controlling the new tab, too.

Filed bug 1526950.