Safe Browsing preferences graying out inconsistency

VERIFIED FIXED in Firefox 48

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: pauly, Assigned: rhankins, Mentored)

Tracking

({regression})

Trunk
Firefox 49
regression
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox46 unaffected, firefox47 unaffected, firefox48 verified, firefox49 verified)

Details

(Whiteboard: [fxprivacy] [good first bug])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8747015 [details]
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]

Updated

2 years ago
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
(Assignee)

Comment 3

2 years ago
Created attachment 8747395 [details] [diff] [review]
1268848.patch

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
(Assignee)

Comment 5

2 years ago
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 :)
(Assignee)

Comment 7

2 years ago
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.
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)
(Assignee)

Comment 12

2 years ago
Created attachment 8748230 [details] [diff] [review]
1268848-rev3.patch

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
(Assignee)

Comment 17

2 years ago
(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).
Keywords: checkin-needed

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6b0b3807493f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Updated

2 years ago
Iteration: --- → 49.1 - May 9
Priority: P3 → P1
(Reporter)

Comment 21

2 years ago
Verified fixed FX 49.0a1 (2016-05-08) Win 7
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified

Updated

2 years ago
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+

Comment 27

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d52c5d23e08d
status-firefox48: affected → fixed
(Reporter)

Comment 28

2 years ago
Verified fixed FX 48.0a2 (2016-06-05) Win 7
status-firefox48: fixed → verified
You need to log in before you can comment on or make changes to this bug.