1431942 - Privacy & Security in Preferences doesn't open without MOZ_DATA_REPORTING

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P5)

Description

[pubkeypin]

Attachment: Check for MOZ_DATA_REPORTING

My limited understanding is, that

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.xul#570

checks for MOZ_DATA_REPORTING when composing preferences.xul, while
the following functions of the completely included privacy.js

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#410
https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#412
https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#414
https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#416

don't. The subsequently called

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#1536
https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#1541
https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#1585

and especially

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#1564

don't bother either about the state of MOZ_DATA_REPORTING, while for example the further invoked

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#1549

assumes, that

let el = document.getElementById(element);

in

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#1555
and
https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#1557

exists.

This assumption is false without MOZ_DATA_REPORTING being "true",
because the "<!-- Firefox Data Collection and Use -->"
part of privacy.xul isn't built into preferences.xul .

Consequently privacy.js throws an error, because no attribute can be applied to the non-existing "element"
and therefore the "Privacy & Security" page of "Preferences" doesn't open on click.

The same result might be observed for the missing preferences and checkboxes inside these functions.

To mirror the decision made at buildtime to leave out parts of privacy.xul, the attached (possibly not correctly formated ?) patch proposes to check for "AppConstants.MOZ_DATA_REPORTING" before calling the relevant functions as it is already done for "AppConstants.MOZ_CRASHREPORTER" in

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.js#411

The described scenario might be considered invalid,
if building with MOZ_DATA_REPORTING set to "false" isn't supposed to work.
But then the mentioned "#ifdef MOZ_DATA_REPORTING" in privacy.xul seems superfluous and shouldn't exist.

Building my browser (without MOZ_DATA_REPORTING) upon version 57.0 with (a rebased) patch lets me open the Privacy & Security page.
Without patch this fails.

If I'm wrong, another bug category is more appropriate or if there is anything else I can help with, please say so.

Comment 1

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

Comment on attachment 8944187 [details] [diff] [review]
Check for MOZ_DATA_REPORTING

Review of attachment 8944187 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch, looking at the code this indeed is an issue that should be fixed. Thank you for the patch.

Can you update your commit message to read (without the quotes):
"Bug 1431942 - Refactor privacy.js initialization code to account for MOZ_DATA_REPORTING being false. r=jaws"

Then when you re-upload the patch I can get it landed for you.

Comment 2

[pubkeypin]

Attachment: Check for MOZ_DATA_REPORTING v2

In Version 2 (only) the commit message is changed.

Thanks for the help Jared, especially with all these flags and landing stuff!

If there are some more buttons I should push or anything else, please don't hesitate to give advice.

Comment 3

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

You're welcome, and thank you for helping to fix a bug in Firefox! Just reporting a bug is a great help, but including a patch to fix it is awesome!

If you'd like to help out with another privacy/telemetry related bug, we need some help for bug 1427104. From the looks of this bug you would probably be able to help out there relatively easily.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87fed30c5759
Refactor privacy.js initialization code to account for MOZ_DATA_REPORTING being false. r=jaws

Comment 5

[pubkeypin]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> You're welcome, and thank you for helping to fix a bug in Firefox! Just
> reporting a bug is a great help, but including a patch to fix it is awesome!

Thank you Jared for guiding me through this bug in a very welcoming manner.

Related to this bug probably worth mentioning is, that with MOZ_DATA_REPORTING being false the groupbox with id="dataCollectionGroup" isn't incorporated into preferences.xul , but the hbox with id="dataCollectionCategory" still is:

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.xul#562

For me that is no problem, but some user interface folks might be disturbed by a heading without actual content.
Perhaps the hbox should be moved below/inside of the "#ifdef MOZ_DATA_REPORTING"?

> If you'd like to help out with another privacy/telemetry related bug, we
> need some help for bug 1427104. From the looks of this bug you would
> probably be able to help out there relatively easily.

Me feels honoured by the offer in regard to bug 1427104.

I really wanted to help and have looked a little bit into this crashreporter area, but have to admit, that this seems currently above my capabilities. Sorry.

In my setup the crashreporter is most of the time disabled and I would have to learn first how to provoke Firefox to crash in order to fully reproduce and understand the reporters problem. There might be platform specific issues and the reporter is on windows, while I'm not.

The assignee in much more qualified.


Hopefully the "needinfo" is not annoying - it said: "please needinfo? me".

I don't know if it should simply help you to track areas of interest, or if the usage is strictly reserved for situations, where your answer is irreplaceable.

If it's the later, that was not my intention and you can ignore it.
I can only offer "as compensation" to implement the mentioned move of the hbox - provided that is wanted.

Comment 6

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/87fed30c5759

Comment 7

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

(In reply to pubkeypin from comment #5)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> > You're welcome, and thank you for helping to fix a bug in Firefox! Just
> > reporting a bug is a great help, but including a patch to fix it is awesome!
> 
> Thank you Jared for guiding me through this bug in a very welcoming manner.
> 
> Related to this bug probably worth mentioning is, that with
> MOZ_DATA_REPORTING being false the groupbox with id="dataCollectionGroup"
> isn't incorporated into preferences.xul , but the hbox with
> id="dataCollectionCategory" still is:
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> preferences/in-content/privacy.xul#562
> 
> For me that is no problem, but some user interface folks might be disturbed
> by a heading without actual content.
> Perhaps the hbox should be moved below/inside of the "#ifdef
> MOZ_DATA_REPORTING"?
> 
> > If you'd like to help out with another privacy/telemetry related bug, we
> > need some help for bug 1427104. From the looks of this bug you would
> > probably be able to help out there relatively easily.
> 
> Me feels honoured by the offer in regard to bug 1427104.
> 
> I really wanted to help and have looked a little bit into this crashreporter
> area, but have to admit, that this seems currently above my capabilities.
> Sorry.
> 
> In my setup the crashreporter is most of the time disabled and I would have
> to learn first how to provoke Firefox to crash in order to fully reproduce
> and understand the reporters problem. There might be platform specific
> issues and the reporter is on windows, while I'm not.
> 
> The assignee in much more qualified.
> 
> 
> Hopefully the "needinfo" is not annoying - it said: "please needinfo? me".
> 
> I don't know if it should simply help you to track areas of interest, or if
> the usage is strictly reserved for situations, where your answer is
> irreplaceable.
> 
> If it's the later, that was not my intention and you can ignore it.
> I can only offer "as compensation" to implement the mentioned move of the
> hbox - provided that is wanted.

Thanks for the comment. The needinfo applies to "helping me track areas of interest" because I get a few hundred emails daily and it is easy for something to slip through the cracks if I'm not specifically flagged, so thank you.

Yes, I think it would be helpful to move the <hbox>, thanks for pointing that out. Can you file a new bug and put up a patch there? Then you can flag me for "review" on the patch, which will be just as good as needinfo.

Thanks!