Closed Bug 1268848 Opened 8 years ago Closed 8 years ago

Safe Browsing preferences graying out inconsistency

Categories

(Firefox :: Settings UI, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 49
Iteration:
49.1 - May 9
Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 --- verified
firefox49 --- verified

People

(Reporter: pauly, Assigned: rhankins, Mentored)

References

Details

(Keywords: regression, Whiteboard: [fxprivacy] [good first bug])

Attachments

(2 files, 2 obsolete files)

Attached image security options.png
[Note]:
- follow-up of bug 874408

[Affected versions]:
- 48.0a2 (2016-04-29), 49.0a1 (2016-04-28)

[Affected platforms]:
- all

[Steps to reproduce]:
1. Start Firefox
2. Open about:preferences#security
3. Check all the 3 options from General
4. Uncheck option #2 => option #3 grayed out.
5. Uncheck/recheck option #1

[Expected result]:
- Option #3 grayed out

[Actual result]:
- Option #3 enabled, even if option #2 is disabled

[Regression range]:
- introduced by bug 874408
Thanks for catching this!
Whiteboard: [fxprivacy] [triage]
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [fxprivacy] [triage] → [fxprivacy] [good first bug]
This one should be easy to fix:

In preferences/in-content/security.js#168[0] we enable the blockUncommonUnwanted element. However, this should only be done if blockDownloads is also checked.

Then, in the assert tests/tests/browser_security.js#47[1] we should make sure the assert also checks against blockDownloads.checked.

[0]https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/security.js#168
[1]https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/tests/browser_security.js#47
Mentor: jhofmann
Attached patch 1268848.patch (obsolete) — Splinter Review
Proposed patch.  I believe the security.js javascript is ok, and I was able to test it but I'm unsure how to test or verify the browser_security.js assert.
Hi Ryan! Thanks for contributing! You can find more information about these tests here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest

tl;dr just run "./mach test browser/components/preferences/in-content/tests/browser_security.js"

If I'm not mistaken your test should fail, because I believe it should test against !blockDownloads.checked instead.

Can you verify and update your patch?

Thanks!!
Assignee: nobody → rhankins
I was able to run the tests; thanks.  It seems like box 3 should be disabled if either box 1 is unchecked or box 2 is unchecked.
This check, however, fails:
is (blockUncommon.hasAttribute("disabled"), !checked || !blockDownloads.checked, "block uncommon checkbox is set correctly");

What am I missing?
Keywords: regression
The expression you're looking for is "checked || !blockDownloads.checked" if I'm not mistaken. Sorry for that, I can understand that usage of the "checked" variable here is pretty confusing.

If you feel like cleaning up you could add "checked = checkbox.checked;" in line 44 and then switch the "checked" variable in the two following assertions (like in your previous message).

Let me know how that works out or if you have any other questions :)
Attached patch 1268848-rev2.patch (obsolete) — Splinter Review
Ok, that makes sense to me (finally) -- the test code toggles the checkbox, so then it uses the inverted checked.  Thanks for the help; I'm sure you could have done this a lot faster yourself, but I understand it now.

Tests pass with this patch.
Attachment #8747395 - Attachment is obsolete: true
(In reply to Ryan Hankins from comment #7)
> Created attachment 8748169 [details] [diff] [review]
> 1268848-rev2.patch
> 
> Ok, that makes sense to me (finally) -- the test code toggles the checkbox,
> so then it uses the inverted checked.  Thanks for the help; I'm sure you
> could have done this a lot faster yourself, but I understand it now.
> 
> Tests pass with this patch.

Great, glad I could help you, that's what mentored bugs are for!
Comment on attachment 8748169 [details] [diff] [review]
1268848-rev2.patch

gcp, would you have time to quickly review this?
Attachment #8748169 - Flags: review?(gpascutto)
Comment on attachment 8748169 [details] [diff] [review]
1268848-rev2.patch

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

::: browser/components/preferences/in-content/tests/browser_security.js
@@ +43,5 @@
>         "safebrowsing.malware.enabled is set correctly");
>  
>      // check if the other checkboxes have updated
>      is(blockDownloads.hasAttribute("disabled"), checked, "block downloads checkbox is set correctly");
> +    is(blockUncommon.hasAttribute("disabled"), checked || !blockDownloads.checked, "block uncommon checkbox is set correctly");

As pointed out this is hella confusing but I understand from the discussion in the bug that's correct.
Attachment #8748169 - Flags: review?(gpascutto) → review+
(In reply to Gian-Carlo Pascutto [:gcp] from comment #10)
> 
> As pointed out this is hella confusing but I understand from the discussion
> in the bug that's correct.

Thanks! I totally agree. Ryan, we could push this now, but I'd love if you could follow my suggestion in comment #6 and make the code easier to read by reading the "checked" variable again in line 44.

What do you think? :)
Flags: needinfo?(rhankins)
It's much easier to follow this way.
Attachment #8748169 - Attachment is obsolete: true
Comment on attachment 8748230 [details] [diff] [review]
1268848-rev3.patch

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

