Closed Bug 1269820 Opened 6 years ago Closed 6 years ago

Remove green lock with grey triangle when Mixed Active Content is Blocked

Categories

(Firefox :: Security, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox50 --- verified

People

(Reporter: tanvi, Assigned: seban, Mentored)

References

Details

(Whiteboard: [fxprivacy])

Attachments

(1 file, 2 obsolete files)

Telemetry data shows that the very few users are "disabling protection" on pages where we have blocked mixed active content.

http://mzl.la/1pGeWUt

Here are percentages of pages that went through a "disable protection" action (unblock mixed content) out of all page loads.
Release 45: 0.0007%
Beta 46: 0.001%
Aurora 47: 0.0009%
Nightly 48: 0.00077%

Note that these numbers are for all page loads, not HTTPS page loads.  To be conservative, we could assume that 25% of pages are HTTPS (when really it is closer to 44) and multiply the above numbers by 4.  Even then we are talking about 0.004% of HTTPS page loads.  Hence, with the current heuristics, we should be able to remove the icon decoration without much trouble.

The disable protection button will still remain in control center.  Nothing from the control center will change.  The only change is that the url bar will show a green lock instead of a green lock with a grey triangle.

For fxprivacy team triage, I think this should be a P3.  It reduces user confusion by eliminating a url bar state and is pretty easy to implement.

I believe all you need to do is change the image here:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/identity-block/identity-block.inc.css#184

to point to:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/identity-block/identity-secure.svg
Mentor: tanvi
Assignee: nobody → kjozwiak
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Hi Tanvi,

I have made the necessary changes and now only the green lock appears (without grey triangle), for mixed contents pages (tested on https://people.mozilla.org/~mkelly/mixed_test.html). Please can you review my patch?
Attachment #8767387 - Flags: review?(tanvi)
Hi Sebastin,

The above patch/changes breaks a test when running "./mach test mcb". Would you like to work on this as well?

The following tests failed:
74 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_mcb_redirect.js | Using active blocked icon - Got url("chrome://browser/skin/identity-secure.svg"), expected url("chrome://browser/skin/identity-mixed-active-blocked.svg")
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:967
chrome://mochitests/content/browser/browser/base/content/test/general/head.js:assertMixedContentBlockingState:734
chrome://mochitests/content/browser/browser/base/content/test/general/browser_mcb_redirect.js:checkUIForTest1:111
resource://gre/modules/RemoteAddonsParent.jsm:EventTargetParent.dispatch/<:592
resource://gre/modules/Prefetcher.jsm:Prefetcher.withPrefetching:460
resource://gre/modules/RemoteAddonsParent.jsm:EventTargetParent.dispatch:588
resource://gre/modules/RemoteAddonsParent.jsm:EventTargetParent.receiveMessage:536

[1] https://pastebin.mozilla.org/8881818
Flags: needinfo?(sebastinssanty)
Comment on attachment 8767387 [details] [diff] [review]
bug1269820_greenlock_for_mixedcontents.diff

Thanks for the patch Sebastin!

We can remove the icon itself as well from browser/themes/shared/identity-block/identity-mixed-active-blocked.svg.

Also it is referenced here, which may relate to the test failure:
http://searchfox.org/mozilla-central/source/browser/base/content/test/general/head.js#734

We should make the same change there, to identity-secure.svg.

Run ./mach mochitest mcb locally.  If that succeeds, push to try.

r- for now and will take another look at the second patch.
Attachment #8767387 - Flags: review?(tanvi) → review-
Thanks Tanvi and Kamil for the suggestions. I have made the necessary changes and tested the same. 

Tanvi: Unfortunately I don't have a push access to try. Can you do this for me?
Attachment #8767387 - Attachment is obsolete: true
Flags: needinfo?(sebastinssanty) → needinfo?(tanvi)
Attachment #8768170 - Flags: review?(tanvi)
Comment on attachment 8768170 [details] [diff] [review]
bug1269820_greenlock_for_mixedcontents.diff

r+, but you also probalby want a browser peer.  I propose Florian Queze or Paolo Amadini.
Attachment #8768170 - Flags: review?(tanvi) → review+
As Tanvi has suggested, can you please review this patch?
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(florian)
Comment on attachment 8768170 [details] [diff] [review]
bug1269820_greenlock_for_mixedcontents.diff

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

I'm reviewing only the actual code changes, as I assume Tanvi has already thought through the behavior change.

In the future, please attach patches with 8 lines of context. You can do it using |hg diff -p -U 8| or configure mercurial to do it automatically, see https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Installing_Mercurial#Basic_configuration

::: browser/themes/shared/identity-block/identity-block.inc.css
@@ +186,3 @@
>  }
>  
>  #urlbar[pageproxystate="valid"] > #identity-box.mixedActiveBlocked > #connection-icon {

Please merge this into the other existing CSS rule in the same file that uses identity-secure.svg.

::: browser/themes/shared/identity-block/identity-mixed-active-blocked.svg
@@ -1,1 @@
> -<?xml version="1.0" encoding="utf-8"?>

When removing a file that goes to a chrome:// url, you need to remove it from the jar.mn file used for packaging. In this case http://searchfox.org/mozilla-central/source/browser/themes/shared/jar.inc.mn#67

Also, it's generally a good idea to search for the file name in a tool like searchfox to ensure there aren't other uses of the file you are removing.
Attachment #8768170 - Flags: review-
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(florian)
I have made the changes. 

I wasn't able to put the 8 lines of context (though I updated my hgrc. My guess is, it is possible only with a new patch)
Attachment #8768170 - Attachment is obsolete: true
Attachment #8768481 - Flags: review?(florian)
Comment on attachment 8768481 [details] [diff] [review]
bug1269820_greenlock_for_mixedcontents.diff

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

Looks good, thanks.

(In reply to Sebastin Santy [:seban] from comment #10)

> I wasn't able to put the 8 lines of context (though I updated my hgrc. My
> guess is, it is possible only with a new patch)

Your updated patch does contain 8 lines of context. :-)
Attachment #8768481 - Flags: review?(florian) → review+
Please can you help me adding checkin and push to try. I don't have any rights!
Flags: needinfo?(florian)
(In reply to Sebastin Santy [:seban] from comment #12)
> Please can you help me adding checkin and push to try. I don't have any
> rights!

I gave editbugs permissions on your bugzilla account, so now you can set the checkin-needed keyword yourself. You can also assign the bug to yourself. Use your new powers wisely :-).

