Closed Bug 1175702 Opened 5 years ago Closed 5 years ago

Move Mixed Content Blocking labels and controls into Control Center

Categories

(Firefox :: General, defect, P1)

defect
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.3 - Aug 10
Tracking Status
firefox42 --- verified

People

(Reporter: bgrins, Assigned: ttaubert)

References

Details

(Whiteboard: [fxprivacy] [campaign])

Attachments

(4 files, 6 obsolete files)

51.69 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
3.76 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
4.68 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
4.87 KB, patch
Details | Diff | Splinter Review
I believe Mixed Content Blocking is getting merged into the Control Center instead of being a separate anchor / doorhanger.

needinfo? for Tim to make sure that this is correct and that there isn't already a bug on file and/or a work in progress for this.
Flags: needinfo?(ttaubert)
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Rank: 4
Will take care of filing all necessary bugs.
Flags: needinfo?(ttaubert)
A few things to do here:

1) Don't show the shield icon (shared with TP currently) anymore when there's mixed content.
2) Add a text that says we blocked insecure stuff to the security section in the main view
3) Add some more text and a button to disable blocking to the security subview
4) Remove the MCB contents from the panel that's shared with TP
Turns out Brian already filed all the necessary bugs, I just clarified a few things. Thanks Brian!
Points: --- → 8
Flags: qe-verify+
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 42.1 - Jul 13
QA Contact: mwobensmith
Iteration: 42.1 - Jul 13 → 42.2 - Jul 27
Points: 8 → 5
Summary: Move Mixed Content Blocking into Control Center → Move Mixed Content Blocking labels and controls into Control Center
Blocks: 1184103
Blocks: 1183609
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
Blocks: 1184312
This patch implements all security and mixed content blocking states. Instead of using the existing CSS classes it uses a secstate="" attribute that has a unique value for each possible state. All elements in the popup have secstate="" attribute that here contains a space-separated list of states this element will be visible for.

For example, with #identity-popup[secstate=chrome] an element having secstate="chrome secure secure-ev" will be visible. It would also be visible for "secure" and "secure-ev".

I would really like to disentangle that popup code from the gIdentityHandler in the future, that would make the code a tad more readable.
Attachment #8638525 - Flags: review?(paolo.mozmail)
Attached file screenshots.zip (obsolete) —
For reference, this zip contains all the different states. All states except "chrome" have two pictures where the first shows the main view, the second the extended subview.

1) [chrome] about:home

A whitelisted page this is reported as being a "secure Firefox page".

2) [insecure] http://www.golem.de/

An unauthenticated page served via http://.

3) [secure] https://timtaubert.de/

An authenticated page using a DV certificate.

4) [secure-ev] https://www.mozilla.org/

An authenticated page using an EV certificate.

5) [weak-cipher] https://rc4.badssl.com/

A page using RC4 for transport encryption. Nightly needs security.tls.unrestricted_rc4_fallback=true to load this page at all.

6) [active-blocked-ev] https://people.mozilla.org/~tvyas/mixedcontent.html

An authenticated page using an EV certificate, trying to load mixed active content.

7) [active-blocked] https://www.maxcdn.com/one/tutorial/solving-mixed-content/

An authenticated page using a DV certificate, trying to load mixed active content.

8) [passive-loaded] https://people.mozilla.org/~tvyas/mixeddisplay.html

An authenticated page (DV or EV doesn't matter) that has mixed display content loaded.

9) [passive-loaded-active-blocked] https://people.mozilla.org/~tvyas/mixedboth.html

An authenticated page (DV or EV doesn't matter) that has mixed display content loaded, trying to load mixed active content.

10 [active-loaded] https://people.mozilla.org/~tvyas/mixedcontent.html (MCB DISABLED)

An authenticated page (DV or EV doesn't matter) that has mixed active content loaded. This requires user action to explicitly disable MCB for the current site and session.


Ash, can you please take a look and check whether I misssed anything? I double-checked all states but the whole set is rather complex :)
Flags: needinfo?(agrigas)
Coordinated with Ash and I'll make a few updates to the style, but happy to hear feedback about the general approach.
Will have to do one more pass with some small alignment fixes.
Attachment #8638525 - Attachment is obsolete: true
Attachment #8638525 - Flags: review?(paolo.mozmail)
reviewed
Flags: needinfo?(agrigas)
(In reply to Tim Taubert [:ttaubert] from comment #5)
> 5) [weak-cipher] https://rc4.badssl.com/
> 
> 8) [passive-loaded] https://people.mozilla.org/~tvyas/mixeddisplay.html
> 
> 9) [passive-loaded-active-blocked]
> https://people.mozilla.org/~tvyas/mixedboth.html

Hi Tim,

