1185173 - Add support for loaded Passive Mixed Content

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

In order to implement the icon states for passive mixed content states in bug 1177576, we need to notify if we're loading passive mixed content. Currently, we only handle active mixed content (active+blocked or active+loaded).

Comment 1

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0ebb7da63ced

Comment 2

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/5721360e23cd

Comment 3

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1185173 - Add support for Passive Mixed Content. r=margaret

Bug 1185173 - Add support for Passive Mixed Content. r=margaret

Comment 4

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1185173 - Update toolbar icons for passive mixed content. r=margaret

Bug 1177576 - Update toolbar icons for passive mixed content. r=margaret

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8644079 [details]
MozReview Request: Bug 1185173 - Update toolbar icons for passive mixed content. r=margaret

Bug 1185173 - Update toolbar icons for passive mixed content. r=margaret

Comment 7

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Passive mixed content loaded

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Active blocked, passive loaded

Comment 9

[Tanvi Vyas[:tanvi]]

(In reply to Chenxia Liu [:liuche] from comment #8)
> Created attachment 8644086 [details]
> Screenshot: Active blocked, passive loaded

Can we used a string similar to Desktop's for this?  Something like:
Although Fennec has blocked some content, there are still elements on this page that are not secure.

Comment 10

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/14087/#review13681

::: mobile/android/base/strings.xml.in:473
(Diff revision 2)
> +  <string name="mixed_content_present">&mixed_content_present;</string>

You may consider changing this string to "mixed_display_content_present" or "mixed_display_content_loaded".  Since mixed active content is loaded only when "mixed_content_protection_disabled".

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java:442
(Diff revision 2)
>          // Check to see if any protection was overridden first

The mixed content and tracking protection states are independent.  But Fennec only has real estate for either the shield or the site identity icons. If I recall correctly, the shield will take precendence.  Is that why this if else first checks tracking protection and then checks mixed content?

::: mobile/android/chrome/content/browser.js:7187
(Diff revision 2)
>           !Services.prefs.getBoolPref("security.mixed_content.block_active_content")) {

I think we want to show the new iconograpy regardless of whether the user has disabled the pref or not.  If the user disables the pref, they shouldn't get a lock in the url bar for a page that loaded mixed active content.

Also, I'm not sure there is supposed to be a ! in front of the pref check.  If the pref is true, !true will fail the check and the mode won't get set.

::: mobile/android/chrome/content/browser.js:7264
(Diff revision 2)
> +      result.secure = true;

What if STATE_IS_INSECURE?  Do we need to check that too or will we only hit this part of the code for https pages?

Comment 11

[:Margaret Leibovic]

Comment on attachment 8644078 [details]
MozReview Request: Bug 1185173 - Add support for Passive Mixed Content. r=margaret

https://reviewboard.mozilla.org/r/14089/#review13689

::: mobile/android/chrome/content/browser.js:7263
(Diff revision 1)
> +    } else {

Nit: We don't need an else statement here, since the if statement ends with a return.

::: mobile/android/chrome/content/browser.js:7173
(Diff revision 1)
> -    if (aState & Ci.nsIWebProgressListener.STATE_BLOCKED_MIXED_ACTIVE_CONTENT) {
> +    if ((aState & Ci.nsIWebProgressListener.STATE_LOADED_MIXED_DISPLAY_CONTENT)) {

Nit: Remove the extra set of parens here.

::: mobile/android/chrome/content/browser.js:7166
(Diff revision 1)
> -      return this.IDENTITY_MODE_DOMAIN_VERIFIED;
> +      return this.IDENTITY_MODE_IDENTIFIED;

Why are these reversed here? Was this wrong before?

::: mobile/android/base/toolbar/SiteIdentityPopup.java:299
(Diff revision 1)
>      private void updateConnectionState(final SiteIdentity siteIdentity) {

It may be worth adding some documentation above this method to explain all the different states and what we expect to happen. There are a lot of logic paths in here!

I looked through this, but I'm not 100% confident in all the different logic paths, so I'll trust you verified all the different states are displayed correctly.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8644079 [details]
MozReview Request: Bug 1185173 - Update toolbar icons for passive mixed content. r=margaret

https://reviewboard.mozilla.org/r/15211/#review13691

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java:131
(Diff revision 2)
> +    private final int LEVEL_SHIELD_DISABLED = 6;

It could also be nice to document what these different icon states mean.

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8644085 [details]
Screenshot: Passive mixed content loaded

Nice!

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8644086 [details]
Screenshot: Active blocked, passive loaded

works for me!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/53d87544fdf4
https://hg.mozilla.org/integration/fx-team/rev/d015d5ec2f32

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53d87544fdf4
https://hg.mozilla.org/mozilla-central/rev/d015d5ec2f32

Comment 17

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/14087/#review13681

> The mixed content and tracking protection states are independent.  But Fennec only has real estate for either the shield or the site identity icons. If I recall correctly, the shield will take precendence.  Is that why this if else first checks tracking protection and then checks mixed content?

Yep! That is correct.

> What if STATE_IS_INSECURE?  Do we need to check that too or will we only hit this part of the code for https pages?

The identityMode check will handle http vs https connections, and only https pages will hit the STATE_IS_BROKEN check (which will take care of loaded mixed content).

> I think we want to show the new iconograpy regardless of whether the user has disabled the pref or not.  If the user disables the pref, they shouldn't get a lock in the url bar for a page that loaded mixed active content.
> 
> Also, I'm not sure there is supposed to be a ! in front of the pref check.  If the pref is true, !true will fail the check and the mode won't get set.

We shouldn't be able to load Active Mixed Content if the pref is disabled, but maybe we shouldn't check for the pref at all then. In the case where the pref fails though, we just display the "Insecure Connection" state - we definitely don't display a green secure lock.

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: nexus 7 (Android 5.1)
Build: Firefox for Android 43.0a1 (2015-09-15)
Mixed Content with passive loaded: https://goo.gl/frb7Ft
Active Mixed Content blocked with passive loaded: https://goo.gl/GOA7QZ

Build: Firefox for Android 42.0a2 (2015-09-15)
Mixed Content with passive loaded: https://goo.gl/S75ZBB
Active Mixed Content blocked with passive loaded: https://goo.gl/jW4aQd

Comment 19

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Samsung S6 Edge (Android 5.1)
Build: Firefox for Android 42 beta 1
Mixed Content with passive loaded: https://goo.gl/zdyVpi
Active Mixed Content blocked with passive loaded: https://goo.gl/pjwZdm

Comment 20

[Teodora Vermesan (:TeoVermesan)]

I will mark this bus as Verified fixed, since passive mixed content is supported on all branches.