For the try server, see https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server
If you file a bug to request Level1 commit access, cc me and I'll vouch for you.

Note: If this was my patch, I would probably push it to fx-team without pushing to try first, as I don't see what this change could break. Not sure if this is convincing enough for the people handling the checkin-needed queue though :-).
Flags: needinfo?(florian)
Assignee: kjozwiak → nobody
Keywords: checkin-needed
Assignee: nobody → sebastinssanty
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c8e993f36248
Remove green lock with grey triangle when Mixed Active Content is Blocked r=florian
Keywords: checkin-needed
Depends on: 1285124
https://hg.mozilla.org/mozilla-central/rev/c8e993f36248
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.3 - Jul 18
Priority: P3 → P1
Flags: qe-verify? → qe-verify+
I reproduced the previous behavior using Firefox Nightly 49.0a1 (2016-05-10) on Windows 10 Pro x64.

The fix is now verified on Firefox Beta 50.0b4 across platforms: Windows 10 Pro x64, Ubuntu 16.04 LTS x64 and Mac OS Sierra 10.12.1
Thank you, Carmen!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
We've updated the SUMO article now. Thanks for the heads up: https://support.mozilla.org/en-US/kb/mixed-content-blocking-firefox
See Also: → 1322183
You need to log in before you can comment on or make changes to this bug.