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

Status: VERIFIED FIXED

(Firefox :: Security, defect, P1)

Description

[Tanvi Vyas[:tanvi]]

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

Comment 1

[Tanvi Vyas[:tanvi]]

We'll also need to update the Learn More link: https://support.mozilla.org/en-US/kb/mixed-content-blocking-firefox?redirectlocale=en-US&as=u&redirectslug=how-does-content-isnt-secure-affect-my-safety&utm_source=inproduct

Comment 2

[Sebastin Santy [:seban]]

Attachment: bug1269820_greenlock_for_mixedcontents.diff

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?

Comment 3

[Kamil Jozwiak [:kjozwiak]]

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

Comment 4

[Tanvi Vyas[:tanvi]]

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.

Comment 5

[Sebastin Santy [:seban]]

Attachment: bug1269820_greenlock_for_mixedcontents.diff

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?

Comment 6

[Tanvi Vyas[:tanvi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e2734f25b65

Comment 7

[Tanvi Vyas[: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.

Comment 8

[Sebastin Santy [:seban]]

As Tanvi has suggested, can you please review this patch?

Comment 9

[Florian Quèze [: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.

Comment 10

[Sebastin Santy [:seban]]

Attachment: bug1269820_greenlock_for_mixedcontents.diff

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)

Comment 11

[Florian Quèze [: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. :-)

Comment 12

[Sebastin Santy [:seban]]

Please can you help me adding checkin and push to try. I don't have any rights!

Comment 13

[Florian Quèze [: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 :-).

Comment 14

[Tanvi Vyas[:tanvi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c294a49bf4a

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c8e993f36248
Remove green lock with grey triangle when Mixed Active Content is Blocked r=florian

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c8e993f36248

Comment 17

[Carmen]

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

Comment 18

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Thank you, Carmen!

Comment 19

[Joni Chan]

We've updated the SUMO article now. Thanks for the heads up: https://support.mozilla.org/en-US/kb/mixed-content-blocking-firefox