1524995 - Several custom settings for browser history remains checked and grayed out after selecting the “Always use private browsing mode” option

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Emil Ghitta, QA [:emilghitta]]

Attachment: preferences.gif

Affected versions

• Firefox 66.0b4 (BuildId:20190131191227).
• Firefox 67.0a1 (BuildId:20190204092937).

Unaffected versions

• Firefox 65.0 (BuildId:20190124174741).
• Firefox 60.5.0esr (BuildId:20190124141046).

Affected platforms

• Windows 10 64bit.
• macOS 10.11.6
• Ubuntu 16.04 64bit.

Steps to reproduce

1. Launch Firefox with a clean profile.
2. Go to about:preferences#privacy page.
3. Select the "Use custom settings for history" option from the History dropdown list.
4. Tick the "Always use private browsing mode" checkbox.
5. Click the “Restart Firefox now” button.
6. Go to about:preferences#privacy page.

Expected result

• The following options are unticked and grayed out:
  "Remember browsing and download history"
  "Remember search and form history"
  "Clear history when Nightly closes"

Actual result

• The following options are ticked and grayed out:
  "Remember browsing and download history"
  "Remember search and form history"

Regression range

• Last good revision: 33ca2ea6b37064d12c0d4fefc81bf689396a5e57

• First bad revision: 9ae7ae0acee4f8807cffaf64fef4f2e3a614b997

• Pushlog:
  https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=33ca2ea6b37064d12c0d4fefc81bf689396a5e57&tochange=9ae7ae0acee4f8807cffaf64fef4f2e3a614b997

• Potential regressor: Bug 1518252

Additional notes

• For further information regarding this issue, please observe the attached screencast.
• Please note that on Firefox 66.0b4, after performing step 6, you need to refresh the about:preferences#privacy page in order to reproduce this issue.

Comment 1

[Emil Ghitta, QA [:emilghitta]]

Hi Zibi,

It seems that mozregression pointed out Bug 1518252 for causing this regression.

Can you please have a look?

Thank you.

Comment 2

[Emil Ghitta, QA [:emilghitta]]

Attachment: preferences2.gif

Also, this issue seems "worse" than I initially thought.

After reproducing this issue, the user can uncheck the "Always use private browsing mode" without restating Firefox (because the prompt is not displayed) but the private browsing mode is preserved and the newly opened tabs are about:privatebrowsing tabs.

Attached a screencast for this.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

[Tracking Requested - why for this release]:

Looks like some of our Privacy and Security preferences in about:preferences can get into a confused state, and we should make sure these things always reflect reality.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 66, and I think marking this as a release blocker is reasonable. We probably shouldn't ship with these privacy related preferences set incorrectly.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

investigating

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Tim, jaws, I'm confused about this bug and start questioning if it has much to do with l10n. I'm wondering if it really is a result of bug 1520350, maybe in some correlation with bug 1518252 but unlikely.

What I see is that in this particular case, privacy.js#init is called, and updatePrivacyMicroControls in result of it, and console.log(document.getElementById("rememberHistory").checked) from inside of that function is set to false, but when I switch to that pane and call document.getElementById("rememberHistory").checked from the console, I see true.

There's no other place in the code that I could find that would touch this element, so I'm wondering if the lazy initialization somehow doesn't preserve the state, or doesn't call the updatePrivacyMicroControls after restoring it?

Could you take a look please?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I don't think I will get to it this week. Possibility weekend.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

One way to verify this quickly is to backout bug 1520350 locally and see if this is still reproducible.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

I reverted bug 1520350 and the bug is not reproducible. Seems like a regression from bug 1520350.

Comment 10

[:Gijs (he/him)]

Just like bug 1525099, 66 is affected, and bug 1520350 is not on beta.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

I can't reproduce the bug in 66b5 I downloaded from https://ftp.mozilla.org/pub/firefox/releases/66.0b5/linux-x86_64/en-US/

Comment 12

[Alice0775 White]

I can still reproduce the issue in comment#0 on 66.0b5 x64(en-US and ja build), Windows10.

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

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: debug.diff

This is the patch I'm using to debug the behavior.

With backing out of bug 1520350 I see:

1. Set the prefs to the testing state
2. Restart the browser
3. Open Preferences

privacy.js#init privacy.js:263:5
updatePrivacyMicroControls privacy.js:730:5
policy is custom privacy.js:738:7
disabled: true privacy.js:740:7
checked: false

and visually I see the rememberHistory unchecked as expected.

Then I reload the page with ctrl+r and I see the same:

privacy.js#init privacy.js:263:5
updatePrivacyMicroControls privacy.js:730:5
policy is custom privacy.js:738:7
disabled: true privacy.js:740:7
checked: false

but this time visually the rememberHistory is checked and when I type document.getElementById("rememberHistory").checked in the console I see true.

