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

VERIFIED FIXED in Firefox 59

Status

()

defect
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: magicp.jp, Assigned: myk)

Tracking

Trunk
Firefox 59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 verified, firefox60 verified)

Details

Attachments

(3 attachments)

Reporter

Description

a year ago
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.
Assignee

Comment 2

a year ago
I see this on Windows and Linux but not Mac.  Investigating…
Assignee: nobody → myk
Status: NEW → ASSIGNED
Reporter

Comment 3

a year ago
Posted image macOS.png
I can reproduce on macOS 10.11 and 10.13
Assignee

Comment 4

a year ago
(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.
Assignee

Comment 5

a year ago
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.
Flags: needinfo?(jaws)
Attachment #8943406 - Flags: review?(jaws)
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.
Attachment #8943406 - Flags: review?(jaws) → review+

Comment 7

a year ago
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac1424f154c
make setValue set/remove 'checked' attribute appropriately; r=jaws

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4ac1424f154c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
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
QA Whiteboard: [testday-20180216]
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.
Status: RESOLVED → VERIFIED
Depends on: 1453227
You need to log in before you can comment on or make changes to this bug.