1910312 - DoH settings expanding broken when compiling with MOZ_NORMANDY set to False

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[k4sum1]

Attachment: Recording of the issue

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:128.0) Gecko/20100101 Firefox/128.0 r3dfox/128.0.2

Steps to reproduce:

Compiling 128.0.2 or later with MOZ_NORMANDY set to False in browser/moz.configure

I've traced the issue to Bug 1908312 or https://hg.mozilla.org/releases/mozilla-release/rev/5a64403f8125

Actual results:

In settings or about:preferences, under Privacy & Security, at the very bottom, the arrows to expand the DNS over HTTPS settings do nothing.

This does not affect stock Firefox as it is built with MOZ_NORMANDY set to True. For this reason, the recording is from a fork that builds with MOZ_NORMANDY set to False.

Expected results:

These arrows should work and expand the settings like in 128.0.1 and earlier.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Networking: DNS' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

:Gijs, since you are the author of the regressor, bug 1908312, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 3

[Malte Jürgens]

This also is producible with MOZ_DATA_REPORTING disabled (I assume MOZ_NORMANDY is somehow related to that).

What seems to be happening is the following: Preferences.get(PREF_UPLOAD_ENABLED) returns null here, causing a error in privacy.js and preventing other code from running (the HTTPS-Only section for example is also broken). It is null because Preferences.addAll isn't being called for PREF_UPLOAD_ENABLED here as MOZ_DATA_REPORTING is disabled.

This wasn't a problem previously, because this code was only called with a previous check to MOZ_DATA_REPORTING here. But Bug 1908312 added a new call here that doesn't check for MOZ_DATA_REPORTING.

I will submit a patch shortly adding that check.

Comment 4

[Malte Jürgens]

Attachment: Bug 1910312 - Unbreak privacy and security settings when MOZ_DATA_REPORTING is disabled r?#settings-reviewers!

Disabling MOZ_DATA_REPORTING results in the PREF_UPLOAD_ENABLED pref not
being loaded with Preferences.add. This means
Preferences.get(PREF_UPLOAD_ENABLED) can possibly be null, which was
previously not handled and resulted in an error for the whole privacy.js file.

So only call dataCollectionCheckboxHandler for the privateAttribution
checkbox if MOZ_DATA_REPORTING is enabled (otherwise the privateAttribution
checkbox also just doesn't exist). Also move the call into separate
initPrivateAttributionCheckbox function to be more consistent with previous
code.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1908312

Comment 6

[k4sum1]

(In reply to Malte Jürgens from comment #3)

This also is producible with MOZ_DATA_REPORTING disabled (I assume MOZ_NORMANDY is somehow related to that).

I might have gotten those backwards when making the bugzilla issue thinking about it now.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1908312

Comment 8

[Donal Meehan [:dmeehan]]

:mossop do you plan on landing this change?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25b21c00ff1d
Unbreak privacy and security settings when MOZ_DATA_REPORTING is disabled r=settings-reviewers,mossop

Comment 11

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/25b21c00ff1d

Comment 12

[k4sum1]

Would it be possible for uplifts to beta and release please?

Comment 13

[Malte Jürgens]

Attachment: Bug 1910312 - Unbreak privacy and security settings when MOZ_DATA_REPORTING is disabled

Disabling MOZ_DATA_REPORTING results in the PREF_UPLOAD_ENABLED pref not
being loaded with Preferences.add. This means
Preferences.get(PREF_UPLOAD_ENABLED) can possibly be null, which was
previously not handled and resulted in an error for the whole privacy.js file.

So only call dataCollectionCheckboxHandler for the privateAttribution
checkbox if MOZ_DATA_REPORTING is enabled (otherwise the privateAttribution
checkbox also just doesn't exist). Also move the call into separate
initPrivateAttributionCheckbox function to be more consistent with previous
code.

Original Revision: https://phabricator.services.mozilla.com/D218265

Comment 14

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Privacy & Security settings will be broken on builds having MOZ_DATA_REPORTING disabled
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: -
• Risk associated with taking this patch: Low
• Explanation of risk level: This is a very simple change, only adding a single extra check before initializing a checkbox in the settings
• String changes made/needed: -
• Is Android affected?: no

Comment 15

[Malte Jürgens]

Attachment: Bug 1910312 - Unbreak privacy and security settings when MOZ_DATA_REPORTING is disabled

Disabling MOZ_DATA_REPORTING results in the PREF_UPLOAD_ENABLED pref not
being loaded with Preferences.add. This means
Preferences.get(PREF_UPLOAD_ENABLED) can possibly be null, which was
previously not handled and resulted in an error for the whole privacy.js file.

So only call dataCollectionCheckboxHandler for the privateAttribution
checkbox if MOZ_DATA_REPORTING is enabled (otherwise the privateAttribution
checkbox also just doesn't exist). Also move the call into separate
initPrivateAttributionCheckbox function to be more consistent with previous
code.

Original Revision: https://phabricator.services.mozilla.com/D218265

Comment 16

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: Privacy & Security settings will be broken on builds having MOZ_DATA_REPORTING disabled (such as the Tor browser, which is why this ESR uplift is especially important)
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: -
• Risk associated with taking this patch: Low
• Explanation of risk level: This is a very simple change, only adding a single extra check before initializing a checkbox in the settings
• String changes made/needed: -
• Is Android affected?: no

Comment 17

[Malte Jürgens]

Requested ESR and beta uplift, judging from the tracking flags I don't think a release uplift is wanted

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/8e0462b5e28f

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/327077da3712