1425699 - No modal restart dialog when toggling OFF always-on PBM after navigating directly to about:preferences#privacy

Status: REOPENED

(Firefox :: Settings UI, defect, P3)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

STR:

1. Navigate to about:preferences#privacy
2. Turn on "Always use private browsing mode" and restart the browser via the modal dialog.
3. Navigate to about:preferences#privacy
4. Turn off "Always use private browsing mode"

Expected:
Modal dialog prompting to restart the browser. If canceled, the checkbox reverts.

Actual:
No modal dialog. Checkbox stays unchecked. If checked again, modal dialog appears to restart Firefox. I'd call this a garbled state and would not be surprised if it causes weird bugs around the custom history prefs.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

This bug as well as bug 1423564 are exposed by bug 1375870. The UI initialization code is being called at a time when state that it depends on hasn't stabilized yet. This worked before because _constructAfterChildren was being called so many times that at some point it triggered initializaiton code in a "change" listener when the state was correct.

I propose exposing a promise in preferences.xml that resolves after _constructAfterChildren, and ensuring any dynamic UI initialization code yields this before doing anything. If I can't find an elegant way to do this I'll probably fire an event at the end of _constructAfterChildren and listen for that.

Stay tuned for a patch!

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1425699 - Delay privacy preference pane UI initialization until constructAfterChildren has been called.

empty to use default message. HG: -- HG: user: Nihanth Subramanya <[e-mail]>
browser/components/preferences/in-content/privacy.js HG: changed
toolkit/content/widgets/preferences.xml

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

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

:jaws, this patch fires an event after _constructAfterChildren, and delays initialization of the privacy pane until this event fires.

Some context in case you need some:
The pref values from <preference> elements are propagated to UI widgets that use them in the updateElements method in the preference binding. This function is called on all <preference> elements from the _constructAfterChildren function of the parent <preferences> element once they have ALL been constructed. Some of the code in init() is using UI state to set other UI states before this happens, which means it gets the wrong value.

I propose that we should do this for all panes; perhaps in register_module in preferences.js? What do you think? I can update the patch to this effect and maybe try and add some tests (at the very least for the custom history setting checkboxes) if you think this is a good approach.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Hmm, seems like this patch causes test failures. I'm investigating, but in the meantime, still curious what you think of the approach and whether you think we can do deferred initialization for all preference panes.

Comment 5

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

Comment on attachment 8937304 [details]
Bug 1425699 - Delay privacy preference pane UI initialization until constructAfterChildren has been called.

https://reviewboard.mozilla.org/r/208010/#review214334

::: commit-message-23868:3
(Diff revision 1)
> +Bug 1425699 - Fire event after _constructAfterChildren; delay privacy pane initialization until this is fired. r=johannh
> +
> +empty to use default message. HG: -- HG: user: Nihanth Subramanya <[e-mail]>

looks like this accidentally got included here.

::: browser/components/preferences/in-content/privacy.js:89
(Diff revision 1)
> +    let prefsElt = document.getElementById("privacyPreferences");
> +    if (prefsElt._constructAfterChildrenCalled) {
> +      this._delayedInit();

I like the idea of delayedInit but I would prefer that we use a Promise here and then we don't need to check if "_constructAfterChildrenCalled", we can simply use promise.then().

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8937304 [details]
Bug 1425699 - Delay privacy preference pane UI initialization until constructAfterChildren has been called.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208010/diff/1-2/

Comment 7

[Florian Quèze [:florian]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #6)
> Comment on attachment 8937304 [details]
> Bug 1425699 - Delay privacy preference pane UI initialization until
> constructAfterChildren has been called.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/208010/diff/1-2/

I think you'll need another update now that bug 1379338 has landed.

Comment 8

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

Comment on attachment 8937304 [details]
Bug 1425699 - Delay privacy preference pane UI initialization until constructAfterChildren has been called.

https://reviewboard.mozilla.org/r/208010/#review216388

This looks good. Were you going to make similar changes to the other preference categories as well? Note that Florian mentioned this will need rebasing.

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

This patch is no longer required. I can no longer reproduce this bug now that bug 1379338 has landed.

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Uh, I spoke too soon. My bad. Apologies.

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

OK, after perusing the new code for a while, I think the best approach is to implement a promise queue in the Preferences object. Specifically, instead of just then'ing updateElements() calls on the domContentLoadedPromise, we can queue the calls on top of each other, and expose the queue promise for consumers. Here's what the result would look like (ref. [1]):

> const updateElementsPromise = new Promise(resolve => {
>   window.addEventListener("DOMContentLoaded", resolve, { capture: true, once: true });
> });

> const Preferences = {
> 
> // [...]
> 
>   _add(prefInfo) {
>     // [...]
>     updateElementsPromise =
>       updateElementsPromise.then(() => { pref.updateElements(); });
>     // [...]
>   },
> 
> // [...]
> }

Consumers that are calling Preferences.add or Preferences.addAll can then wait for updateElements to be called on all of the preference elements in a deterministic fashion.

I'll push a patch to this effect for review soon.

[1] https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/toolkit/content/preferencesBindings.js#27

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1425699 - Delay privacy preference pane UI initialization until updateElements has been called on all prefs.

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

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8941077 [details]
Bug 1425699 - Delay privacy preference pane UI initialization until updateElements has been called on all prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211350/diff/1-2/

Comment 14

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

Comment on attachment 8941077 [details]
Bug 1425699 - Delay privacy preference pane UI initialization until updateElements has been called on all prefs.

https://reviewboard.mozilla.org/r/211350/#review217930

Nice, this is what I was thinking of :)

Comment 15

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8941077 [details]
Bug 1425699 - Delay privacy preference pane UI initialization until updateElements has been called on all prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211350/diff/2-3/

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

Time to admit defeat on this one, I think, I got distracted by other things and no longer have mental context on this patch. Apologies.