840388 - Mixed Content Blocker blocks navigating from https to http on insecure pages

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Attachment: Check for blocked Mixed Content on about:addons

about:addons doesn't have a url bar, so the the design for the mixed content blocker won't work for that page.  Moreover, there really should never be mixed content on about:addons.

When about:addons detects a mixed content page, it displays this page: https://www.dropbox.com/s/4qddil0cjrnf1gl/20130211165245.png because of the code here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1927

When the Mixed Content Blocker is enabled, then nsMixedContentBlocker.cpp will be disabling the content load.  Hence the state will still be STATE_IS_SECURE and the onSecurityChange call will return and cause the browser to fail silently.

To fix this, I have updated the code to check if nsMixedContentBlocker.cpp has blocked a mixed content load.  If it has, extensions.js will call aRequest.cancel() instead of failing silently.

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: Check for blocked Mixed Content on about:addons v2

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 712743 [details] [diff] [review]
Check for blocked Mixed Content on about:addons v2

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

As mentioned on IRC, we should change the last sentence of the error page to say something about an issue with the page, rather than possibly an issue with the person's internet connection. Happy for that to eventually handled in a followup bug so it doesn't hold this up, since it's more of a just-in-case kind of situation that we hope won't happen - filed bug 840471.

Comment 3

[Tanvi Vyas[:tanvi]]

Pushed to try along with the patches in bug 834836: https://tbpl.mozilla.org/?tree=Try&rev=ac63fceb7e30

If everything looks good, I will land later tonight or tomorrow.

Comment 4

[Tanvi Vyas[:tanvi]]

Attachment: Check for blocked Mixed Content on about:addons v3

+      if (!(aState & Ci.nsIWebProgressListener.STATE_BLOCKED_MIXED_ACTIVE_CONTENT) ||
+          !(aState & Ci.nsIWebProgressListener.STATE_BLOCKED_MIXED_DISPLAY_CONTENT)) {

This should be an && not an ||.

New push to try: https://tbpl.mozilla.org/?tree=Try&rev=f0a382ea2080

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Tanvi Vyas [:tanvi] from comment #4)
> This should be an && not an ||.

*sigh* Yes. How hard would it be to write a test for this? :)

Comment 6

[Tanvi Vyas[:tanvi]]

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #5)
> (In reply to Tanvi Vyas [:tanvi] from comment #4)
> > This should be an && not an ||.
> 
> *sigh* Yes. How hard would it be to write a test for this? :)

Well, there is a test for this in browser_discovery.js.  The test fails without this change.

Comment 7

[Tanvi Vyas[:tanvi]]

> Well, there is a test for this in browser_discovery.js.  The test fails
> without this change.

And continues to fail even with this change.  The test fails before onSecurityChange is not called and the request is cancelled.

We start at https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_discovery.js#326 and we call clickLink with an http link and a callback.  clickLink invokes isLoading().

In clickLink, we create a load complete listener that invokes the callback.  Since the load never completes (load is blocked by nsMixedContentBlocker.cpp) the callback is never invoked and the test times out.

Before we had nsMixedContentBlocker, mixed content was detected on response instead of prior to load, which is why this worked before.

In order to fix this, I thought about checking if the first parameter to clickLink is link-http, but that won't quite work on it's own.  If it is a mixed content load and the pref for mixed content blocking is off, the test will pass as is.  If it is a mixed content load and the pref for mixed content blocking is on, nsMixedContentBlocker blocks the load before isLoading() is called.  In this case, isLoading should return an error and then we should invoke the callBack.

Mossop, Unfocused - suggestions on how I can update this test so that it calls onSecurityChange() before it calls isLoading and hence cancels the request before it starts?

Comment 8

[Dave Townsend [:mossop]]

So based on what you're saying I'm assuming that onStateChange is never called with STATE_IS_NETWORK & STATE_STOP in this case? Is it called at all and if so with what flags?

Comment 9

[Tanvi Vyas[:tanvi]]

The insecure content browser_discovery.js test is also breaking with the patch to bug 836951.  That bug does not turn the preference on, so it technically shouldn't interfere with how the about:addons code detects and cancels mixed content requests (http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1927).

The issue in the patch to bug 836951 seems to be with traversing up to the root docshell in nsSecureBrowserUIImpl::MapInternaltoExternalState():


  int32_t docShellType;
  // For content docShell's, the mixed content security state is set on the root docShell.
  if (NS_SUCCEEDED(docShell->GetItemType(&docShellType)) && docShellType == nsIDocShellTreeItem::typeContent) {
    nsCOMPtr<nsIDocShellTreeItem> docShellTreeItem(do_QueryInterface(docShell));
    nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot;
    docShellTreeItem->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot));
    NS_ASSERTION(sameTypeRoot, "No document shell root tree item from document shell tree item!");
    docShell = do_QueryInterface(sameTypeRoot);
    if (!docShell)
      return NS_OK;
  }

Commenting this code out, the test passes.  The original docShell passes the if statement.  Perhaps because it is typeContent inside a typeChrome docshell.

I've tried stepping through this with gdb, but I am still not sure what is going on.  Perhaps I should try attaching the developer tools debugger to the javascript code.

Comment 10

[Tanvi Vyas[:tanvi]]

When we traverse the docShell and set the mixed content State on the root docShell instead of the current docShell (as shown in comment 9), this test fails:

https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_discovery.js

because this code is never executed:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1842

because we cancel the request (instead of returning) here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1932

When we traverse up to the root docShell and find that mixed content is loaded, we change the Security State of the ROOT docShell to STATE_IS_BROKEN:
https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#304

Hence, the load is cancelled by the code in extensions.js.

Mossop - how do I account for this in the test so that I can land bug 836951 and fix a crash in Aurora and Nightly?  https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_discovery.js