For these three cases, the subpanel has two paragraphs.  The second paragraph is indented with the triangle icon and the first is not.  Since the information in the paragraph is all relevant to why the triangle icon is present on the site, perhaps both paragraphs should be indented next to the triangle?  Is there a reason to only indent one?

Also the way the weak cipher case probably needs different text (since it has the old text we used for mixed content blocker and should be updated to be consistent with the new copy).
(In reply to Tanvi Vyas out until 7-24 [:tanvi] from comment #9)
> Also the way the weak cipher case probably needs different text (since it
> has the old text we used for mixed content blocker and should be updated to
> be consistent with the new copy).

Yeah, indeed. We're planning to check and synchronize all strings one last time after this lands. It might be easier to use the same wording when have and see all different states at once.

Also, I came up with the text on the main view, I'm sure we can improve this one too :)
(In reply to Tanvi Vyas out until 7-24 [:tanvi] from comment #9)
> For these three cases, the subpanel has two paragraphs.  The second
> paragraph is indented with the triangle icon and the first is not.  Since
> the information in the paragraph is all relevant to why the triangle icon is
> present on the site, perhaps both paragraphs should be indented next to the
> triangle?  Is there a reason to only indent one?

We started having the first paragraph indented, now it's the second. Indenting both with a yield sign might look not so great, but I'll defer to Ash on that.
Flags: needinfo?(agrigas)
Two yield signs, each for every paragraph: http://i.imgur.com/8sDlXEz.png
Okay, I've taken some time to look at the patch and in the end I'd say I'm not a fan of a single secstate attribute with unique values. It's in fact analogous to the split values of "this._mode" but changing the side where the logic is between JS and CSS. But let's work iteratively, landing a version of this patch and filing new bugs to refactor code later.

In the meantime there's a simple "20 minutes" refactoring we can do now to improve simplicity later. This code in the end is just a waterfall of update functions with a single entry point that is checkIdentity. Data is eventually and unconditionally stored in this block at various points in the waterfall:

  _lastStatus : null,
  _lastUri : null,
  _mode : "unknownIdentity",

So, let's change this block to read:

  _uri : null,
  _sslStatus : null,
  _mode : "",
  _secState : "",

Let's move all the assignments to the above to checkIdentity. Strip all the arguments from setMode and friends. Have two main internal UI refresh functions:

* refreshIdentityBox called only from checkIdentity
* refreshIdentityPopup called from checkIdentity (if panel open) and handleIdentityButtonEvent

Use as many helpers (like updateSitePermissions) as necessary for readability and logic.

File a bug to split/join this._mode and this._secState into as many state variables as needed to keep the logic clear, all initialized in checkIdentity.
(In reply to Tim Taubert [:ttaubert] from comment #11)
> We started having the first paragraph indented, now it's the second.
> Indenting both with a yield sign might look not so great, but I'll defer to
> Ash on that.

Or, collapse the wording into just one paragraph. There's indeed some redundancy there.
(In reply to Tim Taubert [:ttaubert] from comment #12)
> Two yield signs, each for every paragraph: http://i.imgur.com/8sDlXEz.png

I recommend just using the yield for the bottom paragraph as we do in the other states. There is no need for 2 yield signs. I think the copy seems clear and each paragraph has different information so I think Tim's screenshot is sufficient.
Flags: needinfo?(agrigas)
Iteration: 42.2 - Jul 27 → 42.3 - Aug 10
(In reply to agrigas from comment #15)
> (In reply to Tim Taubert [:ttaubert] from comment #12)
> > Two yield signs, each for every paragraph: http://i.imgur.com/8sDlXEz.png
> 
> I recommend just using the yield for the bottom paragraph as we do in the
> other states. There is no need for 2 yield signs. I think the copy seems
> clear and each paragraph has different information so I think Tim's
> screenshot is sufficient.

I don't think we should have two yield signs, I just think that the single yield sign should be used to indent both paragraphs since they are both related to mixed content.
(In reply to agrigas from comment #15)
> (In reply to Tim Taubert [:ttaubert] from comment #12)
> > Two yield signs, each for every paragraph: http://i.imgur.com/8sDlXEz.png
> 
> I recommend just using the yield for the bottom paragraph as we do in the
> other states. There is no need for 2 yield signs. I think the copy seems
> clear and each paragraph has different information so I think Tim's
> screenshot is sufficient.

I don't think two yield signs are necessary, but since both paragraphs relate to mixed content I think they should both be indented.
Working through updating MCB tests in Bug 1186925 with the rebased patch here I noticed that a todo() statement regarding isMixedContentBlocked and redirects [0] (Bug 914860) seemed to be fixed from this.  Tagging it as blocked so we can double check after this lands

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug906190.js?from=browser_bug906190.js#501
Blocks: 914860, 1186925
No longer blocks: 914860
(In reply to Brian Grinstead [:bgrins] from comment #19)
> Working through updating MCB tests in Bug 1186925 with the rebased patch
> here I noticed that a todo() statement regarding isMixedContentBlocked and
> redirects [0] (Bug 914860) seemed to be fixed from this.

That was wrong, sorry for the false alarm
Aislinn and Tanvi settled on sticking with the current paragraph design. We might merge paragraphs in the future but that will require copy/legal review once more, so not now.
Attachment #8642451 - Attachment is obsolete: true
Attachment #8643230 - Flags: review?(paolo.mozmail)
Attachment #8638532 - Attachment is obsolete: true
Depends on: 1191044
Comment on attachment 8643230 [details] [diff] [review]
0001-Bug-1175702-Implement-mixed-content-states-in-the-co.patch, v3

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

We're 90% done on this - worth going the extra mile.

::: browser/base/content/browser.js
@@ -6957,1 @@
>      }

I guess we still need to call refreshIdentityPopup in checkIdentity if the popup is open?

@@ +7039,5 @@
> +      mixedcontent.push("active-blocked");
> +    }
> +    mixedcontent = mixedcontent.join(" ");
> +
> +    // We have no specific flags for weak ciphers (yet). If a connection is

Might be worth annotating this with a bug number.

@@ +7087,1 @@
>        verifier = this._identityBox.tooltipText;

pre-existing nit: This is information we should save in the object instead of retrieving again from the tooltip text. I guess we could improve this in bug 1191044.

@@ +7116,5 @@
>      // Hide subviews when updating panel information.
>      document.getElementById("identity-popup-multiView").showMainView();
>    },
>  
> +  _isFileURI(uri) {

nit: _isURILoadedFromFile?

@@ +7166,2 @@
>  
>      this.updateSitePermissions();

nit: Can you move the updateSitePermissions call at the end of refreshIdentityPopup? It's just another section of the control center, like Tracking Protection.

::: browser/components/controlcenter/content/panel.inc.xul
@@ +23,5 @@
>          <vbox id="identity-popup-security-content" flex="1">
> +          <label observes="identity-popup-content-host"/>
> +          <description class="identity-popup-connection-not-secure"
> +                       value="&identity.connectionNotSecure;"
> +                       connection="not-secure"/>

I know it's tedious, but can you rename these attributes to when-connection="", when-mixedcontent="", when-ciphers=""? Alternatively rename the reference attributes to something like "reference-connection", "reference-mixedcontent", and so on, though I think the first option is clearer.

Given in particular that we set the reference attributes on multiple elements, it might make it more difficult to understand what the CSS rules do later, if two conceptually different attributes (the reference ones and those that control visibility) have the same name.

@@ +26,5 @@
> +                       value="&identity.connectionNotSecure;"
> +                       connection="not-secure"/>
> +          <description class="identity-popup-connection-secure"
> +                       value="&identity.connectionSecure;"
> +                       connection="secure"/>

Okay, so after looking at the CSS rules I think they should be less customized and use simpler matching with just the tilde selector.

For example this would read when-connection="secure secure-ev". The CSS has a "starts with" selector now so it wasn't clear to me that this label was displayed for both cases.

@@ +45,3 @@
>          </vbox>
> +        <button id="identity-popup-security-expander"
> +                class="identity-popup-expander"

You could use a when-connection="not-secure secure secure-ev" rule for this button.

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +5,5 @@
> +%endif
> +
> +/* Hide connection labels by default. */
> +#identity-popup-security-content > description[connection],
> +#identity-popup-securityView-header > description[connection],

I guess a single "description[when-connection]" rule would be as efficient, matching starts from the right anyways. Same for similar rules.

@@ +187,5 @@
>    margin: 2px 0 4px;
>    font-size: 150%;
>  }
>  
> +.identity-popup-gray-yield {

nit: Rename these classes to identity-popup-warning-gray and identity-popup-warning-yellow to match the file names.

@@ +241,5 @@
>  }
>  
> +#identity-popup-security-descriptions > description {
> +  margin-top: 6px;
> +  color: #636363;

Do we set the background color explicitly somewhere else? Both text and background must be either matching system colors or known theme colors, but not a mix of the two. We could freely use opacity though.
Attachment #8643230 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #23)
> ::: browser/base/content/browser.js
> @@ -6957,1 @@
> >      }
> 
> I guess we still need to call refreshIdentityPopup in checkIdentity if the
> popup is open?

I've no idea how Splinter chooses how much context to show. This was pointing to some removed code in setMode.
(In reply to :Paolo Amadini from comment #23)
> I guess we still need to call refreshIdentityPopup in checkIdentity if the
> popup is open?

As discussed on IRC, that's a deliberate change I forgot to point out. I will add a comment that says we do not update opened popups in case a page reloads or is redirected.
(In reply to :Paolo Amadini from comment #23)
> > +    mixedcontent = mixedcontent.join(" ");
> > +
> > +    // We have no specific flags for weak ciphers (yet). If a connection is
> 
> Might be worth annotating this with a bug number.

We don't really have a bug number yet. The next "weak cipher" data point will most likely be "SHA-1 signature somewhere in the cert chain", implemented in bug 1183718. Even that will not add a new flag as it's still okay to reuse it. The first patch introducing (a) new flag(s) will have to touch that code anyway...

> >        verifier = this._identityBox.tooltipText;
> 
> pre-existing nit: This is information we should save in the object instead
> of retrieving again from the tooltip text. I guess we could improve this in
> bug 1191044.

SGTM, added a comment to the bug.
> > +  _isFileURI(uri) {
> 
> nit: _isURILoadedFromFile?

Agreed, that's a better name for it.

> >      this.updateSitePermissions();
> 
> nit: Can you move the updateSitePermissions call at the end of
> refreshIdentityPopup? It's just another section of the control center, like
> Tracking Protection.

Yep, I remember thinking about that yesterday but looks like I forgot to do that.

> ::: browser/components/controlcenter/content/panel.inc.xul
> @@ +23,5 @@
> >          <vbox id="identity-popup-security-content" flex="1">
> > +          <label observes="identity-popup-content-host"/>
> > +          <description class="identity-popup-connection-not-secure"
> > +                       value="&identity.connectionNotSecure;"
> > +                       connection="not-secure"/>
> 
> I know it's tedious, but can you rename these attributes to
> when-connection="", when-mixedcontent="", when-ciphers=""? Alternatively
> rename the reference attributes to something like "reference-connection",
> "reference-mixedcontent", and so on, though I think the first option is
> clearer.

Will do.

> Given in particular that we set the reference attributes on multiple
> elements, it might make it more difficult to understand what the CSS rules
> do later, if two conceptually different attributes (the reference ones and
> those that control visibility) have the same name.

Fair enough.

> @@ +26,5 @@
> > +                       value="&identity.connectionNotSecure;"
> > +                       connection="not-secure"/>
> > +          <description class="identity-popup-connection-secure"
> > +                       value="&identity.connectionSecure;"
> > +                       connection="secure"/>
> 
> Okay, so after looking at the CSS rules I think they should be less
> customized and use simpler matching with just the tilde selector.
> 
> For example this would read when-connection="secure secure-ev". The CSS has
> a "starts with" selector now so it wasn't clear to me that this label was
> displayed for both cases.

Yeah, those selectors are fancy. Doesn't hurt to make that more clear indeed.

> @@ +45,3 @@
> >          </vbox>
> > +        <button id="identity-popup-security-expander"
> > +                class="identity-popup-expander"
> 
> You could use a when-connection="not-secure secure secure-ev" rule for this
> button.

Sure.

> ::: browser/themes/shared/controlcenter/panel.inc.css
> @@ +5,5 @@
> > +%endif
> > +
> > +/* Hide connection labels by default. */
> > +#identity-popup-security-content > description[connection],
> > +#identity-popup-securityView-header > description[connection],
> 
> I guess a single "description[when-connection]" rule would be as efficient,
> matching starts from the right anyways. Same for similar rules.

> > +.identity-popup-gray-yield {
> 
> nit: Rename these classes to identity-popup-warning-gray and
> identity-popup-warning-yellow to match the file names.

Will do.

> > +#identity-popup-security-descriptions > description {
> > +  margin-top: 6px;
> > +  color: #636363;
> 
> Do we set the background color explicitly somewhere else? Both text and
> background must be either matching system colors or known theme colors, but
> not a mix of the two. We could freely use opacity though.

Do you mean the font color? Changed it to "Graytext", that's what the other gray labels in the subview use as well.
Attachment #8643230 - Attachment is obsolete: true
Attachment #8643715 - Flags: review?(paolo.mozmail)
Comment on attachment 8643715 [details] [diff] [review]
0001-Bug-1175702-Implement-mixed-content-states-in-the-co.patch v4

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

Looks great :-)

::: browser/base/content/browser.js
@@ +7038,5 @@
> +    // Update all elements.
> +    let elementIDs = [
> +      "identity-popup",
> +      "identity-popup-securityView-body",
> +      "identity-popup-security-descriptions",

I couldn't find where attributes on "identity-popup-security-descriptions" are used, maybe it's a leftover?

Do we need to set attributes on "identity-popup-securityView-body" because CSS referencing just "identity-popup" as an ancestor doesn't work in that view?

@@ -7092,5 @@
>      this._identityPopupContentSupp.textContent = supplemental;
>      this._identityPopupContentVerif.textContent = verifier;
>  
> -    // Hide subviews when updating panel information.
> -    document.getElementById("identity-popup-multiView").showMainView();

Just ensure that after removing this showMainView() call the panel still opens on the main view when reopened after being closed.
Attachment #8643715 - Flags: review?(paolo.mozmail) → review+
(In reply to Tim Taubert [:ttaubert] from comment #26)
> We don't really have a bug number yet. The next "weak cipher" data point
> will most likely be "SHA-1 signature somewhere in the cert chain",
> implemented in bug 1183718. Even that will not add a new flag as it's still
> okay to reuse it. The first patch introducing (a) new flag(s) will have to
> touch that code anyway...

The main reason to have bug cross-references is that it's easy to forget to touch that code when working on the bug, and conversely if the bug ends up not touching the code, then later people might still wonder whether it was or intentional or not, if there's no way to see if the bug has been resolved. Not a big deal though.
FYI I noticed when working on Bug 1186925 that the tests browser/base/content/test/general/browser_identity_UI.js and browser/base/content/test/general/browser_bug590206.js fail with this patch applied
Comment on attachment 8643715 [details] [diff] [review]
0001-Bug-1175702-Implement-mixed-content-states-in-the-co.patch v4

Do we no longer need the fileuri mode?

diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
index fe4b23b..d421c91 100644
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -6621,22 +6621,20 @@ var gIdentityHandler = {
   IDENTITY_MODE_UNKNOWN                                : "unknownIdentity",  // No trusted identity information
   IDENTITY_MODE_USES_WEAK_CIPHER                       : "unknownIdentity weakCipher",  // SSL with RC4 cipher suite or SSL3
   IDENTITY_MODE_MIXED_DISPLAY_LOADED                   : "unknownIdentity mixedContent mixedDisplayContent",  // SSL with unauthenticated display content
   IDENTITY_MODE_MIXED_ACTIVE_LOADED                    : "unknownIdentity mixedContent mixedActiveContent",  // SSL with unauthenticated active (and perhaps also display) content
   IDENTITY_MODE_MIXED_DISPLAY_LOADED_ACTIVE_BLOCKED    : "unknownIdentity mixedContent mixedDisplayContentLoadedActiveBlocked",  // SSL with unauthenticated display content; unauthenticated active content is blocked.
   IDENTITY_MODE_MIXED_ACTIVE_BLOCKED                   : "verifiedDomain mixedContent mixedActiveBlocked",  // SSL with unauthenticated active content blocked; no unauthenticated display content
   IDENTITY_MODE_MIXED_ACTIVE_BLOCKED_IDENTIFIED        : "verifiedIdentity mixedContent mixedActiveBlocked",  // SSL with unauthenticated active content blocked; no unauthenticated display content
   IDENTITY_MODE_CHROMEUI                               : "chromeUI",         // Part of the product's UI
-  IDENTITY_MODE_FILE_URI                               : "fileURI",  // File path

-    } else {
-      // Create a channel for the sole purpose of getting the resolved URI
-      // of the request to determine if it's loaded from the file system.
-      let resolvedURI = NetUtil.newChannel({uri,loadUsingSystemPrincipal:true}).URI;
-      if (resolvedURI.schemeIs("jar")) {
-        // Given a URI "jar:<jar-file-uri>!/<jar-entry>"
-        // create a new URI using <jar-file-uri>!/<jar-entry>
-        resolvedURI = NetUtil.newURI(resolvedURI.path);
+        mode = this.IDENTITY_MODE_USES_WEAK_CIPHER;
       }
+    }
 
-      if (resolvedURI.schemeIs("file")) {
-        this.setMode(this.IDENTITY_MODE_FILE_URI);
-      } else {
-        this.setMode(this.IDENTITY_MODE_UNKNOWN);
-      }
(In reply to Tim Taubert [:ttaubert] from comment #25)
> (In reply to :Paolo Amadini from comment #23)
> > I guess we still need to call refreshIdentityPopup in checkIdentity if the
> > popup is open?
> 
> As discussed on IRC, that's a deliberate change I forgot to point out. I
> will add a comment that says we do not update opened popups in case a page
> reloads or is redirected.

+    // NOTE: We do NOT update the identity popup (the control center) when
+    // we receive a new security state. If the user opened the popup and looks
+    // at the provided information we don't want to suddenly change the panel
+    // contents.
+

Assume the user is at page https://a.com and see's the lock icon.  They click the lock and get the popup for control center saying the connection is secure.  While the popup is open, http://a.com tries to load a mixed image.  We receive a new security state change, but we ignore it for the popup since we don't want to change contents of the popup while the user is reading it.  Even though the popup doesn't change, does the icon in the urlbar change?

If the user closes the popup and then reopens it, will they see the updated information given the new security state?

Thanks!
(In reply to Tanvi Vyas [:tanvi] from comment #31)
> Do we no longer need the fileuri mode?

We don't use the CSS class for styling anymore, but this is still one of the connection types.
(In reply to Tanvi Vyas [:tanvi] from comment #32)
> Even though the popup doesn't change, does the icon in the urlbar change?
> 
> If the user closes the popup and then reopens it, will they see the updated
> information given the new security state?

Short answer: yes and yes. Thanks for checking! Do you have any concerns with this behavior?
(In reply to Brian Grinstead [:bgrins] from comment #30)
> FYI I noticed when working on Bug 1186925 that the tests
> browser/base/content/test/general/browser_identity_UI.js and
> browser/base/content/test/general/browser_bug590206.js fail with this patch
> applied

Yes, knew about the second one. Thanks for letting me know :)
Comment on attachment 8643715 [details] [diff] [review]
0001-Bug-1175702-Implement-mixed-content-states-in-the-co.patch v4

It is also possible that mixed passive content is blocked.

+    // Determine the mixed content state.
+    let mixedcontent = [];
+    if (this._state & Ci.nsIWebProgressListener.STATE_LOADED_MIXED_DISPLAY_CONTENT) {
+      mixedcontent.push("passive-loaded");
+    }
Add:
     else if (this._state & Ci.nsIWebProgressListener.STATE_BLOCKED_MIXED_DISPLAY_CONTENT) {
      mixedcontent.push("passive-blocked");
    }


+    if (this._state & Ci.nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT) {
+      mixedcontent.push("active-loaded");
+    } else if (this._state & Ci.nsIWebProgressListener.STATE_BLOCKED_MIXED_ACTIVE_CONTENT) {
+      mixedcontent.push("active-blocked");
+    }
+    mixedcontent = mixedcontent.join(" ");
(In reply to Tanvi Vyas [:tanvi] from comment #31)
> Do we no longer need the fileuri mode?

It's still there, we just don't need it as a "mode", it doesn't affect the url bar. It's just a state of the new connection="" attribute now.
(In reply to Tanvi Vyas [:tanvi] from comment #36)
> It is also possible that mixed passive content is blocked.

Right.

> Add:
>      else if (this._state &
> Ci.nsIWebProgressListener.STATE_BLOCKED_MIXED_DISPLAY_CONTENT) {
>       mixedcontent.push("passive-blocked");
>     }

How does that affect the visual state? We never had any mockups for that...
Comment on attachment 8643715 [details] [diff] [review]
0001-Bug-1175702-Implement-mixed-content-states-in-the-co.patch v4

+    // We have no specific flags for weak ciphers (yet). If a connection is
+    // broken and we can't detect any mixed content then it's a weak cipher.
+    let ciphers = "";
+    if (isBroken && !mixedcontent) {
+      ciphers = "weak";
+    }
+

This isn't quite right.  If mixed active content is blocked and the state is broken, you may be on a page that has weak encryption.  Same if mixed passive is blocked.  What you want to check here is if active-loaded or passive-loaded.


+          <vbox id="identity-popup-security-descriptions">
+            <description class="identity-popup-warning-gray"
+                         when-mixedcontent="active-blocked">&identity.activeBlocked;</description>
+            <description class="identity-popup-warning-yellow"
+                         when-mixedcontent="passive-loaded">&identity.passiveLoaded;</description>
+            <description when-mixedcontent="active-loaded">&identity.activeLoaded;</description>
+            <description class="identity-popup-warning-yellow"
+                         when-ciphers="weak">&identity.weakEncryption;</description>

What happens if you have both passive-loaded and active-loaded (we should show the activeLoaded string)?  What happens if you have passive-loaded and active-blocked (passive loaded text)?  Do you need descriptions for these states too?
+<!-- Strings for connection state warnings. -->
+<!ENTITY identity.activeBlocked "&brandShortName; has blocked parts of this page that are not secure.">
+<!ENTITY identity.passiveLoaded "Parts of this page are not secure (such as images).">
+<!ENTITY identity.activeLoaded "You have disabled protection on this page.">
+<!ENTITY identity.weakEncryption "This page uses weak encryption.">
(In reply to Tanvi Vyas [:tanvi] from comment #39)
> +    // We have no specific flags for weak ciphers (yet). If a connection is
> +    // broken and we can't detect any mixed content then it's a weak cipher.
> +    let ciphers = "";
> +    if (isBroken && !mixedcontent) {
> +      ciphers = "weak";
> +    }
> +
> 
> This isn't quite right.  If mixed active content is blocked and the state is
> broken, you may be on a page that has weak encryption.  Same if mixed
> passive is blocked.  What you want to check here is if active-loaded or
> passive-loaded.

Oh, indeed. Thanks for spotting this!

> What happens if you have both passive-loaded and active-loaded (we should
> show the activeLoaded string)?  What happens if you have passive-loaded and
> active-blocked (passive loaded text)?  Do you need descriptions for these
> states too?

"active blocked" is overridden by "passive loaded", is overridden by "active loaded". Those are all covered.
(In reply to Tim Taubert [:ttaubert] from comment #38)
> (In reply to Tanvi Vyas [:tanvi] from comment #36)
> > It is also possible that mixed passive content is blocked.
> 
> Right.
> 
> > Add:
> >      else if (this._state &
> > Ci.nsIWebProgressListener.STATE_BLOCKED_MIXED_DISPLAY_CONTENT) {
> >       mixedcontent.push("passive-blocked");
> >     }
> 
> How does that affect the visual state? We never had any mockups for that...

We don't have mocks for this.  passive-blocked doesn't trigger any UI currently, but it should.  This was first part of bug 893533 which I fixed but was backed out because of failing tests I didn't have a chance to look at yet.  The changes to control center will surely bitrot that patch and I know I'll have to start from scratch.

For now, adding "passive-blocked" would just ensure that the "mixedcontent" variable is properly populated.  And then later we could determine what the UI should look like for that case (likely identical to the mixed active content blocked state).  But adding "passive-blocked" to "mixedcontent" may require a bit more code complexity right now, since you will have to check for more combinations of mixedcontent states.  And since we are running out of time, lets just leave that as a followup that we can do in bug 893533.
(In reply to Tanvi Vyas [:tanvi] from comment #41)
> (In reply to Tim Taubert [:ttaubert] from comment #38)
> > (In reply to Tanvi Vyas [:tanvi] from comment #36)
> > > Ci.nsIWebProgressListener.STATE_BLOCKED_MIXED_DISPLAY_CONTENT) {
> > >       mixedcontent.push("passive-blocked");
> > >     }
> > 
> > How does that affect the visual state? We never had any mockups for that...
> 
> We don't have mocks for this.  passive-blocked doesn't trigger any UI
> currently, but it should.  This was first part of bug 893533 which I fixed
> but was backed out because of failing tests I didn't have a chance to look
> at yet.  The changes to control center will surely bitrot that patch and I
> know I'll have to start from scratch.

Yes, I saw your comment there and really appreciate you waiting until this big patch here landed :)

> For now, adding "passive-blocked" would just ensure that the "mixedcontent"
> variable is properly populated.  And then later we could determine what the
> UI should look like for that case (likely identical to the mixed active
> content blocked state).  But adding "passive-blocked" to "mixedcontent" may
> require a bit more code complexity right now, since you will have to check
> for more combinations of mixedcontent states.  And since we are running out
> of time, lets just leave that as a followup that we can do in bug 893533.

I can add that state and just ignore it, or we could introduce it with bug 893533?
(In reply to Tim Taubert [:ttaubert] from comment #42)
> (In reply to Tanvi Vyas [:tanvi] from comment #41)
> > For now, adding "passive-blocked" would just ensure that the "mixedcontent"
> > variable is properly populated.  And then later we could determine what the
> > UI should look like for that case (likely identical to the mixed active
> > content blocked state).  But adding "passive-blocked" to "mixedcontent" may
> > require a bit more code complexity right now, since you will have to check
> > for more combinations of mixedcontent states.  And since we are running out
> > of time, lets just leave that as a followup that we can do in bug 893533.
> 
> I can add that state and just ignore it, or we could introduce it with bug
> 893533?

I'll leave this for bug 893533 to not introduce any unwanted state for now. That pref is off by default anyway.
(In reply to :Paolo Amadini from comment #28)
> > +      "identity-popup",
> > +      "identity-popup-securityView-body",
> > +      "identity-popup-security-descriptions",
> 
> I couldn't find where attributes on "identity-popup-security-descriptions"
> are used, maybe it's a leftover?

Yes, sweet!

> Do we need to set attributes on "identity-popup-securityView-body" because
> CSS referencing just "identity-popup" as an ancestor doesn't work in that
> view?

No, that works fine. I do have few special rules in the CSS for elements contained in there so it's easy to be more specific and override display:inherit when needed.

> > -    // Hide subviews when updating panel information.
> > -    document.getElementById("identity-popup-multiView").showMainView();
> 
> Just ensure that after removing this showMainView() call the panel still
> opens on the main view when reopened after being closed.

Yes, I did ensure that. I added the call when we updated an opened popup so we don't need that anymore.
All feedback applied. r=paolo (and r=tanvi maybe too I guess :)
Attachment #8643715 - Attachment is obsolete: true
Attachment #8644287 - Flags: review+
Will work on fixing the two tests next.
This patch fixes browser_identity_UI.js, but more importantly brings back the case where "uri.host" throws. That got unintentionally lost when rewriting the first version of this patch. Yay, tests.
Attachment #8644307 - Flags: review?(paolo.mozmail)
Looks like I pulled the requirement that uri.host must not exist for "file" connections out of my hat. This isn't how we implemented it before and also fixes the another test.
Attachment #8644318 - Flags: review?(paolo.mozmail)
Comment on attachment 8644307 [details] [diff] [review]
0002-Bug-1175702-Fix-browser_identity_UI.js-failures.patch

I did notice that the uri.host check was removed but just thought it was a simplification resulting from the refactoring. Yes, good we have tests!

The "can't do anything useful here" comment isn't quite true, what we're doing in this case is useful, isn't it? :-) We do handle other cases where the host is ignored like "chrome" and "file", so maybe we should specify in the comment the expected result for URIs without a host.

By the way, there's some code special-casing the "jar:" protocol later, in the code that determines if a URI is loaded locally. How does that interact with this check here?
Attachment #8644307 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8644318 [details] [diff] [review]
0003-Bug-1175702-Fix-browser_bug590206.js-failures.patch

Makes sense, but I'd still like to know what is the expected result for the "jar:http:..." and "jar:file:..." cases (I'm not sure we can load the former at all).
Attachment #8644318 - Flags: review?(paolo.mozmail) → review+
(In reply to :Paolo Amadini from comment #49)
> The "can't do anything useful here" comment isn't quite true, what we're
> doing in this case is useful, isn't it? :-)

Oh, right. That was mostly copied from what it was before, definitely good to correct that.

> We do handle other cases where
> the host is ignored like "chrome" and "file", so maybe we should specify in
> the comment the expected result for URIs without a host.

Will clarify in the comment.

> By the way, there's some code special-casing the "jar:" protocol later, in
> the code that determines if a URI is loaded locally. How does that interact
> with this check here?

The URL bar will itself always show non-network URLs as an unidentified connection. The control center then later goes and decides whether to tell the user that it's an "internal" resource, stored on the computer.
(In reply to :Paolo Amadini from comment #50)
> Makes sense, but I'd still like to know what is the expected result for the
> "jar:http:..." and "jar:file:..." cases (I'm not sure we can load the former
> at all).

jar:http://mozilla.org/example.jar!/ is a secure connection, which is fair because it's redirected to https://mozilla.org. It doesn't load the content but the connection is secure :)

jar:file:///system/b2g/omni.ja!/modules/ril_worker.js simply redirects to file:// internally and is thus a resource stored on your computer.

Both of those cases should be fine.
Depends on: 1192056
This is causing the Control Center UI to break (see Bug 1192056).  It looks like this is missing some [when-connection] [when-cipher] and [when-mixedcontent] selectors from the CSS, so all messages are always showing up.  I tried to come up with a quick fix but it's showing all of the mixedcontent messages in the subview.

I'll attach my WIP patch for that, but in the meantime I think the best thing to do it to back this out so it doesn't land in nightly and break the control center.
Flags: needinfo?(ttaubert)
This is as far as I got towards a fix.  I ran into an issue where the [when-mixedcontent~="passive-loaded"] wasn't working for the warnings in the subview because I think we really only wanted certain warnings to show up when "active-blocked passive-loaded" for instance.  But if I just did [when-mixedcontent="passive-loaded"] I think it was maybe not showing up in a case where we wanted it to be.
Whoops, didn't catch the backout before I merged.

remote:   https://hg.mozilla.org/mozilla-central/rev/4fc077aa4dc0
remote:   https://hg.mozilla.org/mozilla-central/rev/e78c32436369
remote:   https://hg.mozilla.org/mozilla-central/rev/40d5e458c162
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 42 → ---
I didn't notice a merge conflict when applying the git patches in my push-only fx-team copy. That's what I get for pushing stuff late at night.
Flags: needinfo?(ttaubert)
Depends on: 1192162
Depends on: 1192214
https://hg.mozilla.org/mozilla-central/rev/3b01a9b09305
https://hg.mozilla.org/mozilla-central/rev/d297c4095088
https://hg.mozilla.org/mozilla-central/rev/8ad569af634d
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Verified as fixed on:

FF 42
Build Id:20150811030206
OS: Win 7 x64, Ubuntu 12.04 x86, Mac Os X 10.10.4
Status: RESOLVED → VERIFIED
Depends on: 1193742
Blocks: 1193849
Depends on: 1194258
You need to log in before you can comment on or make changes to this bug.