::: browser/components/preferences/in-content/tests/browser_security.js
@@ +44,5 @@
>  
>      // check if the other checkboxes have updated
> +    checked = checkbox.checked;
> +    is(blockDownloads.hasAttribute("disabled"), !checked, "block downloads checkbox is set correctly");
> +    is(blockUncommon.hasAttribute("disabled"), !checked || !blockDownloads.checked, "block uncommon checkbox is set correctly");

Shouldn't these read "unset" or "unchecked" rather than "set"?
Attachment #8748230 - Flags: review+
(In reply to Gian-Carlo Pascutto [:gcp] from comment #13)
> Comment on attachment 8748230 [details] [diff] [review]
> 1268848-rev3.patch
> 
> Review of attachment 8748230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/in-content/tests/browser_security.js
> @@ +44,5 @@
> >  
> >      // check if the other checkboxes have updated
> > +    checked = checkbox.checked;
> > +    is(blockDownloads.hasAttribute("disabled"), !checked, "block downloads checkbox is set correctly");
> > +    is(blockUncommon.hasAttribute("disabled"), !checked || !blockDownloads.checked, "block uncommon checkbox is set correctly");
> 
> Shouldn't these read "unset" or "unchecked" rather than "set"?

Well they're either set or unset, depending on if the checkbox.
Flags: needinfo?(rhankins)
Ok, now we will need a Try push to see if everything's still working as it should. You can find more details here: https://wiki.mozilla.org/ReleaseEngineering/TryServer

I'll do this for you for now. If you want to work on more bugs in the future you should think about getting your own Try access!

Another thing: Please format your commit message according to our commit message conventions. Details here: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions
(In reply to Johann Hofmann [:johannh] from comment #15)
> Ok, now we will need a Try push to see if everything's still working as it
> should. You can find more details here:
> https://wiki.mozilla.org/ReleaseEngineering/TryServer
> 
> I'll do this for you for now. If you want to work on more bugs in the future
> you should think about getting your own Try access!
> 
> Another thing: Please format your commit message according to our commit
> message conventions. Details here:
> https://developer.mozilla.org/en-US/docs/Mercurial/
> Using_Mercurial#Commit_Message_Conventions

Ok, thanks -- that was fun.
(In reply to Ryan Hankins from comment #17)
> 
> Ok, thanks -- that was fun.

Great that you liked it! :)

I'll add the "checkin-needed" flag so that this will get pushed into the tree if the try looks ok.

We would obviously be happy if you'd keep contributing even more! The only bug I'm mentoring right now is Bug 1194874. That one might be a little too easy. You could still do it to get into the contributing workflow or leave it for someone else, it's up to you :)

Another great way of finding good bugs to work on is Bugs Ahoy (http://www.joshmatthews.net/bugsahoy). I know for certain that e.g. the DevTools people have a great number of ESLint errors that they need help fixing (http://www.joshmatthews.net/bugsahoy/?devtools=1&unowned=1).
https://hg.mozilla.org/mozilla-central/rev/6b0b3807493f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Iteration: --- → 49.1 - May 9
Priority: P3 → P1
Verified fixed FX 49.0a1 (2016-05-08) Win 7
Status: RESOLVED → VERIFIED
Flags: qe-verify? → qe-verify+
Want to request uplift to aurora for this? The regression is from 48 so it would be nice to fix before the regression ships to release.
Flags: needinfo?(rhankins)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22)
> Want to request uplift to aurora for this? The regression is from 48 so it
> would be nice to fix before the regression ships to release.

Sure, thanks! How do we do that? :)
Flags: needinfo?(rhankins) → needinfo?(lhenry)
From the view of the patch in bugzilla, set the flag for mozilla-aurora-approval to "?" and fill out the questions that will pop up in the latest bugzilla comment.   It's some questions about risk, the cause of the regression, test coverage, and so on.  

Uplift and approvals are described here, https://wiki.mozilla.org/Release_Management/Uplift_rules#Guidelines_on_approval_comments
Flags: needinfo?(lhenry)
Comment on attachment 8748230 [details] [diff] [review]
1268848-rev3.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 874408, which changes the safe browsing settings
[User impact if declined]: Through a bit of clicking users might get to a situation where the UI is not reflecting the actual safe browsing settings. Confusing but not harmful.
[Describe test coverage new/current, TreeHerder]: Automated tests and QA verified
[Risks and why]: This is a really simple change that only affects the settings UI. If anything were wrong, the safe browsing settings UI would malfunction. Given the testing around this it's highly unlikely that will happen, though.
[String/UUID change made/needed]:none
Attachment #8748230 - Flags: approval-mozilla-aurora?
Comment on attachment 8748230 [details] [diff] [review]
1268848-rev3.patch

Fix a new regression, taking it
Attachment #8748230 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed FX 48.0a2 (2016-06-05) Win 7
You need to log in before you can comment on or make changes to this bug.