(I'm still working on how to fix browser_discovery.js so that we can land bug 834836)

Comment 11

[Tanvi Vyas[:tanvi]]

For bug 834836, when the prefrence to block mixed actiev content is set to true, clicking an mixed content link will fail, and hence the test times out here: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_discovery.js#182

This happens before onSecurityChange is called in extensions.js (http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1927) so the onSecurityChange code can't fix this.

I've worked on this for quite a while and am still stuck on how to fix the insecure content tests on about:addons so that they can work with the nsMixedContentBlocker (or perhaps be replaced by nsMixedContentBlocker tests), so calling out for help from the jetpack/addons team.  Marking needinfo? for Mossop, and perhaps he or someone from his team can help.  Thanks!

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 713214 [details] [diff] [review]
Check for blocked Mixed Content on about:addons v3

If any content was blocked then aState is no longer STATE_IS_SECURE anyway so this patch doesn't really add anything.

Comment 13

[Tanvi Vyas[:tanvi]]

(In reply to Dave Townsend (:Mossop) from comment #12)
> Comment on attachment 713214 [details] [diff] [review]
> Check for blocked Mixed Content on about:addons v3
> 
> If any content was blocked then aState is no longer STATE_IS_SECURE anyway
> so this patch doesn't really add anything.

If content is blocked, aState can be STATE_IS_SECURE && STATE_BLOCKED_MIXED_ACTIVE_CONTENT or STATE_IS_SECURE && STATE_BLOCKED_MIXED_DISPLAY_CONTENT.

If STATE_IS_SECURE and mixed content was blocked, what is the right behavior?  Should we return or continue to cancel the request?  The patch will cancel the request if either STATE_BLOCKED_MIXED_ACTIVE_CONTENT or STATE_BLOCKED_MIXED_DISPLAY_CONTENT are set in addition to STATE_IS_SECURE.

Did you have any luck figuring out why the insecure content tests in browser_discovery.js are failing? Or why the mixed content loads are being treated as TYPE_SUBDOCUMENT when they shouldn't be frames?

Comment 14

[Dave Townsend [:mossop]]

Attachment: detect mixed content blocks and show the error pane

This is a complicated problem mainly caused by the fact that about:addons is loaded in a content docshell (bug 652736). Because of this the <browser> that displays the discovery content is often deemed to be a subdocument rather than a top-level content document because the two docshells are of the same type. We seem to have a few options here:

Switch to loading about:addons in a non-content docshell. This is hard since we allow loading from the url bar and so we'd have to swap docshells on the browser element somehow which I'm sure would totally break history. It'd probably also break the in-page history that we use currently.

Make it so the content policy checks for the loads in the <browser> use TYPE_DOCUMENT rather than TYPE_SUBDOCUMENT, that means the mixed content blocking stuff wouldn't be blocking main page loads here and when it blocked inner content it would do so by correctly sending onSecurityChange notifications to the <browser>'s progress listeners (they currently go to the top-level about:addons listeners). This is straightforward if there is a sane way to label the case where this should happen that nsDocShell can spot (http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#11528).

Detect the current content blocking messages that get sent to about:addons. I've done that in this patch and it largely works. The only problem is that once about:addons is flagged as having loaded unsafe content it is impossible to unflag that so once the error is shown for unsafe content you have to refresh about:addons to get hasMixedActiveContentBlocked back to false in order to load the discovery page again. If there were a way to manually revert that from JS then we could use this patch.

Finally we could just disable mixed content blocking for about:addons, I have a two line patch that does it using nsIDocShell.mixedContentChannel but it suffers from the same problem as above since hasMixedActiveContentLoaded gets set and so the <browser> never gets to see STATE_IS_SECURE again.

As far as I can see we can't fix this problem without more platform changes and I'm not sure which is the best approach right now. Bz, do you have any comments on the first two options I mentioned?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Switch to loading about:addons in a non-content docshell.

I would love it if we could do this in general.  You're right that doing it in the same tab as existing content would be a pain; loading about:addons from the url bar would need to open a new tab (and loading something else in that tab would need to either effectively act as a replace load or once again open a new tab).  I agree that makes for sucky UI.  It might be possible to not affect in-page history with some platform changes to make the docshell involved a "history root" or something... But yes, if done naively I believe it too would break.

This approach is not usable as a quick fix, and is a bit open-ended in terms of how much work it will take...

> Make it so the content policy checks for the loads in the <browser> use TYPE_DOCUMENT
> rather than TYPE_SUBDOCUMENT

This seems eminently doable if we really want to, as you say.  We could just add an explicit flag on the docshell that the script here can set or something.  Basically similar to how <iframe mozbrowser> works anyway.

Comment 16

[Tanvi Vyas[:tanvi]]

Attachment: Treat privileged <browser> code as TYPE_DOCUMENT v1

r? to bz.  bz, if you think it's better to pass this to smaug, let me know, but I figured since you were already consulting on the bug, I would r? it to you.

Mossop, please also take a look.  The browser_discovery tests all succeed with this patch (with the patches that broke the tests also attached).  I will do a push to try to see if this breaks anything.  Thanks!

Comment 17

[Tanvi Vyas[:tanvi]]

Pushed to try, along with the patches in Bug 836951 and Bug 834836:
https://tbpl.mozilla.org/?tree=Try&rev=3b9e02950972

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 720932 [details] [diff] [review]
Treat privileged <browser> code as TYPE_DOCUMENT v1

This patch makes no sense.  Why do we need to check IsChromeDoc?  What exact policy are you trying to enforce and how does it fix this bug?

Comment 19

[Tanvi Vyas[:tanvi]]

Attachment: Treat <browser> code as TYPE_DOCUMENT v1

(In reply to Boris Zbarsky (:bz) from comment #18)
> Comment on attachment 720932 [details] [diff] [review]
> Treat privileged <browser> code as TYPE_DOCUMENT v1
> 
> This patch makes no sense.  Why do we need to check IsChromeDoc?  What exact
> policy are you trying to enforce and how does it fix this bug?

I spoke to dveditz before writing the patch and he suggested I add in a second check for IsChromeDoc() just in case remote xul comes back someday.  Here is a new patch without the IsChromeDoc() checks.

Comment 20

[Boris Zbarsky [:bzbarsky]]

The patch _still_ makes no sense.  Again, what policy are we trying to enforce and why?

Comment 21

[Tanvi Vyas[:tanvi]]

> Make it so the content policy checks for the loads in the <browser> use TYPE_DOCUMENT
> rather than TYPE_SUBDOCUMENT

If the requestingElement is <browser>, treat it as content type TYPE_DOCUMENT instead of TYPE_SUBDOCUMENT.  When the content policies are called, the content type will be TYPE_DOCUMENT.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> If the requestingElement is <browser>, treat it as content type TYPE_DOCUMENT instead of
> TYPE_SUBDOCUMENT.

OK.  Why is that a reasonable thing to do in general?  In particular, if there's some random <browser> in chrome, why do we want it to be a DOCUMENT, not SUBDOCUMENT?

I guess the argument is that a <browser> implies that it's not "part of your page"?

Comment 23

[Tanvi Vyas[:tanvi]]

(In reply to Boris Zbarsky (:bz) from comment #22)
> > If the requestingElement is <browser>, treat it as content type TYPE_DOCUMENT instead of
> > TYPE_SUBDOCUMENT.
> 
> OK.  Why is that a reasonable thing to do in general?  In particular, if
> there's some random <browser> in chrome, why do we want it to be a DOCUMENT,
> not SUBDOCUMENT?
> 
> I guess the argument is that a <browser> implies that it's not "part of your
> page"?

What is the alternative?
We could create a flag on the docshell that indicates that the requestingElement is browser?  And then check that flag in nsMixedContentBlocker.cpp?

I am not too familiar with how <iframe mozbrowser> works, but from my understanding, it is an attribute on an iframe (like iframe sandbox).  How would <browser> code add the attribute, when <browser> isn't really an iframe, but is just treated as one (TYPE_SUBDOCUMENT).

Comment 24

[Boris Zbarsky [:bzbarsky]]

> What is the alternative?

Well.  We could check for <browser> whose parent docshell is type="content" and assume that means in-tab UI.

We could check for a magic attribute on the <browser> and set it where needed.

What you really want here is review from someone who does api design for toolkit on the concept.  Once we're clear on that, we can worry about the implementation.

Comment 25

[Tanvi Vyas[:tanvi]]

Thanks BZ for your comments!  Gavin, how do you think we should handle this?

This bug is blocking a crash in Aurora and Nightly, so hopefully we can figure out a solution, implement a fix, and push before the next merge.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #22)
> OK.  Why is that a reasonable thing to do in general?  In particular, if
> there's some random <browser> in chrome, why do we want it to be a DOCUMENT,
> not SUBDOCUMENT?

I don't really know what the DOCUMENT/SUBDOCUMENT distinction means in practice, so it's hard for me to comment on this intelligently. That being said, I could see it making sense for us to treat all chrome-parented docshells as individual documents, particularly since we don't typically care about the mixed-content state of loads in chrome docshells. I don't think that solves this bug, though, since in this case AIUI the <browser> is a child of a content docshell. I suppose we could check for system-principal-parented rather than type=chrome-parented? I.e. I think "we don't care about the mixed-content state of loads in children of system-principaled documents" holds too?

Comment 27

[Tanvi Vyas[:tanvi]]

I spoke to Gavin a bit on IRC.  He says he is proposing a patch like https://bugzilla.mozilla.org/attachment.cgi?id=720932, but without the check for the <browser> tagname.

I'm don't follow this. If we have a docshell that has a parent and we know the rootDocShell is privileged, what tagName options for requestingElement exist other than <browser>?

> I suppose we could check for system-principal-parented rather than type=chrome-
> parented?
I don't know the difference between a system-principal-parented doc and a chrome-parented doc.  If you are chrome, don't you have system-principal?

Comment 28

[Boris Zbarsky [:bzbarsky]]

> what tagName options for requestingElement exist other than <browser>?

<object>, <iframe>, <embed>, at the very least.

> I don't know the difference between a system-principal-parented doc and a
> chrome-parented doc.

They're unrelated, basically.

> If you are chrome, don't you have system-principal?

If you're a type="chrome" docshell, you can have arbitrary documents loaded inside you (arguably a bug).  These may or may not have the system principal.

Comment 29

[Tanvi Vyas[:tanvi]]

Attachment: WIP patch - Check if any parent frames are https v1

BZ, Gavin, and I discussed this yesterday.

Turns out this could be because of a testcase I didn't consider.  If you have an http page with an https iframe, and then try to navigate the https iframe to an http page, you get a mixed content warning.  You shouldn't, since none of the frames parents are https.  To solve for this, we have to check if any parents are https.

It seems that aRequestingLocation is the location the iframe was originally at.  I have to work out how to differentiate a newly loaded iframe from an iframe that is being navigated from https->http.

This is a work in progress patch, I'm sure there are still things wrong with it, but thought I would post something ;)

Test case: http://people.mozilla.com/~tvyas/navigateframe2.html

Comment 30

[Tanvi Vyas[:tanvi]]

We run into an infinite loop in this test case - http://people.mozilla.org/~tvyas/navigategrandframe2.html.  And the mixed content tests also ends up in the infinite loop - content/base/test/test_mixed_content_blocker.html.  So there is something wrong with my check to see if we have reached the last parent with if(rootTreeItem == parentTreeItem).  Investigating.

Comment 31

[Tanvi Vyas[:tanvi]]

Attachment: WIP patch - Check if any parent frames are https v2

Updated patch (with a lot of debugging code).  This patch isn't working as intended.

Test case 1:
http://people.mozilla.com/~tvyas/navigategrandframe.html

* This is an http grandparent page.
* The grandparent has an https parent frame - https://people.mozilla.com/~tvyas/navigateframe2.html
* The parent has an https grandchild - https://people.mozilla.com/~tvyas/navigate2.html.


Test case 2:
http://people.mozilla.com/~tvyas/navigategrandframe2.html

This is an http grandparent page.
It has an http parent frame - http://people.mozilla.com/~tvyas/navigateframe2.html
The parent has an https grandchild - https://people.mozilla.com/~tvyas/navigate2.html.

In test case 1, navigating the https grandchild to an http page by clicking a link should cause a mixed content warning/shield because the grandchild's parent has a secure connection.

In test case 2, navigating the https grandchild to an http page should NOT cause a mixed content warning/shield since none of the grandchild's parents are secure.

These test cases are not working as intended because of aRequestingContext.  In nsMixedContentBlocker.cpp, I get the docShell from aRequestingContext.  I expect the docShell to be the docShell of the grandChild.  I could then traverse up to the docShell until I get to a) an secure parent or b) the root.  If I find a secure parent, I know that there is mixed content.  If I get to the root, I know there is no secure parent and hence no mixed content.

However, aRequestingContext yeilds the docShell of the parent (not the grandchild, as I had expected).  This would make sense the first time the parent loads the grandchild iframe.  But when we navigate the grandchild iframe, I expect the aRequestingContext to be the grandchild, no the parent.

I think the clue to fixing this lies in the difference between navigating a frame and loading a frame.  If I can figure out how to distinguish between loading a new frame and navigating an existing frame, then perhaps I can figure out how to invoke my code on the grandchild aRequestingContext.

Comment 32

[Tanvi Vyas[:tanvi]]

Comment on attachment 724656 [details] [diff] [review]
WIP patch - Check if any parent frames are https v1

># HG changeset patch
># Parent 5edb28ad1c7de4a3d4fb3e34478081e716799d68
># User Tanvi Vyas <[e-mail]>
>Check if any parents are https.
>
>diff --git a/content/base/src/nsMixedContentBlocker.cpp b/content/base/src/nsMixedContentBlocker.cpp
>--- a/content/base/src/nsMixedContentBlocker.cpp
>+++ b/content/base/src/nsMixedContentBlocker.cpp
>@@ -17,16 +17,17 @@
> #include "nsIDocument.h"
> #include "nsIContentViewer.h"
> #include "nsIChannel.h"
> #include "nsIHttpChannel.h"
> #include "mozilla/Preferences.h"
> #include "nsIScriptObjectPrincipal.h"
> #include "nsISecureBrowserUI.h"
> #include "nsIDocumentLoader.h"
>+#include "nsIWebNavigation.h"
> #include "nsLoadGroup.h"
> 
> #include "prlog.h"
> 
> using namespace mozilla;
> 
> // Is mixed script blocking (fonts, plugin content, scripts, stylesheets,
> // iframes, websockets, XHR) enabled?
>@@ -347,33 +348,90 @@ nsMixedContentBlocker::ShouldLoad(uint32
>     *aDecision = REJECT_REQUEST;
>     return NS_OK;
>   }
>   if (!parentIsHttps) {
>     *aDecision = ACCEPT;
>     return NS_OK;
>   }
> 
>+
>   // If we are here we have mixed content.
> 
>   // Determine if the rootDoc is https and if the user decided to allow Mixed Content
>   nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(aRequestingContext);
>   NS_ENSURE_TRUE(docShell, NS_OK);
>   bool rootHasSecureConnection = false;
>   bool allowMixedContent = false;
>   bool isRootDocShell = false;
>   rv = docShell->GetAllowMixedContentAndConnectionData(&rootHasSecureConnection, &allowMixedContent, &isRootDocShell);
>   if (NS_FAILED(rv)) {
>      return rv;
>   }
> 
>-  // Get the root document from the docshell
>+  // Get the rootTreeItem from the docshell
>+  nsCOMPtr<nsIDocShellTreeItem> rootTreeItem;
>+  docShell->GetRootTreeItem(getter_AddRefs(rootTreeItem));
>+  NS_ASSERTION(rootTreeItem, "No root tree item from docshell!");
>+
>+  // When navigating an iframe, the parent (aRequestingLocation) may be https 
>+  // but its parent may not be.  Check the parents to see if they are https.
>+  // If none of the parents are https, allow the load.
>+  // TANVI - Question - When navigating an iframe, is aRequestingLocation the original iframe location?
>+  // or the location of the iframes parent?
>+  if (aContentType==TYPE_SUBDOCUMENT && !rootHasSecureConnection) {
>+
>+    bool currentParentIsHttps = false;
>+    bool httpsParentExists = false;
>+    nsCOMPtr<nsIDocShell> parentShell = docShell;
>+
>+    while(!httpsParentExists) {
>+      nsCOMPtr<nsIDocShellTreeItem> parentTreeItem;
>+      //parentShell->GetSameTypeParent(getter_AddRefs(parentTreeItem));
>+      parentShell->GetParent(getter_AddRefs(parentTreeItem));
>+      NS_ASSERTION(parentTreeItem, "No document parent tree item from document shell tree item!");
>+
>+      //nsIWebNavigation - documents.
>+      nsCOMPtr<nsIWebNavigation> parentAsNav(do_QueryInterface(parentTreeItem));
>+      NS_ASSERTION(parentAsNav, "No web navigation object from parent's docshell tree item");
>+      nsCOMPtr<nsIURI> parentURI;
>+      parentAsNav->GetCurrentURI(getter_AddRefs(parentURI));
>+      NS_ASSERTION(parentURI, "No uri from parent's document.");
>+      rv = parentURI->SchemeIs("https", &currentParentIsHttps);
>+
>+      // if getting the scheme fails, assume there is a https parent and break.
>+      if (NS_FAILED(rv)) {
>+        httpsParentExists = true;
>+        break;
>+      }      
>+
>+      if (currentParentIsHttps) {
>+        httpsParentExists = true;
>+      }
>+
>+      if(rootTreeItem == parentTreeItem) {
>+        printf("\n\n\nparent and root are the same now\n\n\n\n\n");
>+        break;
>+      }
>+    } // end while loop.
>+
>+    if (!httpsParentExists) {
>+      *aDecision = nsIContentPolicy::ACCEPT;
>+      return NS_OK;
>+    }
>+  }
>+  //TANVI - how do we distinguish navigation and framing new content?
>+
>+ 
>+  // Get the sameTypeRoot from the docshell
>   nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot;
>   docShell->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot));
>-  NS_ASSERTION(sameTypeRoot, "No document shell root tree item from document shell tree item!");
>+  NS_ASSERTION(sameTypeRoot, "No document shell root tree item from docshell!");
>+
>+  // Get the root document from the sameTypeRoot
>   nsCOMPtr<nsIDocument> rootDoc = do_GetInterface(sameTypeRoot);
>   NS_ASSERTION(rootDoc, "No root document from document shell root tree item.");
> 
>   // Get eventSink and the current security state from the docShell
>   nsCOMPtr<nsISecurityEventSink> eventSink = do_QueryInterface(docShell);
>   NS_ASSERTION(eventSink, "No eventSink from docShell.");
>   nsCOMPtr<nsIDocShell> rootShell = do_GetInterface(sameTypeRoot);
>   NS_ASSERTION(rootShell, "No root docshell from document shell root tree item.");

Comment 33

[Tanvi Vyas[:tanvi]]

Attachment: Check if any parent frames are https v3

It turns out that in the case of navigating a frame and loading a frame, shouldLoad() is always called on the frames parent (it's never called on the actual frame that is being navigated or being loaded).  Hence, we just need to consider the current docShell the parent.  This passes my test cases, but I still need to run some more tests locally.  Not quite ready for review yet, but much closer :)

Comment 34

[Tanvi Vyas[:tanvi]]

Attachment: Check if any parent frames are https v4

Updated whitespace and comments.

Tested this locally and will push to try once it reopens.

r? to smaug.

And this definitely needs some mochitests.

Thanks BZ and Mossop for your help on this!

Comment 35

[Tanvi Vyas[:tanvi]]

Attachment: Check if any parent frames are https v5

Comment 36

[Tanvi Vyas[:tanvi]]

Updating the Summary.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 725690 [details] [diff] [review]
Check if any parent frames are https v5

Space before and after ==
Null check parentURI, don't assert.


Do we want this behavior? Do we really have to prevent loading
if top level page isn't https?

Comment 38

[Tanvi Vyas[:tanvi]]

Attachment: Check if any parent frames are https v6

Updated patch per Olli's comments.  Carrying over r+.

> 
> Do we want this behavior? Do we really have to prevent loading
> if top level page isn't https?
Yes.  If we have a secure frame with mixed content in it, we want to alert the users of the issue.  Otherwise, an attacker could get around mixed content blocking by embedding mixed content pages.

In fact, bug 826599 is to block the mixed content on subframes completely, without an option for users to disable protection and load anyway.  Whether we want to do this (more drastic) step is still under debate.  The argument for blocking is this:
Assume bank.com has a mixed content advertisement.  The user treads carefully on bank.com because they take precaution when it comes to their assets.  The page still works properly, so they don't allow mixed content.
Then the user visits foo.com.  foo.com has a mixed content flash object.  The user wants to watch the video so disables mixed content protection.  What the user doesn't know is that foo.com has a hidden iframe to bank.com.  In our current implementation of the Mixed Content Blocker, the mixed content on bank.com loads and an http connection to bank.com is observed and potentially mitm'ed on the network.

The argument against this is that users don't understand iframes and subdocuments.  If they see a broken page, they will want to disable protection and allow mixed content on that page.  If we only allow mixed content on the top level document (and perhaps iframes of the same domain), there may still be parts of the page that are broken and the user would have no way of fixing them.

Comment 39

[Tanvi Vyas[:tanvi]]

Attachment: [WIP] Mochitest Plain tests v1

I am working on the tests for this bug, and am not sure if using a browser chrome mochitest is the right way to go (so that I can check if a mixed content popup notification shows up) or if I can just use a regular mochitest and use a timeout to determine if navigating an https frame to an http frame succeeds or fails.

I've tried both and have 2 Work in Progress patches. Olli, can you advise on which of the two I should move forward with?

Comment 40

[Tanvi Vyas[:tanvi]]

Attachment: [WIP] Browser Chrome Mochitests v1

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

mochitest with timeout should be ok, *if* you try to get the right result few times.
I mean, if after timeout you still don't have the right result, fire the timeout again.

Comment 42

[Tanvi Vyas[:tanvi]]

Attachment: Mochitest plain tests v2

(In reply to Olli Pettay [:smaug] from comment #41)
> mochitest with timeout should be ok, *if* you try to get the right result
> few times.
> I mean, if after timeout you still don't have the right result, fire the
> timeout again.

Here is a patch that adds 8 mochitest plain tests.

1) Navigate a child https page to an http page when it is embedded in an http page.
2) Navigate a grandchild https page to an http page when it is embedded in http pages.
3) Navigate a child https page to an http page when it is embedded in an https page.
4) Navigate a granchild https page to an http page when it is embedded in an https parent and has an https grandparent.

