Closed Bug 1185173 Opened 4 years ago Closed 4 years ago

Add support for loaded Passive Mixed Content

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox42 --- verified

People

(Reporter: liuche, Assigned: liuche)

References

Details

Attachments

(4 files)

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).
Assignee: nobody → liuche
Blocks: 1177576
No longer blocks: 860581
Status: NEW → ASSIGNED
Blocks: 1185600
Bug 1185173 - Add support for Passive Mixed Content. r=margaret
Attachment #8644078 - Flags: review?(margaret.leibovic)
Bug 1177576 - Update toolbar icons for passive mixed content. r=margaret
Attachment #8644079 - Flags: review?(margaret.leibovic)
Duplicate of this bug: 1185600
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
Attachment #8644079 - Attachment description: MozReview Request: Bug 1177576 - Update toolbar icons for passive mixed content. r=margaret → MozReview Request: Bug 1185173 - Update toolbar icons for passive mixed content. r=margaret
Attachment #8644085 - Flags: review?(alam)
Attachment #8644086 - Flags: review?(alam)
(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.
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 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.
Attachment #8644078 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8644079 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8644085 [details]
Screenshot: Passive mixed content loaded

Nice!
Attachment #8644085 - Flags: review?(alam) → review+
Comment on attachment 8644086 [details]
Screenshot: Active blocked, passive loaded

works for me!
Attachment #8644086 - Flags: review?(alam) → review+
https://hg.mozilla.org/mozilla-central/rev/53d87544fdf4
https://hg.mozilla.org/mozilla-central/rev/d015d5ec2f32
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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.
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
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
I will mark this bus as Verified fixed, since passive mixed content is supported on all branches.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.