Control Center and location bar don't agree on mixed content state with iframes

VERIFIED FIXED in Firefox 42

Status

()

P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: ttaubert, Assigned: bgrins)

Tracking

Trunk
Firefox 43
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox42+ verified, firefox43 verified)

Details

(Whiteboard: [fxprivacy], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
When an http page includes an https iframe that includes another http iframe then the state in the location bar and the control center don't agree.

http://people.mozilla.org/~tvyas/mixedgrandiframe.html
Flags: qe-verify+
Flags: firefox-backlog?
(Reporter)

Comment 1

3 years ago
The normal state seems fine:

http://i.imgur.com/CSgvP8j.png
http://i.imgur.com/KZl7Bdt.png

We show that the connection is not secure but also notify the user that we blocked parts of the page. There is a button to unblock active content as well.

Not when the user decides to unblock it looks like this:

http://i.imgur.com/4AN1aNd.png
http://i.imgur.com/iM6SwC3.png

The location bar shows the globe for a non-secure connection, the control center however shows the "disabled mixed active content blocking" icon.

Tanvi, what's the desired state here? What would we do if there's passive content loaded in the https iframe?
Flags: needinfo?(tanvi)
(Reporter)

Comment 2

3 years ago
(In reply to Tim Taubert [:ttaubert] from comment #1)
> Not when the user decides to unblock it looks like this:

*Now when...

Updated

3 years ago
Whiteboard: [fxprivacy]

Updated

3 years ago
Blocks: 1188565
Flags: firefox-backlog? → firefox-backlog+
Priority: -- → P2

Comment 3

3 years ago
I agree that the normal state looks fine.  After disable, it would be nice if we matched the globe in the location bar, but not a big deal if we don't.  We can wait until 1182551 lands and then handle this in Firefox 43 (or potentially uplift).

The mixed passive iframe is a bigger deal, because it shows a lock on an http page (http://people.mozilla.org/~tvyas/http_parent_with_mixed_passive_https_child.html).  Bug
https://bugzilla.mozilla.org/show_bug.cgi?id=1182551 takes care of this.  I just need to add a test.

Note that the changes in bug 1182551 may affect the behavior here and could potentially fix this bug too.  We could test to check if that is the case.
Flags: needinfo?(tanvi)
See Also: → bug 1182551
(Assignee)

Updated

3 years ago
Depends on: 1182551

Comment 4

3 years ago
The platform changes in bug 1182551 don't fix this bug and it is a bigger problem then I imagined before.  In the passive case, the control center and url bar don't agree either and you see this:
https://people.mozilla.org/~tvyas/lock_icon_on_http_page_panel.png
https://people.mozilla.org/~tvyas/lock_icon_on_http_page_subpanel.png

Putting the big lock on an http page is bad!  We should fix this for Firefox 42.
Target Milestone: --- → Firefox 42
(In reply to Tanvi Vyas [:tanvi] from comment #4)
> The platform changes in bug 1182551 don't fix this bug and it is a bigger
> problem then I imagined before.  In the passive case, the control center and
> url bar don't agree either and you see this:
> https://people.mozilla.org/~tvyas/lock_icon_on_http_page_panel.png
> https://people.mozilla.org/~tvyas/lock_icon_on_http_page_subpanel.png
> 
> Putting the big lock on an http page is bad!  We should fix this for Firefox
> 42.

Added to the agenda for Monday team meeting to discuss upgrading the priority level of the bug and which iteration development can happen.
(Assignee)

Comment 6

3 years ago
[Tracking Requested - why for this release]: See Comment 4
tracking-firefox42: --- → ?

Updated

3 years ago
Priority: P2 → P1
We're going to prioritize this one for 43 and plan on requesting uplift.
(Assignee)

Updated

3 years ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

Updated

3 years ago
Iteration: --- → 43.1 - Aug 24
QA Contact: mwobensmith
(Assignee)

Comment 8

3 years ago
Regarding Comment 4 and MCB in an https frame on an http page:

It looks like the reason the globe is (correctly) showing up in the URL bar is that we don't set any MCB state for it inside refreshIdentityBlock, but in refreshIdentityPopup we are adding the passive-loaded state to show the "Parts of this page are not secure (such as images)" text.

I think we need to add the MCB state attribute on identity-popup-securityView-body at all times to show the description text, but only add it to identity-popup if STATE_IS_BROKEN so that the globe shows up in this case.  We also need some test coverage for these cases.
(Assignee)

Comment 9

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #8)
> I think we need to add the MCB state attribute on
> identity-popup-securityView-body at all times to show the description text,
> but only add it to identity-popup if STATE_IS_BROKEN so that the globe shows
> up in this case.  We also need some test coverage for these cases.

I was mistaken about this - identity-popup-securityView-body is the subpanel element.  We may need to actually store 'isbroken' as an attribute on identity-popup so we know if we can switch the icon.

Comment 10

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #8)
> We also need some test coverage for these cases.
You may be able to extend the this test to check the panel icon as well as the url bar icon:
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_mixedContentFramesOnHttp.js

Comment 11

3 years ago
Actually, you could probably just build it into this method, that way every test that uses it will check both the url bar and the control center:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/head.js#966

Comment 12

3 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #11)
> Actually, you could probably just build it into this method, that way every
> test that uses it will check both the url bar and the control center:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> general/head.js#966

Sorry so many comments.  I was wrong here.  This is only checking the webprogress listener flags.  Not actually looking at what the UI is showing.
(Assignee)

Comment 13

3 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #12)
> (In reply to Tanvi Vyas [:tanvi] from comment #11)
> > Actually, you could probably just build it into this method, that way every
> > test that uses it will check both the url bar and the control center:
> > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> > general/head.js#966
> 
> Sorry so many comments.  I was wrong here.  This is only checking the
> webprogress listener flags.  Not actually looking at what the UI is showing.

I was thinking of modifying the assertMixedContentBlockingState method to add some checks for things like computed background images / classes, and then also call that method from browser_mixedContentFramesOnHttp.js.  I think the MCB coverage has enough callers (including all variations of mixed content being present / active / disabled) that it would cover most states of these icons.
(Assignee)

Comment 14

3 years ago
Created attachment 8649544 [details]
MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN

Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi
Attachment #8649544 - Flags: review?(tanvi)
(Assignee)

Comment 15

3 years ago
Tanvi, sorry if the test changes are hard to follow.  I couldn't find anywhere else that we were directly testing this stuff so I had to come up with my best approximation of the right states for both the identity box and the CC based on STATE_IDENTITY_EV_TOPLEVEL, STATE_IS_SECURE, STATE_IS_BROKEN, along with the expected MCB state.  It's a little tricky to get right because refreshIdentityBlock and refreshIdentityPopup have different flows of logic.

I considered adding a new test function to just test identity and CC icons / state, but they are so tied in with MCB that I thought it was best to put it there.  Also assertMixedContentBlockingState is already being called from many tests and in many different security environments, so it should have fairly good coverage already.  Let me know if you think we should adjust the approach here.

The fix itself is to not change to the lock icon if STATE_IS_BROKEN does not exist and MCB is happening.  I believe this is accurately finding the problem case (http page with MCB happening in a subframe).

Comment 16

3 years ago
Comment on attachment 8649544 [details]
MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN

https://reviewboard.mozilla.org/r/16425/#review14749

::: browser/base/content/test/general/head.js:792
(Diff revision 1)
> +  if (!stateEV && !stateSecure && !stateBroken) {

Why not just check for the insecure state?  Or check for state_is_insecure in addition?

Also, we check for stateEV here but not in line 863.  I believe stateEV is included in stateSecure (i.e. if something has STATE_IDENTITY_EV_TOPLEVEL it also has STATE_IS_SECURE.  worth doublechecking).  So you might not need stateEV at all.

::: browser/base/content/test/general/head.js:798
(Diff revision 1)
> +    ok(!classList.contains("mixedActiveContent"), "No MCB icon on HTTP page");

I wanted to confirm that this is only testing the actual icon.  There are cases where we would have mixed active content blocked on an http page (http top level, https iframe with http active content - http://people.mozilla.org/~tvyas/mixedgrandiframe.html).  We offer the disable button in the control center and we'd like to keep that functionality.

The if/else clause on lines 792/803, make it look like a bunch of these tests are skipped for this scenario (state_is_insecure && state_blocked_mixed_active_content).

::: browser/base/content/test/general/head.js:870
(Diff revision 1)
> +      is(securityViewBG, "url(\"chrome://browser/skin/controlcenter/conn-not-secure.svg\")",

Are you sure this is the right icon for stateBroken and activeBlocked?  I think you want the grey lock with the yellow triangle in this case.  And also in the else if (passiveLoaded) case.
Attachment #8649544 - Flags: review?(tanvi)
(Assignee)

Comment 17

3 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #16)
> Comment on attachment 8649544 [details]
> MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for
> http pages that have https frames with MCB;r=tanvi
> 
> https://reviewboard.mozilla.org/r/16425/#review14749
> 
> ::: browser/base/content/test/general/head.js:792
> (Diff revision 1)
> > +  if (!stateEV && !stateSecure && !stateBroken) {
> 
> Why not just check for the insecure state?  Or check for state_is_insecure
> in addition?
> 
> Also, we check for stateEV here but not in line 863.  I believe stateEV is
> included in stateSecure (i.e. if something has STATE_IDENTITY_EV_TOPLEVEL it
> also has STATE_IS_SECURE.  worth doublechecking).  So you might not need
> stateEV at all.
> 
> ::: browser/base/content/test/general/head.js:798
> (Diff revision 1)
> > +    ok(!classList.contains("mixedActiveContent"), "No MCB icon on HTTP page");
> 
> I wanted to confirm that this is only testing the actual icon.  There are
> cases where we would have mixed active content blocked on an http page (http
> top level, https iframe with http active content -
> http://people.mozilla.org/~tvyas/mixedgrandiframe.html).  We offer the
> disable button in the control center and we'd like to keep that
> functionality.

Yes, this is just testing the class on the identity box (the element in the URL bar).  These classes just control that icon so you will see the globe in the URL bar in the http page regardless of MCB state.  The CC will still show the warning and subview controls, and with this patch applied you will also see the globe as the icon in the CC.
(Assignee)

Comment 18

3 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #16)
> Comment on attachment 8649544 [details]
> MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for
> http pages that have https frames with MCB;r=tanvi
> 
> https://reviewboard.mozilla.org/r/16425/#review14749
> 
> ::: browser/base/content/test/general/head.js:792
> (Diff revision 1)
> > +  if (!stateEV && !stateSecure && !stateBroken) {
> 
> Why not just check for the insecure state?  Or check for state_is_insecure
> in addition?
> 
> Also, we check for stateEV here but not in line 863.  I believe stateEV is
> included in stateSecure (i.e. if something has STATE_IDENTITY_EV_TOPLEVEL it
> also has STATE_IS_SECURE.  worth doublechecking).  So you might not need
> stateEV at all.

You are right, I added stateInsecure and removed stateEV
(Assignee)

Comment 19

3 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #16)
> ::: browser/base/content/test/general/head.js:870
> (Diff revision 1)
> > +      is(securityViewBG, "url(\"chrome://browser/skin/controlcenter/conn-not-secure.svg\")",
> 
> Are you sure this is the right icon for stateBroken and activeBlocked?  I
> think you want the grey lock with the yellow triangle in this case.  And
> also in the else if (passiveLoaded) case.

This is for the CC, so the yellow triangle isn't part of the icon.  I don't think activeBlocked has any special state for the CC icon:

* isSecure && activeBlocked shows the green lock: https://people.mozilla.org/~tvyas/mixedcontent.html
* isBroken && activeBlocked shows the grey lock, but that's because of ciphers="weak" [0].  For example: https://people.mozilla.org/~tvyas/mixedboth.html.

And I think this has uncovered some missing test coverage, because even if I change this assertion to ok(false) it doesn't ever fail in test coverage.  Is there somewhere in the tests that should be catching this case -> (isBroken && activeBlocked)?

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7055-7061
Flags: needinfo?(tanvi)
(Assignee)

Comment 20

3 years ago
Comment on attachment 8649544 [details]
MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN

Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi
Attachment #8649544 - Flags: review?(tanvi)
(Assignee)

Comment 21

3 years ago
Created attachment 8650178 [details]
MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN

Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi
Attachment #8650178 - Flags: review?(tanvi)
(Assignee)

Comment 22

3 years ago
OK, after discussion I've updated the patch.  I took browser_mixedcontent_securityflags.js, converted it to add_task format, and added a case where {activeBlocked: true, passiveLoaded: true} by checking on the page before setting the security.mixed_content.block_display_content pref.

I also incorporated changes from the first round of reviews, but this does get complicated indeed so let me know if you see anything else that looks out of place.

Comment 23

3 years ago
Comment on attachment 8649544 [details]
MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN

https://reviewboard.mozilla.org/r/16425/#review14829

::: browser/base/content/test/general/browser_mixedcontent_securityflags.js:5
(Diff revision 2)
> -// on it while the "block mixed content" settings are _on_.
> +// and makes sure that mixed content flags have been set correctly.

The original text here is not great, so I'm going to attempt to give a better and more elaborate explanation that we can put in the comment instead:

The test loads a web page with mixed active and mixed display content and makes sure that the mixed content flags on the docshell are set correctly.
* Using default about:config prefs (mixed active blocked, mixed display loaded) we load the page and check the flags.
* We change the about:config prefs (mixed active blocked, mixed display blocked), reload the page, and check the flags again.
* We override protection so all mixed content can load and check the flags again.

::: browser/base/content/test/general/browser_mixedcontent_securityflags.js:13
(Diff revision 2)
> +const PREF_DISPLAY = "security.mixed_content.block_display_content";

You should explicity set what you want the prefs to be set to in this test, as one day the defaults might change (or they might change for a specific build) and we don't want to update every test at that point.

::: browser/base/content/test/general/browser_mixedcontent_securityflags.js:35
(Diff revision 2)
> +  // Turn "block mixed content" settings on and reload the page

Change to "Turn on mixed active and mixed display blocking and reload the page."
Attachment #8649544 - Flags: review?(tanvi)

Comment 24

3 years ago
https://reviewboard.mozilla.org/r/16425/#review14831

::: browser/base/content/test/general/head.js
(Diff revisions 1 - 2)
> -    if (activeBlocked || activeLoaded || passiveLoaded) {

Why remove this activeBlocked || activeLoaded || passiveLoaded case?

::: browser/base/content/test/general/head.js:895
(Diff revisions 1 - 2)
> -  } else {
> +    } else {

You could put the is(securityViewBG, "url(\"chrome://browser/skin/controlcenter/conn-degraded.svg\")","CC using degraded icon"); and is(securityContentBG,...) tests here anyway.

Comment 25

3 years ago
Comment on attachment 8650178 [details]
MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN

https://reviewboard.mozilla.org/r/16559/#review14833

Ship It!
Attachment #8650178 - Flags: review?(tanvi) → review+

Comment 26

3 years ago
Clearing my needinfo because Brian and I discussed this over irc and he has new patches.
Flags: needinfo?(tanvi)
(Assignee)

Comment 27

3 years ago
Comment on attachment 8649544 [details]
MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN

Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi
Attachment #8649544 - Flags: review?(tanvi)
(Assignee)

Comment 28

3 years ago
Comment on attachment 8650178 [details]
MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN

Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi
(Assignee)

Comment 29

3 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #24)
> https://reviewboard.mozilla.org/r/16425/#review14831
> 
> ::: browser/base/content/test/general/head.js
> (Diff revisions 1 - 2)
> > -    if (activeBlocked || activeLoaded || passiveLoaded) {
> 
> Why remove this activeBlocked || activeLoaded || passiveLoaded case?
> 
> ::: browser/base/content/test/general/head.js:895
> (Diff revisions 1 - 2)
> > -  } else {
> > +    } else {
> 
> You could put the is(securityViewBG,
> "url(\"chrome://browser/skin/controlcenter/conn-degraded.svg\")","CC using
> degraded icon"); and is(securityContentBG,...) tests here anyway.

Oops, missed these two points before repushing.  Will add those two parts next
(Assignee)

Comment 30

3 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #24)
> https://reviewboard.mozilla.org/r/16425/#review14831
> 
> ::: browser/base/content/test/general/head.js
> (Diff revisions 1 - 2)
> > -    if (activeBlocked || activeLoaded || passiveLoaded) {
> 
> Why remove this activeBlocked || activeLoaded || passiveLoaded case?

Double checked, and if condition has been removed, but it's just been changed to an assertion above:

    is (classList.contains("mixedContent"), activeBlocked || activeLoaded || passiveLoaded,
       "identityBox has expected class for mixed content");

This is actually a better check since it will fail on the case where none of those are true but the classList still contains mixedContent

Comment 31

3 years ago
Both patches look good aside from Comment 29.  You probably need a browser peer to review too.

(In reply to Brian Grinstead [:bgrins] from comment #30)
> (In reply to Tanvi Vyas [:tanvi] from comment #24)
> > https://reviewboard.mozilla.org/r/16425/#review14831
> > 
> > ::: browser/base/content/test/general/head.js
> > (Diff revisions 1 - 2)
> > > -    if (activeBlocked || activeLoaded || passiveLoaded) {
> > 
> > Why remove this activeBlocked || activeLoaded || passiveLoaded case?
> 
> Double checked, and if condition has been removed, but it's just been
> changed to an assertion above:
> 
>     is (classList.contains("mixedContent"), activeBlocked || activeLoaded ||
> passiveLoaded,
>        "identityBox has expected class for mixed content");
> 
> This is actually a better check since it will fail on the case where none of
> those are true but the classList still contains mixedContent

Will the classList contain mixedContent if mixed passive content is blocked?  I don't think it will (since I believe the frontend ignores that case for now), so you should be okay here.
(Assignee)

Updated

3 years ago
Attachment #8649544 - Attachment description: MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi → MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN
Attachment #8649544 - Flags: review?(MattN+bmo)
(Assignee)

Comment 32

3 years ago
Comment on attachment 8649544 [details]
MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN

Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN
(Assignee)

Comment 33

3 years ago
Comment on attachment 8650178 [details]
MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN

Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN
Attachment #8650178 - Attachment description: MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi → MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN
Attachment #8650178 - Flags: review?(MattN+bmo)

Updated

3 years ago
Attachment #8649544 - Flags: review?(tanvi) → review+
Comment on attachment 8649544 [details]
MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN

https://reviewboard.mozilla.org/r/16425/#review14953

::: browser/base/content/test/general/browser_mixedcontent_securityflags.js:18
(Diff revision 4)
> -  SpecialPowers.pushPrefEnv({"set": [["security.mixed_content.block_active_content", true],
> -                            ["security.mixed_content.block_display_content", true]]}, blockMixedContentTest);
> +
> +registerCleanupFunction(function() {

pushPrefEnv is actually preferred nowadays since it does cleanup for you. You could wrap it in a promise if you want and put it in a setup_… task.
Attachment #8649544 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8650178 [details]
MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN

https://reviewboard.mozilla.org/r/16559/#review14955

Ship It!
Attachment #8650178 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 37

3 years ago
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #35)
> browser/base/content/test/general/browser_mixedcontent_securityflags.js:18
> (Diff revision 4)
> > -  SpecialPowers.pushPrefEnv({"set": [["security.mixed_content.block_active_content", true],
> > -                            ["security.mixed_content.block_display_content", true]]}, blockMixedContentTest);
> > +
> > +registerCleanupFunction(function() {
> 
> pushPrefEnv is actually preferred nowadays since it does cleanup for you.
> You could wrap it in a promise if you want and put it in a setup_… task.

I spent some time looking into this, and it seems that flushPrefEnv isn't called in between the b-c tests.  So if I run:

  ./mach mochitest -f browser browser/base/content/test/general/ --start-at browser_mixedcontent_securityflags.js --end-at browser_no_mcb_on_http_site.js

And then log this in browser_no_mcb_on_http_site.js:

  console.log("Second test", Services.prefs.getBoolPref("security.mixed_content.block_display_content"), Services.prefs.getBoolPref("security.mixed_content.block_active_content"));

The prefs haven't been reset!

I believe this could be a bug in the existing 20 something usages of pushPrefEnv in browser/: https://dxr.mozilla.org/mozilla-central/search?q=path%3Abrowser%2F+pushprefenv&redirect=true&case=false&limit=60&offset=0 and we need to file a bug for the test harness to call flushPrefEnv() in between tests.  I'd also like if pushPrefEnv would return a Promise by default.  In any case, I'll file those bugs but am just going to push it without that function for now since this patch needs to be uplifted and I don't want to track that work also.
(Assignee)

Comment 39

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #37)
> (In reply to Matthew N. [:MattN] (behind on reviews) from comment #35)
> > pushPrefEnv is actually preferred nowadays since it does cleanup for you.
> > You could wrap it in a promise if you want and put it in a setup_… task.
> 
> I spent some time looking into this, and it seems that flushPrefEnv isn't
> called in between the b-c tests.

Filed Bug 1197266
(Assignee)

Comment 40

3 years ago
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #35)
> pushPrefEnv is actually preferred nowadays since it does cleanup for you.
> You could wrap it in a promise if you want and put it in a setup_… task.

Filed Bug 1197310 for the promise return value
https://hg.mozilla.org/mozilla-central/rev/c61cc794b84c
https://hg.mozilla.org/mozilla-central/rev/c9f61ebf2af5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 42 → Firefox 43
The globe in the location bar matches now with the control center window on:
http://people.mozilla.org/~tvyas/mixedgrandiframe.html
http://people.mozilla.org/~tvyas/http_parent_with_mixed_passive_https_child.html
Verified fixed FF 43.0a1 (2015-08-24) Win 7, Ubuntu 14.04, OS X 10.10.
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
(Assignee)

Comment 43

3 years ago
Comment on attachment 8649544 [details]
MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN

Approval Request Comment
[Feature/regressing bug #]: 1175702
[User impact if declined]: The control center shows a lock icon for an http page that includes an iframe with mixed content
[Describe test coverage new/current, TreeHerder]: Enhanced existing test coverage for all mixed content browser chrome tests, including a regression test for this case
[Risks and why]: Code changes are very small (1 line of JS and a few CSS selector updates in the Control Center) so the risk is mostly limited to changing which icons are visible in the Control Center
[String/UUID change made/needed]:
Attachment #8649544 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 44

3 years ago
Comment on attachment 8650178 [details]
MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN

Approval Request Comment
[Feature/regressing bug #]: 1175702
[User impact if declined]: The control center is adding a weak ciphers attribute when there is passive mixed content but no weak ciphers.  This doesn't change the visible icon because the CSS was working around this.  But if we need to do future uplifts for 42 campaign it'd be best to have 42 in the same state as 43 for these attributes.
[Describe test coverage new/current, TreeHerder]: assertMixedContentBlockingState handles this case, but weak ciphers don't yet have any b-c tests
[Risks and why]: Small risk, simple JS and CSS change.  The attribute change is limited to the visible icons / messages in the control center
[String/UUID change made/needed]:
Attachment #8650178 - Flags: approval-mozilla-aurora?
Comment on attachment 8649544 [details]
MozReview Request: Bug 1192162 - Show not-secure icon in Control Center for http pages that have https frames with MCB;r=tanvi,r=MattN

Patch has been in Nightly for a week. Seems safe for uplift to Aurora42.
Attachment #8649544 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8650178 [details]
MozReview Request: Bug 1192162 - Only set the ciphers attribute in the Control Center when no mixed content is detected;r=tanvi,r=MattN

Aurora42+
Attachment #8650178 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox42: ? → +
(In reply to Paul Silaghi, QA [:pauly] from comment #42)
> The globe in the location bar matches now with the control center window on:
> http://people.mozilla.org/~tvyas/mixedgrandiframe.html
> http://people.mozilla.org/~tvyas/http_parent_with_mixed_passive_https_child.
> html
> Verified fixed FF 43.0a1 (2015-08-24) Win 7, Ubuntu 14.04, OS X 10.10.
Verified fixed 42.0a2 (2015-09-02) Win 7
status-firefox42: fixed → verified
You need to log in before you can comment on or make changes to this bug.