Closed Bug 1269820 Opened 7 years ago Closed 7 years ago

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


(Firefox :: Security, defect, P1)




Firefox 50
50.3 - Jul 18
Tracking Status
firefox50 --- verified


(Reporter: tanvi, Assigned: seban, Mentored)



(Whiteboard: [fxprivacy])


(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.

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:

to point to:
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 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:

Flags: needinfo?(sebastinssanty)
Comment on attachment 8767387 [details] [diff] [review]

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:

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]

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]

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

::: browser/themes/shared/identity-block/
@@ +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 file used for packaging. In this case

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]

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
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
Remove green lock with grey triangle when Mixed Active Content is Blocked r=florian
Keywords: checkin-needed
Depends on: 1285124
Closed: 7 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!
Flags: qe-verify+
We've updated the SUMO article now. Thanks for the heads up:
See Also: → 1322183
You need to log in before you can comment on or make changes to this bug.