1430391 - Shield studies checkbox is checked even if checked attribute is "false"

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[magicp]

Attachment: shield-studies-checkbox.png

Steps to reproduce:
1. Launch Nightly
2. Go to about:preferences#privacy > Nightly Data Collection and Use
3. Check off "Allow Firefox to install and run studies"
4. Restart Nightly
5. Confirm "Allow Firefox to install and run studies"

Actual results:
Shield studies checkbox is checked even if checked attribute is "false" (app.shield.optoutstudies.enabled = false).

Expected results:
Display checkbox correctly.

Comment 1

[magicp]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d760cf06ca40056077e2a354b8b69350a5765ae3&tochange=0adedb70b7883ab39239b8531d07f1308ffc10c7

Comment 2

[Myk Melez [:myk] [@mykmelez]]

I see this on Windows and Linux but not Mac.  Investigating…

Comment 3

[magicp]

Attachment: macOS.png

I can reproduce on macOS 10.11 and 10.13

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to magicp from comment #0)
> 3. Check off "Allow Firefox to install and run studies"
> 4. Restart Nightly
> 5. Confirm "Allow Firefox to install and run studies"

On Linux, anyway, I don't even need to restart Nightly.  I just need to switch to the General tab, reload the preferences page, and switch back to the Privacy & Security tab.

I think I know what's going on here.  My next step is to confirm a fix and write an automated test for it.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: make setValue set/remove "checked" attribute appropriately

The setValue function, which I copied without changes from preferences.xml to preferencesBindings.js, sets either an element's property or the equivalent attribute, depending on whether or not the element has a property with the given name.

The function's comment explains the reason for this:

>Initialize a UI element property with a value. Handles the case
>where an element has not yet had a XBL binding attached for it and
>the property setter does not yet exist by setting the same attribute
>on the XUL element using DOM apis and assuming the element's
>constructor or property getters appropriately handle this state.

In other words, setValue can race XBL binding attachment, so it accounts for that possibility by setting the attribute rather than the property if the property isn't yet defined.

In the case of the checkbox and listitem elements, however, when the element doesn't yet have a XBL binding attached, then setValue sets the "checked" attribute incorrectly.  Those elements expect the attribute to be present (with any value) if the element is checked and absent if the element is unchecked.  But setValue sets the attribute to "true" if the element is checked and "false" if the element is unchecked.

In other words, setValue sets checked="false" when the element shouldn't be checked, which gets interpreted the same as checked="true": the element appears checked.

I'm unsure why this bug didn't manifest previously, but I suspect it's because setValue used to be called later in the page load process, since it used to be triggered by <preference> binding constructors, and now it's triggered by DOMContentLoaded.  Thus it presumably won the race less frequently, making this bug less obvious.

In any case, the fix is obvious: make setValue set/remove the "checked" attribute appropriately.  Here's the fix.

Unfortunately, I'm unsure how to write an automated test for this, since by definition the behavior is indeterminate.  The "checked" attribute won't always be set, and asserting that a checked element's internal state is consistent doesn't prove that it's because the "checked" attribute is now being correctly set/removed.

Once we replace the checkbox and menuitem XBL implementations with a non-XBL version, we should be able to make the behavior here determinate: either setValue always runs before the checkbox/menuitem implementations are instantiated, or it always runs afterward.  After which we will be able to remove one of the two paths here and test that the correct thing always happens.

Comment 6

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

Comment on attachment 8943406 [details] [diff] [review]
make setValue set/remove "checked" attribute appropriately

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

Thanks for the explanation. Yes, in XUL we treat presence of an attribute as true and absence as false.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac1424f154c
make setValue set/remove 'checked' attribute appropriately; r=jaws

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4ac1424f154c

Comment 9

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 59.0a1 (2018-01-13)on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Beta !

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

Comment 10

[Oana Botisan, Desktop QA]

I verified the fix on macOS 10.13 and Ubuntu 16.04 using latest Nightly 60.0a1 and beta 59.0b10, too. The bug is not reproducing anymore.