Can someone verify this experience?

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Also, when I see the bug reproduced visually, if I call gPrivacyPane.updatePrivacyMicroControls(); from the console it fixes the state.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

I verified that it's not affected by setting/removing data-l10n-id. It does however changes depending on the preference attribute.

This led me to my hypothesis that it's a change in race between the code that sets the state based on the preference attribute on an element and the updatePrivacyMicroControls called from init.

If I'm correct there's going to be a lot of red herrings because a lot of code can subtly tip the race one way or the other. I believe that bug 1518252 did just that.

Before bug 1518252, the order was likely that the preference attribute was reflected in the DOM, and then init/updatePrivacyMicroControls got called.
After it, the order reversed and now the preference attribute handler applies onto DOM after updatePrivacyMicroControls.

The observed result is that in cases where the JS code manually alters the UI element that has preference attribute as well, we may see a change.

I'm not sure what the right solution is here, because we can't remove the preference, but we need the updatePrivacyMicroControls to be called after it's handler got applied.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

I think I confirmed the hypothesis. In the error scenario, the order of operation is:

1. preferences.js initializes paneGeneral
2. which calls Preferences.updateAllElements
3. then it calls paneGeneral.init
4. then it fires preferencesBindings::onDOMContentLoaded
5. then it fires preferences.js#init_all
6. that, in order, calls Preferences.updateAllElements for each and every panel
7. and then it calls init for each and every panel

That already seems wrong, but what's also interesting is that due to a high number of Promises in the preferencesBindings, the panePrivacy calls (6) and then (7), but the preferences from (7) (panePrivacy.init) get applied first, and the privacyBindings.js#setElementValue for rememberHistory gets called after it.

That results in first init setting it to checked=false and then preferencesBindings#setElementValue called via:

• preferences.js#init_all
• PreferencesBindings.updateAllElements
• PreferencesBindings.setElementValue

which sets checked=true.

This seems like this is by design racy? I'm not sure how to disentangle it and I'm not sure how DocumentL10n.cpp change could have affect it except of tipping the race the other way. :(

Comment 17

[:Gijs (he/him)]

Attachment: Bug 1524995 - update privacy pane history mode dependent control checkboxes correctly in permanent private browsing mode, r?jaws

Depends on D19143

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b12e81ab8cf
update privacy pane history mode dependent control checkboxes correctly in permanent private browsing mode, r=jaws

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/7b12e81ab8cf

Comment 20

[:Gijs (he/him)]

(In reply to Emil Ghitta, QA [:emilghitta] from comment #2)

Created attachment 9041165 [details]
preferences2.gif

Also, this issue seems "worse" than I initially thought.

After reproducing this issue, the user can uncheck the "Always use private browsing mode" without restating Firefox (because the prompt is not displayed) but the private browsing mode is preserved and the newly opened tabs are about:privatebrowsing tabs.

I don't think this is the same issue (certainly it is not fixed by the patch that I attached); can you file a separate bug for this and find a regression window if indeed it is a regression?

Comment 21

[:Gijs (he/him)]

Comment on attachment 9042460 [details]
Bug 1524995 - update privacy pane history mode dependent control checkboxes correctly in permanent private browsing mode, r?jaws

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

bug 1520350, bug 1518252

User impact if declined

Confusing UI in the preferences/options

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

No

Needs manual test from QE?

Yes

If yes, steps to reproduce

See comment #0

List of other uplifts needed

n/a

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Very small, well-understood JS-only change to this specific bit of the preferences

String changes made/needed

no

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am a bit confused by this bug. Bug 1520350 didn't reach Fx66. Why do we need to uplift this one?

Comment 23

[:Gijs (he/him)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo) from comment #22)

I am a bit confused by this bug. Bug 1520350 didn't reach Fx66. Why do we need to uplift this one?

Because 66 is affected, and Gandalf removed bug 1518252 as a blocking bug even though it did trip the race condition that triggers this problem going the other way. bug 1520350 just tripped it in even more cases. See other comments on the bug (comment #0, comment #1, comment 12, etc.)

Comment 24

[:Gijs (he/him)]

I ended up filing bug 1526793 myself.

Comment 25

[Emil Ghitta, QA [:emilghitta]]

The issue mentioned in comment 0 is verified fixed using Firefox 67.0a1 (BuildId:20190210213844) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 18.04 64bit

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9042460 [details]
Bug 1524995 - update privacy pane history mode dependent control checkboxes correctly in permanent private browsing mode, r?jaws

Fix for confusing/inaccurate pref pane conditions.
Verified in nightly, ok for uplift for beta 7.

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a61f757afab3

Comment 28

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 66.0b7 (BuildId:20190211185957) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 18.04 64bit.

Comment 29

[Firefox Bug Husbandry Bot]

Please specify a root cause for this bug. See :tmaity for more information.