These 4 tests are run with security.mixed_content.block_active_content set to true, and then run again when the pref is set to false.

For the timeout, I check that an iframe's location has changed once every 100 ms 100 times (so 10 seconds total).  If the location doesn't change, we assume that navigation failed and that the mixed content blocker prevented navigation from an https iframe to an http iframe.  Olli, is this what you were asking for?  Or do you want me to cycle through the timeout a few more times (increasing the max timeout to 30 or 40 seconds?).  That seems a bit long.

Comment 43

[Tanvi Vyas[:tanvi]]

Attachment: Mochitest plain tests v3

Updated a few typos.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 727480 [details] [diff] [review]
Mochitest plain tests v3


>+  //For tests that require setTimeout, set the maximum polling time to 100 x 100ms = 10 seconds.
>+  var MAX_COUNT = 100;
Perhaps 50 is enough?

>+  {
>+    // When the page is navigating, it goes through about:blank and we will get a permission denied for loc.
>+    // Catch that specific exception and return
>+    try {
>+      var loc = document.getElementById("test1").contentDocument.location;
>+    } catch(e) {
>+      if (e.message && e.message.indexOf("Permission denied to access property") == -1) {
>+        //we received an exception we didn't expect.
>+        throw e;
>+      }
>+      counter_test1++;
>+      return;
>+    }
I certainly don't understand this. What exception are you getting? Can't you just access 
document.getElementById("test1").contentWindow.location?

>+++ b/content/base/test/file_mixed_content_server.sjs
>@@ -26,20 +26,61 @@ function handleRequest(request, response
>     case "stylesheet":
>       response.setHeader("Content-Type", "text/css", false);
>       break;
> 
>     case "object":
>       response.setHeader("Content-Type", "application/x-test", false);
>       break;
> 
>-   case "xhr":
>+    case "xhr":
>       response.setHeader("Content-Type", "text/xml", false);
>       response.setHeader("Access-Control-Allow-Origin", "https://example.com");
>       response.write('<?xml version="1.0" encoding="UTF-8" ?><test></test>');
>       break;
> 
>+    case "insecurePage_navigate_child":
>+      response.setHeader("Content-Type", "text/html", false);
>+      response.write(
>+                    '<!DOCTYPE HTML><html><body><a href="http://example.com/tests/content/base/test/file_mixed_content_server.sjs?type=insecurePage_navigate_child_response" id="link">Testing</a><script>document.getElementById("link").click();</script></body></html>');
>+      break;
>+
>+    case "insecurePage_navigate_child_response":
>+      response.setHeader("Content-Type", "text/html", false);
>+      response.write('<!DOCTYPE HTML><html><body><script>parent.parent.postMessage({"test": "insecurePage_navigate_child", "msg": "navigated to insecure iframe on insecure page"}, "http://mochi.test:8888");</script><p>Navigated from secure to insecure frame on an insecure page</p></body></html>');
>+      break;
>+
>+    case "insecurePage_navigate_grandchild":
>+      response.setHeader("Content-Type", "text/html", false);
>+      response.write('<!DOCTYPE HTML><html><body><a href="http://example.com/tests/content/base/test/file_mixed_content_server.sjs?type=insecurePage_navigate_grandchild_response" id="link">Testing2</a><script>document.getElementById("link").click();</script></body></html>');
>+      break;
>+
>+    case "insecurePage_navigate_grandchild_response":
>+      response.setHeader("Content-Type", "text/html", false);
>+      response.write('<!DOCTYPE HTML><html><body><script>parent.parent.parent.postMessage({"test": "insecurePage_navigate_grandchild", "msg": "navigated to insecure grandchild iframe on insecure page"}, "http://mochi.test:8888");</script><p>Navigated from secure to insecure grandchild frame on an insecure page</p></body></html>');
>+      break;
>+
>+    case "securePage_navigate_child":
>+      response.setHeader("Content-Type", "text/html", false);
>+      response.write('<!DOCTYPE HTML><html><body><a href="http://example.com/tests/content/base/test/file_mixed_content_server.sjs?type=securePage_navigate_child_response" id="link">Testing</a><script>document.getElementById("link").click();</script></body></html>');
>+      break;
>+
>+    case "securePage_navigate_child_response":
>+      response.setHeader("Content-Type", "text/html", false);
>+      response.write('<!DOCTYPE HTML><html><body><script>parent.parent.postMessage({"test": "securePage_navigate_child", "msg": "navigated to insecure iframe on secure page"}, "http://mochi.test:8888");</script><p>Navigated from secure to insecure frame on a secure page</p></body></html>');
>+      break;
>+
>+    case "securePage_navigate_grandchild":
>+      response.setHeader("Content-Type", "text/html", false);
>+      response.write('<!DOCTYPE HTML><html><body><a href="http://example.com/tests/content/base/test/file_mixed_content_server.sjs?type=securePage_navigate_grandchild_response" id="link">Testing2</a><script>document.getElementById("link").click();</script></body></html>');
>+      break;
>+
>+    case "securePage_navigate_grandchild_response":
>+      response.setHeader("Content-Type", "text/html", false);
>+      response.write('<!DOCTYPE HTML><html><body><script>parent.parent.parent.postMessage({"test": "securePage_navigate_grandchild", "msg": "navigated to insecure grandchild iframe on secure page"}, "http://mochi.test:8888");</script><p>Navigated from secure to insecure granchild frame on a secure page</p></body></html>');
>+      break;
>+
Sorry, but this stuff makes following the test really hard.
Why can't you just have some generic page and hmm, perhaps communicate with it by passing #data to the url or postMessage. Or am I missing something here?


>+  function checkTestsCompleted() {
>+    for (var prop in testsToRun) {
>+      // some test hasn't run yet so we're not done
>+      if (!testsToRun[prop])
>+        return;
>+    }
>+    //if the testsToRun are all completed, chnage the pref and run the tests again until we have cycled through all the prefs.
>+    if(counter < 1) {
>+       for (var prop in testsToRun) {
>+         testsToRun[prop] = false;
>+       }
>+      //call to change the preferences
>+      counter++;
>+      SpecialPowers.setBoolPref("security.mixed_content.block_active_content", false);
>+      blockActive = SpecialPowers.getBoolPref("security.mixed_content.block_active_content");
>+      log("blockActive set to "+blockActive+".");
>+      document.getElementById('framediv').innerHTML = '<iframe id="testHarness" src="http://example.com/tests/content/base/test/file_mixed_content_frameNavigation.html"></iframe>';
>+      document.getElementById('secure_framediv').innerHTML = '<iframe id="testHarness" src="https://example.com/tests/content/base/test/file_mixed_content_frameNavigation_secure.html"></iframe>';
You're adding elements with same id? And you don't seem to actually use id for anything

You have testHarness twice in the document itself too.

Comment 45

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v4

(In reply to Olli Pettay [:smaug] from comment #44)
> Comment on attachment 727480 [details] [diff] [review]
> Mochitest plain tests v3
> >+  {
> >+    // When the page is navigating, it goes through about:blank and we will get a permission denied for loc.
> >+    // Catch that specific exception and return
> >+    try {
> >+      var loc = document.getElementById("test1").contentDocument.location;
> >+    } catch(e) {
> >+      if (e.message && e.message.indexOf("Permission denied to access property") == -1) {
> >+        //we received an exception we didn't expect.
> >+        throw e;
> >+      }
> >+      counter_test1++;
> >+      return;
> >+    }
> I certainly don't understand this. What exception are you getting? Can't you
> just access 
> document.getElementById("test1").contentWindow.location?

When the frame navigates, it goes through about:blank.  When we try to access contentWindow.location when the location is about:blank, we get an exception that says Permission denied to access property 'location' in the error console.  This code checks for that specific exception, catches it and moves on (waiting for location to move through about:blank to the destination url, which we can access and will not get a permission denied).

> 
> >+++ b/content/base/test/file_mixed_content_server.sjs
> >@@ -26,20 +26,61 @@ function handleRequest(request, response
>
> Sorry, but this stuff makes following the test really hard.
> Why can't you just have some generic page and hmm, perhaps communicate with
> it by passing #data to the url or postMessage. Or am I missing something
> here?
> 
>
We used file_mixed_content_server.sjs for some of the previous mixed content blocker tests (in bug 62178) and I had decided to just add on to this file.  It does make things more confusing, and we don't actually need an sjs file.  So I've updated the patch and included a new html file that checks the type= parameter in the url and responds accordingly.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 728663 [details] [diff] [review]
Mochitests v4

Could you please align the files in the Makefile.in.
Looks like you've been using spaces when tabs have been used by others.

I know I suggested timeout usage, and yes it is a bit odd, but
at least this shouldn't cause random orange.

s/chnage/change/

"cycled through all the prefs"
prefs? or tests perhaps


if (e.message && e.message.indexOf("Permission denied to access property") == -1) {
Please use the exception type (is it .code or what),  not message.

Could you rename the _server file. Maybe _inner or great_grandchildparent

if switch(event.data.test) {
could there be default:
ok(false, "unexpected message");


+  // get the Content-Type to serve from the query string
+  var contentType = null;
+  var params = document.location.search.slice(1).split("&");
+  for (var i = 0; i < params.length; i++)
+  {
+    var tmp = params[i].split("=");
+    if (tmp[0]="type")
+      contentType=tmp[1];
+  }

Nothing to do with contenttype.
You could just drop type= from the urls and 
use
var type = location.search.substring(1);



über-hard to review. Make sure to run this through tryserver few times before landing.

Comment 47

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v5

Updated per Olli's comments.  Carrying over r+.

(In reply to Olli Pettay [:smaug] from comment #46)
> if (e.message && e.message.indexOf("Permission denied to access property")
> == -1) {
> Please use the exception type (is it .code or what),  not message.

This mdn page 
https://developer.mozilla.org/en-US/docs/DOM/DOMException says that code is deprecated and to use
Deprecated use DOMError.name instead, linking to https://developer.mozilla.org/en-US/docs/DOM/DOMError.

So I am now checking for (e.name === "SecurityError") instead of the error message.

> 
> über-hard to review. Make sure to run this through tryserver few times
> before landing.

I have run this through try along with a bunch of other patches.  Most recent runs are here:
https://tbpl.mozilla.org/?tree=Try&rev=d078bae998d3 - This one has some oranges due to a problem I'm working on in bug 834836 that is not related to this patch.

https://tbpl.mozilla.org/?tree=Try&rev=27e901d5fbe9 - This one looks good.

Running it one more time with hopes to land this bug and bug 836951 tomorrow:
https://tbpl.mozilla.org/?tree=Try&rev=69544c714afd

Comment 48

[Tanvi Vyas[:tanvi]]

It looks like this is ready to be pushed!

Comment 49

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v6

Failed to properly update the tab/spacing in the Makefile.  This patch includes the whitespace fix.

Comment 50

[Tanvi Vyas[:tanvi]]

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd91df842f2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/196fbc36c878

Comment 51

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c3ed1c7b9aeb and https://hg.mozilla.org/integration/mozilla-inbound/rev/eaab632b48a3 - unexpected assertion in test_mixed_content_blocker_frameNavigation.html (https://tbpl.mozilla.org/php/getParsedLog.php?id=21138570&tree=Mozilla-Inbound and friends, after the bustage you landed into was cleared out, is probably the easier place to see it) and I didn't want to be the one to decide either whether it was sort of expected, or whether to leave the code in while removing the tests.

Comment 52

[Phil Ringnalda (:philor)]

Oh, and on Android? It doesn't assert (well, it might, but we don't run debug Android anywhere you'll see), but it times out every single time.

Comment 53

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v7

The assertion failure is due to a bug in nsSHEntry (probably related to bug 462076).  Here is a pastebin with a stack trace: http://www.pastebin.mozilla.org/2251254.  The mochitests here trigger an edge case that perhaps hasn't been considered.  They are quite complicated mochitests, because we have 2 iframes that both have grandchildren and great grand child frames that navigate.  Having 2 sibiling iframes with grandchildren that navigate is required to reproduce the assertion failure (not sure yet if the great grandchidren would be needed in a test case created outside of the testing framework).  I will file a separate bug for this.

Anyway, I've rewritten the mochitests so that this edge case doesn't occur.  Instead of having 2 sibling iframes running tests, I have one frame run tests.  Once they are complete, I change the location of the frame and run the next set of tests.

For the android failures, android mochitests don't have ssl.  I forgot to add my tests to http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/android.json#22 so that they would not be run for android.  I have added that to this patch.

Comment 54

[Tanvi Vyas[:tanvi]]

Push to try: https://tbpl.mozilla.org/?tree=Try&rev=ff620f504bd1

Comment 55

[Tanvi Vyas[:tanvi]]

> The assertion failure is due to a bug in nsSHEntry (probably related to bug
> 462076)
....
> I will file a separate bug for
> this.
> 

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=855443.

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 730299 [details] [diff] [review]
Mochitests v7

firstTest seems to be something for debugging.
Perhaps remove it or add a comment why it is there.

File naming is still super-confusing, but I don't have good suggestions how to fix that.

I hope I'm not missing anything.
Testing only one thing at once and not trying to load several iframes simultaneously would probably make this less hard to read.

Comment 57

[Phil Ringnalda (:philor)]

You'll also need to disable it on b2g.

Comment 58

[Tanvi Vyas[:tanvi]]

(In reply to Phil Ringnalda (:philor) from comment #57)
> You'll also need to disable it on b2g.

Not sure if I have to do this explicitly (or how to).  I didn't for previous mixed content blocker mochitests.  I only added them to android.json to make sure they didn't run in fennec.  We don't have a mixed content blocker for b2g right now.

Comment 59

[Phil Ringnalda (:philor)]

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/b2g.json#96 etc. You didn't have to before because at first b2g wasn't running any tests in content/*, and then someone manually excluded the ones, including the mixed content ones, that didn't work on b2g.

Comment 60

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v8

Changed firstTest to firstTestDebugMessage and added this test and the mochitest for bug 803255 to b2g.json.

Carrying over r+.  Try looks good so far (aside from the b2g failure, that will no longer be an issue with this updated patch that excludes the test from b2g).  Will push later today.

Comment 61

[Tanvi Vyas[:tanvi]]

Pushed:
hg.mozilla.org/integration/mozilla-inbound/rev/cf276949f2b6
hg.mozilla.org/integration/mozilla-inbound/rev/cdcf6581c682

Comment 62

[Daniel Holbert [:dholbert]]

(In reply to Tanvi Vyas [:tanvi] from comment #61)
> Pushed:
> hg.mozilla.org/integration/mozilla-inbound/rev/cf276949f2b6

This added a completely unused variable "bool chromeParent = false;" in nsMixedContentBlocker::ShouldLoad(), which triggered a new unused-variable build warning.

Given that the variable is never read, and it's never set outside of the line where it's declared, I'm going to assume that variable can just be dropped.  I pushed a trivial followup to remove it: https://hg.mozilla.org/integration/mozilla-inbound/rev/293498096b28

(In the unlikely case that we really want this variable to stay in, then feel free to add it back, along with whatever usages it's supposed to have.) :)

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf276949f2b6
https://hg.mozilla.org/mozilla-central/rev/cdcf6581c682
https://hg.mozilla.org/mozilla-central/rev/293498096b28

Comment 64

[Tanvi Vyas[:tanvi]]

Comment on attachment 727060 [details] [diff] [review]
Check if any parent frames are https v6

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Fixing top crasher Bug 836951 depends on this patch.

User impact if declined: Top crasher in Firefox 21

Testing completed (on m-c, etc.): Landed on central.

Risk to taking this patch (and alternatives if risky): Fixes a crash.  If a user has security.mixed_content.block_active_content set to true (it is false by default) then it prevents a false positive where content is blocked that isn't actually mixed content (and hence shouldn't be blocked).

String or IDL/UUID changes made by this patch: None.

Comment 65

[Tanvi Vyas[:tanvi]]

Comment on attachment 730391 [details] [diff] [review]
Mochitests v8

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Fixing top crasher Bug 836951 depends on this patch.

User impact if declined: Top crasher in Firefox 21

Testing completed (on m-c, etc.): Landed on central.

Risk to taking this patch (and alternatives if risky): One of the 8 mochitests in this patch has intermittent failures on inbound - Bug 855730. We could uplift the other two patches to aurora (other patch in this bug, and patch marked for uplift in bug 836951) and NOT uplift this patch.  Then we would have landed some code without associated mochitests in Aurora.

String or IDL/UUID changes made by this patch: None.

Comment 66

[Tanvi Vyas[:tanvi]]

(In reply to Daniel Holbert [:dholbert] from comment #62)
> (In reply to Tanvi Vyas [:tanvi] from comment #61)
> > Pushed:
> > hg.mozilla.org/integration/mozilla-inbound/rev/cf276949f2b6
> 
> This added a completely unused variable "bool chromeParent = false;" in
> nsMixedContentBlocker::ShouldLoad(), which triggered a new unused-variable
> build warning.
> 
> Given that the variable is never read, and it's never set outside of the
> line where it's declared, I'm going to assume that variable can just be
> dropped.  I pushed a trivial followup to remove it:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/293498096b28
> 
> (In the unlikely case that we really want this variable to stay in, then
> feel free to add it back, along with whatever usages it's supposed to have.)
> :)

Thanks dholbert for catching this!  If we uplift this patch to aurora, we should also uplift dholberts trivial patch to remove this unused variable.  I can also qfold it in, but I'm not sure if I should change the patches before the uplift to aurora now that they have already been applied on central.
https://hg.mozilla.org/integration/mozilla-inbound/rev/293498096b28

Comment 67

[bhavana bajaj [:bajaj]]

(In reply to Tanvi Vyas [:tanvi] from comment #65)
> Comment on attachment 730391 [details] [diff] [review]
> Mochitests v8
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Fixing top crasher Bug 836951
> depends on this patch.
> 
> User impact if declined: Top crasher in Firefox 21
> 
> Testing completed (on m-c, etc.): Landed on central.
> 
> Risk to taking this patch (and alternatives if risky): One of the 8
> mochitests in this patch has intermittent failures on inbound - Bug 855730.
> We could uplift the other two patches to aurora (other patch in this bug,
> and patch marked for uplift in bug 836951) and NOT uplift this patch.  Then
> we would have landed some code without associated mochitests in Aurora.

Looks like  bug 836951 just got reviewed, lets land that as soon as you can so we can make sure the dependencies are resolved .Holding off approval until then
> 
> String or IDL/UUID changes made by this patch: None.

Comment 68

[bhavana bajaj [:bajaj]]

err, I meant Bug 855730 above :-|

Comment 69

[Ryan VanderMeulen [:RyanVM]]

This doesn't approval yet (and there's no need to request approval once it does, I have bug queries that'll find it).

Comment 70

[Tanvi Vyas[:tanvi]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #69)
> This doesn't approval yet (and there's no need to request approval once it
> does, I have bug queries that'll find it).

Sorry, I marked checkin-needed on the wrong bug.  Meant to mark it on bug 855730 for the mochitest logging patch.  That is checked into inbound now.

Comment 71

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #67)
> (In reply to Tanvi Vyas [:tanvi] from comment #65)
> > Comment on attachment 730391 [details] [diff] [review]
> > Mochitests v8
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): Fixing top crasher Bug 836951
> > depends on this patch.
> > 
> > User impact if declined: Top crasher in Firefox 21
> > 
> > Testing completed (on m-c, etc.): Landed on central.
> > 
> > Risk to taking this patch (and alternatives if risky): One of the 8
> > mochitests in this patch has intermittent failures on inbound - Bug 855730.
> > We could uplift the other two patches to aurora (other patch in this bug,
> > and patch marked for uplift in bug 836951) and NOT uplift this patch.  Then
> > we would have landed some code without associated mochitests in Aurora.
> 
> Looks like  bug 836951 just got reviewed, lets land that as soon as you can
> so we can make sure the dependencies are resolved .Holding off approval
> until then
> > 
> > String or IDL/UUID changes made by this patch: None.

Approving the uplift on aurora as this is needed to help fix the top-crasher.
Spoke offline to tanvi & the scope of the failing mochitest is limited to mixed content blocker code which is preffed off by default on Fx21.In addition the failing test is being actively investigated in Bug 855730.

Comment 72

[Tanvi Vyas[:tanvi]]

Attachment: Check if any parent frames are https v7

I have updated the patch to remove the extra boolean that dholbert found.  This patch can be used for the aurora update.  The only difference is that the extra flag is gone.  Carrying over the r+.

host-5-247:patches tvyas$ diff ParentFrames-03-19-13.patch ParentFrames-03-29-13.patch 
2c2
< # Parent 5edb28ad1c7de4a3d4fb3e34478081e716799d68
---
> # Parent 4967844a6ae2bd64c8a37dd65a2664602ecddb09
9c9
< @@ -17,16 +17,17 @@
---
> @@ -18,16 +18,17 @@
27c27
< @@ -347,33 +348,75 @@ nsMixedContentBlocker::ShouldLoad(uint32
---
> @@ -348,33 +349,74 @@ nsMixedContentBlocker::ShouldLoad(uint32
63d62
< +    bool chromeParent = false;

Bhavana, if this is fine with you, please carry over the approval-mozilla-aurora flag.  Thanks!

Comment 73

[bhavana bajaj [:bajaj]]

(In reply to Tanvi Vyas [:tanvi] from comment #72)
> Created attachment 731382 [details] [diff] [review]
> Check if any parent frames are https v7
> 
> I have updated the patch to remove the extra boolean that dholbert found. 
> This patch can be used for the aurora update.  The only difference is that
> the extra flag is gone.  Carrying over the r+.
> 
> host-5-247:patches tvyas$ diff ParentFrames-03-19-13.patch
> ParentFrames-03-29-13.patch 
> 2c2
> < # Parent 5edb28ad1c7de4a3d4fb3e34478081e716799d68
> ---
> > # Parent 4967844a6ae2bd64c8a37dd65a2664602ecddb09
> 9c9
> < @@ -17,16 +17,17 @@
> ---
> > @@ -18,16 +18,17 @@
> 27c27
> < @@ -347,33 +348,75 @@ nsMixedContentBlocker::ShouldLoad(uint32
> ---
> > @@ -348,33 +349,74 @@ nsMixedContentBlocker::ShouldLoad(uint32
> 63d62
> < +    bool chromeParent = false;
> 
> Bhavana, if this is fine with you, please carry over the
> approval-mozilla-aurora flag.  Thanks!

Considering this piece is already on m-c and the updated patch removes unused variable, approving for uplift

Comment 74

[bhavana bajaj [:bajaj]]

Comment on attachment 731382 [details] [diff] [review]
Check if any parent frames are https v7

updated patch per comment 66, approving for uplift

Comment 75

[Tanvi Vyas[:tanvi]]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/c4237eb085c3
https://hg.mozilla.org/releases/mozilla-aurora/rev/4717c038bc0c

Note that I didn't have to exclude the mochitest from b2g.json (just android.json) because mozilla-aurora doesn't seem to do /content tests for b2g anyway.  That was enabled later on.  The aurora b2g.json file doe snot have any /content tests in it:
https://mxr.mozilla.org/mozilla-aurora/source/testing/mochitest/b2g.json

Comment 76

[Mihai Morar, (:MihaiMorar)]

Tanvi is anything I can do here as long there is a mochitest for this fix?

Comment 77

[Tanvi Vyas[:tanvi]]

(In reply to Mihai Morar, QA (:MihaiMorar) [needinfo? me for any requests] from comment #76)
> Tanvi is anything I can do here as long there is a mochitest for this fix?

I think we are good here.