822367 - Implement Mixed Content Blocker Doorhanger - Backend Changes

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Tanvi Vyas[:tanvi]]

Implement the Backend changes necessary for the Drop down doorhanger when Mixed Active Content is blocked.


+++ This bug was initially created as a clone of Bug #782654 +++

Bug 62178 implements a mechanism to block http script/active or http display content on https pages.  This bug is to actually implement the blocking UI and enhance the patch in bug 62178 to make this feasible.

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: Plumbing for Mixed Content Blocker Doorhanger v1

This patch looks like it should work, and allow mixed active content when the user overrides the blocking on a particular page.  However, I can't seem to set gBrowser.docShell.userOverrideBlockedMixedActiveContent to true in browser.js.  Every time I dump the value it is false (even if I change it's default state to true in nsDocShell.cpp).  Not sure what I'm missing here.

Comment 2

[Tanvi Vyas[:tanvi]]

I fixed the bug I had and am able to set gBrowser.docShell.userOverrideBlockedMixedActiveContent.  After the flag is set, I want to reload the page with and allow mixed content loads.  This flag is on the Document.  But after it's set, document is recreated with the reload (and the flag is hence reset).  I talked to jst and he suggested using the httpchannel instead of the document.  I've tried this as well and the same thing happens (presumably because we also create a new channel on reloads).

I also looked at the nsIWebNavigation load flags, but they are all used for very specific cases so I wouldn't be able to use an existing flag and pass it into BrowserReloadWithFlags().

I could save the flag in the docShell, then reload, and then reset the flag after the load, but that is risky.  If a user stops the reload midway through and decides to go to another website, what will happen?  Since they are in the same docshell, we could end up allowing mixed active content on that page even though the user didn't consent to "disable protection" for that page.

Thoughts on how/where I should preserve the userOverride flag through a browser reload?

Comment 3

[Dão Gottwald [:dao]]

(In reply to Tanvi Vyas [:tanvi] from comment #2)
> I also looked at the nsIWebNavigation load flags, but they are all used for
> very specific cases so I wouldn't be able to use an existing flag and pass
> it into BrowserReloadWithFlags().

So maybe we should create a new one?

Comment 4

[Tanvi Vyas[:tanvi]]

Dao - thanks for the suggestion.  Spoke to bz and jorgev today and decided to get a new flag in nsIWebNavigation that is below 0xffff (so that we can use it in a reload).  In order to do this, I moved LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP from 0x2000 to 0x100000 (since it's not allowed in reloads anyway - mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShellLoadTypes.h#31).  Jorgev checked and only found a couple addons using this flag by the constant name (and not the value).

Now that I can set a LOAD_FLAGS_ALLOW_MIXED_CONTENT in BrowserReloadWithFlags(), I have to figure out how to get the document and set the user override flag before we call the content policies (http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8464).  Not sure if I should use mContentViewer or mScriptGlobal/context.  I attempted to use mScriptGlobal, but the flag doesn't seem be set in the ShouldLoad() call in nsMixedContentBlocker.cpp

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: Plumbing for Mixed Content Blocker Doorhanger and User Override v2

Finally have a working implementation!  Olli, there are a couple questions in the comments.

Thanks!

Comment 6

[Tanvi Vyas[:tanvi]]

Attachment: Plumbing for Mixed Content Blocker Doorhanger and User Override v3

(In reply to Tanvi Vyas [:tanvi] from comment #5)
> Finally have a working implementation!  Olli, there are a couple questions
> in the comments.
> 
> Thanks!

Added a couple new uuids that were missing.

Comment 7

[Tanvi Vyas[:tanvi]]

Here's an explanation of what the attached patch does:

This patch adds the flag hasMixedActiveContentBlocked to nsIDocument.  This tells the browser that mixed content exists on the page and has been blocked by the Mixed Content Blocker content policy.  Browser.js checks if this flag is true in checkIdentity.  If it is true, it will drop down a doorhanger and include an icon on the left side of the url bar (something like http://people.mozilla.com/~tvyas/MixedUI4A.png and http://people.mozilla.com/~tvyas/MixedUI4B.png, which is covered in the frontend bug 822371).  When the user clicks "Disable Protection on this Page", a callback in browser.js is invoked to reload the browser with the mixed content enabled.  

In order to do this, I've added another flag called userUserHasOverridenBlockedMixedContent to the httpchannel.  If it is true, the user has overriden the mixed content blocking, and mixed content should be allowed on the current page.  We check this flag on the httpchannel in shouldLoad in nsMixedContentBlocker and allow all content to load (both mixed display and mixed active) when the flag is true.  The reason this flag is on the httpchannel and not the document is because the new document hasn't been created yet within the BrowserReloadWithFlags() call, but the new httpchannel is created (see nsDocShell::DoURILoad()).

I also had to add a flag to nsIWebNavigation that could be passed into BrowserReloadWithFlags.  See comments 3 & 4 for more details on why we decided to take this approach.

Comment 8

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

Comment on attachment 695079 [details] [diff] [review]
Plumbing for Mixed Content Blocker Doorhanger and User Override v3

All browser/* needs a review from a browser peer

>   onSecurityChange: function (aWebProgress, aRequest, aState) {
>     // Don't need to do anything if the data we use to update the UI hasn't
>     // changed
>     if (this._state == aState &&
>-        !this._hostChanged) {
>+        !this._hostChanged &&
>+        !gBrowser.docShell.hasMixedActiveContentBlocked &&
>+        !gBrowser.docShell.hasMixedActiveContentLoaded) {
I don't know browser/ code, but does this really work if some page is loaded in a background tab?
And in general, how should we handle the case when a background tab tries to load mixed content?

>@@ -6623,18 +6626,55 @@ var gIdentityHandler = {
>     this._lastStatus = currentStatus;
>     this._lastLocation = location;
> 
>     let nsIWebProgressListener = Ci.nsIWebProgressListener;
>     if (location.protocol == "chrome:" || location.protocol == "about:") {
>       this.setMode(this.IDENTITY_MODE_CHROMEUI);
>     } else if (state & nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL) {
>       this.setMode(this.IDENTITY_MODE_IDENTIFIED);
>+      if (gBrowser.docShell.hasMixedActiveContentBlocked) {
>+        let messageString = gNavigatorBundle.getString("mixedContentBlockerMessage.message");
>+        let action = {
>+          label: gNavigatorBundle.getString("mixedContentBlockerMessage.label"),
>+          accessKey: gNavigatorBundle.getString("mixedContentBlockerMessage.accesskey"),
>+          callback: function() {
>+                                // Reload the page with the content unblocked
>+                                BrowserReloadWithFlags(nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT);
>+                               }
Odd indentation



>@@ -318,22 +319,78 @@ nsMixedContentBlocker::ShouldLoad(uint32
>     *aDecision = REJECT_REQUEST;
>     return NS_OK;
>   }
>   if (!parentIsHttps) {
>     *aDecision = ACCEPT;
>     return NS_OK;
>   }
> 
>+  // Set the flag that tells the document that Mixed Active Content was blocked on the page
>+  nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(aRequestingContext);
>+  nsCOMPtr<nsIDocShellTreeItem> currentDocShellTreeItem(do_QueryInterface(docShell));
>+  if (!currentDocShellTreeItem) {
>+      return NS_OK;
>+  }
2 space indentation

>+  nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot;
>+  currentDocShellTreeItem->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot));
>+  NS_ASSERTION(sameTypeRoot, "No document shell root tree item from document shell tree item!");
>+
>+  // now get the document from sameTypeRoot
>+  nsCOMPtr<nsIDocument> rootDoc = do_GetInterface(sameTypeRoot);
>+  NS_ASSERTION(rootDoc, "No root document from document shell root tree item.");
>+
>+  nsCOMPtr<nsIChannel> channel = rootDoc->GetChannel();
>+  nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(channel));
I think the following would look a bit better
nsCOMPtr<nsIDocShell> root = do_QueryInterface(sameTypeRoot);
nsCOMPtr<nsIChannel> channel;
root->GetCurrentDocumentChannel(getter_AddRefs(channel));
nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel);
// ... null check http channel and check the flag ...


>   // If we are here we have mixed content.
>   // If the content is display content, and the pref says display content should be blocked, block it.
>-  // If the content is mixed content, and the pref says mixed content should be blocked, block it.
>-  if ( (sBlockMixedDisplay && classification == eMixedDisplay) || (sBlockMixedScript && classification == eMixedScript) ) {
>-     *aDecision = nsIContentPolicy::REJECT_REQUEST;
>+  if (sBlockMixedDisplay && classification == eMixedDisplay) {
>+     if (userOverride) {
>+       *aDecision = nsIContentPolicy::ACCEPT;
>+       rootDoc->SetHasMixedActiveContentLoaded(true);
>+     } else {
>+       *aDecision = nsIContentPolicy::REJECT_REQUEST;
>+     }
>      return NS_OK;
>+  } else if ((sBlockMixedScript && classification == eMixedScript)) {
extra ()


>+    // Call eventsink to invoke the Mixed Content UI
>+    nsCOMPtr<nsIDocShell> rootShell = do_GetInterface(sameTypeRoot);
>+    NS_ASSERTION(rootShell, "No root docshell from document shell root tree item.");
>+    nsCOMPtr<nsISecureBrowserUI> aSecurityUI;
>+    rootShell->GetSecurityUI(getter_AddRefs(aSecurityUI));
>+    uint32_t aState;
only parameters should be named aFoo.
And you don't actually use aState if userOverride is true, so could you call GetState only if needed.

> NS_IMETHODIMP
>+nsDocShell::GetHasMixedActiveContentBlocked(bool *aHasMixedActiveContentBlocked)
bool* aHasMixedActiveContentBlocked
(I know, nsDocShell has odd coding style, well, almost no coding style at all)

>   /**
>-   * This flag specifies that the URI may be submitted to a third-party
>-   * server for correction. This should only be applied to non-sensitive
>-   * URIs entered by users.  This flag must not be passed to Reload.
>-   */
>-  const unsigned long LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP = 0x2000;
So we set LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP in many places. Why is it ok to remove it?
It is also used by several addons.
https://mxr.mozilla.org/addons/search?string=LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP


>+    This flag is set when a user explicitly disables the Mixed Content
>+    Blocker, and allows Mixed Content to load on an https page.
>+  */
>+  const unsigned long LOAD_FLAGS_ALLOW_MIXED_CONTENT = 0x2000;
> 
>   /**
>    * This flag specifies that this is the first load in this object.
>    * Set with care, since setting incorrectly can cause us to assume that
>    * nothing was actually loaded in this object if the load ends up being 
>    * handled by an external application.  This flag must not be passed to
>    * Reload.
>    */
>@@ -186,16 +185,23 @@ interface nsIWebNavigation : nsISupports
>   const unsigned long LOAD_FLAGS_DISALLOW_INHERIT_OWNER = 0x40000;
> 
>   /**
>    * Assume the URI is encoded in UTF-8.
>    */
>   const unsigned long LOAD_FLAGS_URI_IS_UTF8 = 0x80000;
> 
>   /**
>+   * This flag specifies that the URI may be submitted to a third-party
>+   * server for correction. This should only be applied to non-sensitive
>+   * URIs entered by users.  This flag must not be passed to Reload.
>+   */
>+  const unsigned long LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP = 0x100000;
Oh, you don't remove it but just have different value... Why exactly?


>+++ b/netwerk/protocol/http/HttpBaseChannel.cpp
all netwerk/* needs a review from necko peer.


I'd like to see a new patch. And I still wonder if we should add a flag to loadgroup, not to http channel.
It would be easier to check the load group flag everywhere.

Comment 9

[Tanvi Vyas[:tanvi]]

Attachment: Plumbing for Mixed Content Blocker Doorhanger and User Override v4

This is the first of 4 patches.  

1) This patch just has the changes in /content and /docshell.  Within nsMixedContentBlocker.cpp, I include 2 options for the flag that determines whether the user has overridden the Mixed Content Blocker (one using loadgroup and another using HttpChannel).  This patch also incorporates the regression I found last week (bug 824871)

2) browser.js edits for /browser peer review.

3) LoadGroup option - patch to add a flag nsIRequest

4) HttpChannel option - patch to add a flag to nsIHttpChannel

---------------------------------------------------
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 695079 [details] [diff] [review]
> Plumbing for Mixed Content Blocker Doorhanger and User Override v3
To address smaug's review comments; I handled the nits and here is what is remaining:

> >+  nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot;
> >+  currentDocShellTreeItem->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot));
> >+  NS_ASSERTION(sameTypeRoot, "No document shell root tree item from document shell tree item!");
> >+
> >+  // now get the document from sameTypeRoot
> >+  nsCOMPtr<nsIDocument> rootDoc = do_GetInterface(sameTypeRoot);
> >+  NS_ASSERTION(rootDoc, "No root document from document shell root tree item.");
> >+
> >+  nsCOMPtr<nsIChannel> channel = rootDoc->GetChannel();
> >+  nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(channel));
> I think the following would look a bit better
> nsCOMPtr<nsIDocShell> root = do_QueryInterface(sameTypeRoot);
> nsCOMPtr<nsIChannel> channel;
> root->GetCurrentDocumentChannel(getter_AddRefs(channel));
> nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel);
> // ... null check http channel and check the flag ...
> .

I need the rootDoc anyway to set a flag (e.g. see line 404 in the new patch - rootDoc->SetHasMixedActiveContentLoaded(true);)


 
> >   /**
> >+   * This flag specifies that the URI may be submitted to a third-party
> >+   * server for correction. This should only be applied to non-sensitive
> >+   * URIs entered by users.  This flag must not be passed to Reload.
> >+   */
> >+  const unsigned long LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP = 0x100000;
> Oh, you don't remove it but just have different value... Why exactly?
> 
Comment 4 in this bug explains this.


> I'd like to see a new patch. And I still wonder if we should add a flag to
> loadgroup, not to http channel.
> It would be easier to check the load group flag everywhere

The loadGroup is per docshell (not per page) which is one of the many reasons I'm having trouble using the loadGroup.  See the comments that associated with the next few patches for more specific details.

Comment 10

[Tanvi Vyas[:tanvi]]

Attachment: Invoke Mixed Content Blocker Doorhanger in Browser.js v1

Creates a function in browser.js called showOverrideMixedContent() which shows the Mixed Content Blocker Doorhanger when a document has Mixed Active Content that is blocked.

This can happen on an https page where all mixed content is blocked (IDENTITY_MODE_IDENTIFIED and IDENTITY_MODE_DOMAIN_VERIFIED).  Or an http page with an https iframe that has mixed content blocked (IDENTITY_MODE_UNKNOWN - see bug 824871 if you would like more details on this case).

In comment 8, Olli noted that we decide to show the doorhanger on gBrowser.docShell.  What happens if an https page tries to load mixed content in a background tab?  I'm not sure how to handle this case, so advice would be helpful :)

Thanks!

Comment 11

[Tanvi Vyas[:tanvi]]

Attachment: Option 1 - Set a Flag in the LoadGroup when Blocking is Overridden v1

Add a flag to the LoadGroup (instead of the http channel of the rootDoc) that tells the browser whether or not the user has overridden blocking from the Mixed Content Blocker on the current page.

Issues:
The LoadGroup exists per docshell, and not per document.  In order to get this to work, we need to figure out where we can reset this flag onLocationChange.
We also need to differentiate between a location change and a BrowserReload with the override flag set (since in this case, we do want the mixed content to load and don't want to display the doorhanger).
and want that specific loadflag to persist in the loadgroup).

Comment 12

[Tanvi Vyas[:tanvi]]

Attachment: Option 2 - Set a Flag in the HttpChannel when Blocking is Overridden v1

Add a flag to the root Http Channel (instead of the loadGroup) that tells the browser whether or not the user has overridden blocking from the Mixed Content Blocker for the current page.

Issues:
There could be some edge cases where this doesn't work.  For example, document.open() creates a new channel; if document.open() overwrites the contents of the page after the user has enabled mixed script, any mixed script the document.open() invokes will be blocked and the user will be presented with a doorhanger again, asking them if they want to allow the blocked content.

document.open() may not be the only or biggest edge case.  Are there other edge cases where the root http channel changes without a location change or reload?

Thanks!

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 696844 [details] [diff] [review]
Invoke Mixed Content Blocker Doorhanger in Browser.js v1

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

If Dao is available, I think it would be good for him to review this as well.

I found the strings used in this patch in a patch in bug 822371. It's not clear to me in what order these patches will land, but this patch will need bug 822371 to land first. When will the patch for bug 822371 be marked as ready for review?

::: browser/base/content/browser.js
@@ +4198,5 @@
>      // changed
>      if (this._state == aState &&
> +        !this._hostChanged &&
> +        !gBrowser.docShell.hasMixedActiveContentBlocked &&
> +        !gBrowser.docShell.hasMixedActiveContentLoaded) {

In reply to comment #8, as I understand it this code is called when the location changes for gBrowser. So if the page is loading in the background, this code isn't run until the tab is switched to. This explains how gBrowser.contentWindow can be used later on in this function and how onLocationChange references gBrowser.selectedBrowser. http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4069 should update the popup notifications when the selected tab changes or if the selected tab has a navigation.

@@ +6624,5 @@
>      } else if (state & nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL) {
>        this.setMode(this.IDENTITY_MODE_IDENTIFIED);
> +      if (gBrowser.docShell.hasMixedActiveContentBlocked) {
> +        this.showOverrideMixedContent();
> +      }

nit, style for browser.js is to only include curly brackets for multiline if-blocks.

@@ +6648,5 @@
> +  /**
> +   * Display the Mixed Content Blocker doohanger, providing an option
> +   * to the user to override mixed content blocking
> +   */
> +  showOverrideMixedContent:function() {

nit, spaces around the colon (I'm just following the style of getEffectiveHost below), like so:
showOverrideMixedContent : function() {

@@ +6655,5 @@
> +      label: gNavigatorBundle.getString("mixedContentBlockerMessage.label"),
> +      accessKey: gNavigatorBundle.getString("mixedContentBlockerMessage.accesskey"),
> +      callback: function() { // Reload the page with the content unblocked
> +        BrowserReloadWithFlags(nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT); }
> +        };

nit: the curly bracket after BrowserReloadWithFlags should be on its own line and it should line up with the 'c' in "callback". The '};' should be dedented four spaces to line up with the 'l' in "let" five lines above.

Comment 14

[Tanvi Vyas[:tanvi]]

Attachment: Invoke Mixed Content Blocker Doorhanger in Browser.js v2

Thanks for the review comments Jared!  I addressed the nits.

This bug has to land at the same time as Bug 822371 and Bug 822366.  The 3 bugs go together.  Landing one change without the others doesn't really make sense, but I had to split up the different frontend and backend parts into different bugs.  Bug 822366 is ready to go and for bug 822371, we are waiting for a visual design from Stephen Horlander.  Jared, do you think this patch makes more sense as part of 822371?

Marking r? to dao and carrying over Jared's r+.

Comment 15

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

(In reply to Tanvi Vyas [:tanvi] from comment #14)
> Created attachment 696945 [details] [diff] [review]
> Invoke Mixed Content Blocker Doorhanger in Browser.js v2

Doesn't this patch belong in bug 822371?

Comment 16

[Tanvi Vyas[:tanvi]]

Comment on attachment 696945 [details] [diff] [review]
Invoke Mixed Content Blocker Doorhanger in Browser.js v2

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #15)
> (In reply to Tanvi Vyas [:tanvi] from comment #14)
> > Created attachment 696945 [details] [diff] [review]
> > Invoke Mixed Content Blocker Doorhanger in Browser.js v2
> 
> Doesn't this patch belong in bug 822371?

Yes, that does make more sense.  Moving this patch over to bug 822371.

Comment 17

[Tanvi Vyas[:tanvi]]

Attachment: Plumbing for Mixed Content Blocker Doorhanger and User Override v5

I spoke to smaug about the issues with setting the user override flag on the LoadGroup and the Channel.  I'll obsoleting both of those patches, as we decided to try for another option.

smaug suggested creating a member variable mMixedContentChannel in the docshell.  When the user overrides the content, we will set mMixedContentChannel to the current channel.  In nsMixedContentBlocker.cpp, if mMixedContentChannel == currentChannel, then mixed content is allowed.

For the edge case of document.open() (where a new channel is created), I will write code to handle that case in nsHTMLDocument::Open() and add a new patch.  I have not included that in this patch.  But everything else seems to work.

This patch also incorporates the fix to bug 824871.

Thank you Olli and bz for your help with this!!!

Comment 18

[Tanvi Vyas[:tanvi]]

Comment on attachment 697367 [details] [diff] [review]
Plumbing for Mixed Content Blocker Doorhanger and User Override v5

Oops!  Obsoleted the wrong patch.

Comment 19

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

Comment on attachment 697367 [details] [diff] [review]
Plumbing for Mixed Content Blocker Doorhanger and User Override v5


>+  // Get the rootDoc and the loadgroup/OR/channel, so we can get and set the appropriate flags
>+  nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(aRequestingContext);
>+  nsCOMPtr<nsIDocShellTreeItem> currentDocShellTreeItem(do_QueryInterface(docShell));
>+  if (!currentDocShellTreeItem) {
>+    return NS_OK;
>+  }
>+  nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot;
>+  currentDocShellTreeItem->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot));
>+  NS_ASSERTION(sameTypeRoot, "No document shell root tree item from document shell tree item!");
>+
>+  // now get the document from sameTypeRoot
>+  nsCOMPtr<nsIDocument> rootDoc = do_GetInterface(sameTypeRoot);
>+  NS_ASSERTION(rootDoc, "No root document from document shell root tree item.");
>+
>+  nsCOMPtr<nsIScriptObjectPrincipal> prin = do_QueryInterface(rootDoc);
>+  nsCOMPtr<nsIPrincipal> rootPrincipal = prin->GetPrincipal();
>+  NS_ASSERTION(rootPrincipal, "No root principal from root document");
>+  nsCOMPtr<nsIURI> rootUri;
>+  rootPrincipal->GetURI(getter_AddRefs(rootUri));
>+  NS_ASSERTION(rootUri, "No root uri from root principal");
>+  bool rootIsHttps;
>+  rv = rootUri->SchemeIs("https", &rootIsHttps);
>+  if (NS_FAILED(rv)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+
>+
>+  // Check the channel against the nsDocShell:mMixedContentChannel to see
>+  // if they are the same.  If they are the same, the user has overriden
>+  // the block.  If they are not the same, block the content.
>+  nsCOMPtr<nsIChannel> channel = rootDoc->GetChannel();
>+  NS_ASSERTION(channel, "\n\nNo channel from the rootDoc--------------.\n\n\n");
>+  nsCOMPtr<nsIDocShell> rootDocShell = do_GetInterface(sameTypeRoot);
>+  NS_ASSERTION(rootDocShell, "No root docShell from document shell root tree item.");
>+
>+  bool userOverride = false;
>+  nsCOMPtr<nsIChannel> mixedChannel;
>+  rv = rootDocShell->GetMixedContentChannel(getter_AddRefs(mixedChannel));
>+  if (NS_SUCCEEDED(rv) && (mixedChannel == channel))
>+  {
>+    userOverride = true;
>+  }

So I think we should have helper method in nsIDocShell which checks the top level same type docshell and gets the state from there.
This all would become something like
nsCOMPtr<nsIDocShell> docShell = NS_CP_GetDocShellFromContext(aRequestingContext);
NS_ENSURE_TRUE(docShell, NS_OK);
bool rootHasSecureConnection = false;
bool allowMixedContent = false;
docShell->GetAllowMixedContentOnSecurePage(&rooHasSecureConnection, &allowMixedContent);

And then replace s/userOverride/allowMixedContent/ and s/rootIsHttps/rootHasSecureConnection/


>+nsDocShell::SetMixedContentChannel(nsIChannel *aMixedContentChannel)
nsIChannel*

>+{
>+    mMixedContentChannel = aMixedContentChannel;
>+    return NS_OK;
>+}
Should we check that aMixedContentChannel is https?
And warn if this is set for non-top-level content docshell


>+
>+NS_IMETHODIMP
>+nsDocShell::GetMixedContentChannel(nsIChannel **aMixedContentChannel)
>+{
>+    NS_ENSURE_ARG_POINTER(aMixedContentChannel);
>+    *aMixedContentChannel = mMixedContentChannel;
>+    if (mMixedContentChannel) NS_ADDREF(*aMixedContentChannel);
>+    return NS_OK;
>+}
So this would become GetAllowMixedContentOnSecurePage and would do the channel checks

Comment 20

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

Comment on attachment 697367 [details] [diff] [review]
Plumbing for Mixed Content Blocker Doorhanger and User Override v5


> 
>+    if (mLoadType == LOAD_MIXED_CONTENT) {
>+          rv = SetMixedContentChannel(channel);
>+          NS_ENSURE_SUCCESS(rv, rv);
>+    }
  else {
      SetMixedContentChannel(nullptr);
  }
?

Comment 21

[Tanvi Vyas[:tanvi]]

Attachment: Plumbing for Mixed Content Blocker Doorhanger and User Override v6

All comments addressed.  Thanks Olli!

(In reply to Olli Pettay [:smaug] from comment #19)
> Comment on attachment 697367 [details] [diff] [review]
> Plumbing for Mixed Content Blocker Doorhanger and User Override v5
> >+nsDocShell::SetMixedContentChannel(nsIChannel *aMixedContentChannel)
> nsIChannel*
> 
> >+{
> >+    mMixedContentChannel = aMixedContentChannel;
> >+    return NS_OK;
> >+}
> Should we check that aMixedContentChannel is https?
> And warn if this is set for non-top-level content docshell
> 
I checked if the channel is associated with the root doc shell, and warn if it isn't.  The channel might actually be associated with an http page though (http page with https iframe.  Example - http://people.mozilla.com/~tvyas/mixediframe2.html.  See bug 824871).

Comment 22

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

Comment on attachment 697678 [details] [diff] [review]
Plumbing for Mixed Content Blocker Doorhanger and User Override v6


>+  // Get the root document from the docshell
>+  nsCOMPtr<nsIDocShellTreeItem> currentDocShellTreeItem(do_QueryInterface(docShell));
>+  NS_ASSERTION(currentDocShellTreeItem, "No DocShellTreeItem from docshell");  
>+  nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot;
>+  currentDocShellTreeItem->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot));
>+  NS_ASSERTION(sameTypeRoot, "No document shell root tree item from document shell tree item!");
>+  nsCOMPtr<nsIDocument> rootDoc = do_GetInterface(sameTypeRoot);
>+  NS_ASSERTION(rootDoc, "No root document from document shell root tree item.");
Ah, I had hoped that we wouldn't need to do this rootDoc stuff, but we do.
>+nsDocShell::GetMixedContentChannel(nsIChannel **aMixedContentChannel)
>+{
>+    NS_ENSURE_ARG_POINTER(aMixedContentChannel);
>+    *aMixedContentChannel = mMixedContentChannel;
>+    if (mMixedContentChannel) NS_ADDREF(*aMixedContentChannel);
NS_IF_ADDREF(*aMixedContentChannel = mMixedContentChannel);


>+NS_IMETHODIMP
>+nsDocShell::GetAllowMixedContentAndConnectionData(bool *rootHasSecureConnection, bool *allowMixedContent)
bool* aRootHas..., bool* aAllowMixed...

And initialize both out params to false right at the beginning of the method.


>+{
>+  nsCOMPtr<nsIDocShellTreeItem> currentDocShellTreeItem = static_cast<nsIDocShellTreeItem *>(this);
>+  NS_ASSERTION(currentDocShellTreeItem, "No DocShellTreeItem from docshell");
>+
>+  nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot;
>+  currentDocShellTreeItem->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot));
>+  NS_ASSERTION(sameTypeRoot, "No document shell root tree item from document shell tree item!");
>+
>+  // now get the document from sameTypeRoot
>+  nsCOMPtr<nsIDocument> rootDoc = do_GetInterface(sameTypeRoot);
>+  NS_ASSERTION(rootDoc, "No root document from document shell root tree item.");
>+
>+  nsCOMPtr<nsIScriptObjectPrincipal> prin = do_QueryInterface(rootDoc);
>+  nsCOMPtr<nsIPrincipal> rootPrincipal = prin->GetPrincipal();

rootDoc->NodePrincipal();


>+  nsresult rv = rootUri->SchemeIs("https", rootHasSecureConnection);
>+  if (NS_FAILED(rv)) {
>+    return NS_ERROR_FAILURE;
>+  }
NS_ENSURE_SUCCESS(rv, rv);

>+
>+  // Check the root doc's channel against the root docShell's mMixedContentChannel to see
>+  // if they are the same.  If they are the same, the user has overriden
>+  // the block.
>+
>+  nsCOMPtr<nsIChannel> channel = rootDoc->GetChannel();
>+  NS_ASSERTION(channel, "No channel from the rootDoc.");
>+  nsCOMPtr<nsIDocShell> rootDocShell = do_GetInterface(sameTypeRoot);
>+  NS_ASSERTION(rootDocShell, "No root docShell from document shell root tree item.");
Not useful assertion. We'll just crash anyway right after this code

>+
>+
extra newline

>+  nsCOMPtr<nsIChannel> mixedChannel;
>+  rv = rootDocShell->GetMixedContentChannel(getter_AddRefs(mixedChannel));
>+  if (NS_SUCCEEDED(rv) && (mixedChannel == channel))
>+  {
>+    *allowMixedContent = true;
>+  }
Perhaps
*aAllowMixedContent = mixedChannel && (mixedChannel == rootDoc->GetChannel())

Comment 23

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

Comment on attachment 697678 [details] [diff] [review]
Plumbing for Mixed Content Blocker Doorhanger and User Override v6


> NS_IMETHODIMP
>+nsDocShell::SetMixedContentChannel(nsIChannel* aMixedContentChannel)
>+{
>+
>+     // Get the root docshell.
>+     nsCOMPtr<nsIDocShellTreeItem> root;
>+     GetSameTypeRootTreeItem(getter_AddRefs(root));
>+     NS_ENSURE_TRUE(root, NS_ERROR_FAILURE);
>+     NS_WARN_IF_FALSE(
>+       root.get() == static_cast<nsIDocShellTreeItem *>(this), 
>+       "Setting mMixedContentChannel on a docshell that is not the root docshell"
>+     );
All the root docshell usage should be inside
#ifdef DEBUG
#endif

Comment 24

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

And no need for NS_ENSURE_TRUE(root, NS_ERROR_FAILURE); there

Comment 25

[Tanvi Vyas[:tanvi]]

Attachment: Plumbing for Mixed Content Blocker Doorhanger and User Override v7

Comments addressed.  Thanks!

Comment 26

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

Comment on attachment 697712 [details] [diff] [review]
Plumbing for Mixed Content Blocker Doorhanger and User Override v7


>+nsDocShell::GetMixedContentChannel(nsIChannel **aMixedContentChannel)
>+{
>+    NS_ENSURE_ARG_POINTER(aMixedContentChannel);
>+    *aMixedContentChannel = mMixedContentChannel;
>+    NS_IF_ADDREF(*aMixedContentChannel = mMixedContentChannel);
No need to assign twice *aMixedContentChannel = mMixedContentChannel.
Just keep NS_IF_ADDREF(*aMixedContentChannel = mMixedContentChannel);


>     if (aLoadType == LOAD_NORMAL ||
>+        //aLoadType == LOAD_MIXED_CONTENT || //smaug - do you know if this needs to be here?
Shouldn't be needed. We should do full (re)load, not just load #someid

Comment 27

[Tanvi Vyas[:tanvi]]

Attachment: Plumbing for Mixed Content Blocker Doorhanger and User Override v8

(In reply to Olli Pettay [:smaug] from comment #26)
> Comment on attachment 697712 [details] [diff] [review]
> Plumbing for Mixed Content Blocker Doorhanger and User Override v7
> 
> 
> >+nsDocShell::GetMixedContentChannel(nsIChannel **aMixedContentChannel)
> >+{
> >+    NS_ENSURE_ARG_POINTER(aMixedContentChannel);
> >+    *aMixedContentChannel = mMixedContentChannel;
> >+    NS_IF_ADDREF(*aMixedContentChannel = mMixedContentChannel);
> No need to assign twice *aMixedContentChannel = mMixedContentChannel.
> Just keep NS_IF_ADDREF(*aMixedContentChannel = mMixedContentChannel);
> 
Ah yes!  Sorry, I forgot to remove that.

Comments addressed.  Carrying over r+.  Thank you Olli!

Comment 28

[Tanvi Vyas[:tanvi]]

Attachment: Handle document.open case when user allows mixed content v1

Hi Olli,

Here is a patch for the document.open() edge case where we have a new channel.  It is very short ;)

I wrote some test cases and ran into a different problem.  The patches work with a page like this - 
https://people.mozilla.com/~tvyas/mixeddocument.html
mixeddocument.html has http script, so the user is asked if they want to override the blocking.  The user can override and the script will load.  Clicking on the "New Document" button creates a new document with document.open.  Mixed active content will also load on the new document.

The problem is with this test case - 
https://people.mozilla.com/~tvyas/mixeddocument2.html
mixeddocument2.html doesn't have mixed content on the main page.  It only has it on the new document.  Clicking on new document invokes the mixed content blocker doorhanger.  Clicking "disable protection", however, does nothing.  This is because we have set a load type of:
LOAD_MIXED_CONTENT = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_ALLOW_MIXED_CONTENT),

If we were to use LOAD_RELOAD_NORMAL (which we already discussed and decided we didn't want to do), then this wouldn't be a problem.  Since we use LOAD_CMD_NORMAL, we end up stuck here in nsDocShell::InternalLoad and return NS_ERROR_FAILURE:
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8418

What should we do?

Thanks!

Comment 29

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

Comment on attachment 697793 [details] [diff] [review]
Handle document.open case when user allows mixed content v1

You'll get warning if shell isn't the root shell.
Perhaps GetAllowMixedContentAndConnectionData could return yet 3rd flag
indicating whether the current shell is the root, and if so, 
set the new channel. Ugly yes, but should work.

Comment 30

[Tanvi Vyas[:tanvi]]

Attachment: Handle document.open case when user allows mixed content v2

This patch handles the document.open() case.  It also changes the nsDocShellLoadType from:

LOAD_MIXED_CONTENT = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_ALLOW_MIXED_CONTENT)

to:
LOAD_RELOAD_ALLOW_MIXED_CONTENT = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_RELOAD, nsIWebNavigation::LOAD_FLAGS_ALLOW_MIXED_CONTENT)

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

Here are some test cases that I used to test the document.open() behavior:
1)
https://people.mozilla.com/~tvyas/mixeddocument.html - Mixed content on the original page and the document.open() page.
https://people.mozilla.com/~tvyas/iframedocumentopen.html - mixeddocument.html iframed.


https://people.mozilla.com/~tvyas/mixeddocument2.html - Mixed content only on the new document page.
https://people.mozilla.com/~tvyas/iframedocumentopen3.html - mixeddocument2.html iframed.  In this case, when we click the button to open the new document in an iframe, we get the mixed content doorhanger.  When we click "disable protection" the page reloads and we get the button again.  Clicking on the button to create a new document, loads the mixed content.  I think this is the right behavior, but if others would like to take a look and chime in about this case, that would be great.

Thanks!

Comment 31

[Tanvi Vyas[:tanvi]]

Attachment: [WIP] Mochitests

First mochitest; working on adding more tests to the patch.  Feedback is welcome, since this is the first time I'm writing chrome browser mochitests and there may be some best practices and techniques that I am missing.

Thanks!

Comment 32

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests

Browser Chrome Mochitests.  I need to do one more for the document.open() mixed content case.

Comment 33

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v1

Added two document.open tests.

Comment 34

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 698557 [details] [diff] [review]
Mochitests v1

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

::: content/base/test/chrome/browser_bug822367.js
@@ +2,5 @@
> + * User Override Mixed Content Block - Tests for Bug 822367
> + */
> +
> +
> +const pref_display = "security.mixed_content.block_display_content";

I think consts are usually in all caps, but maybe not, and maybe it's not a big deal.

@@ +9,5 @@
> +var origBlockActive;
> +var gTestBrowser = null;
> +
> +function MixedTestsCompleted() {
> +    gBrowser.removeCurrentTab();

Looks like the rest of the file is two-spaces indented, so you'll want to be consistent here.

@@ +30,5 @@
> +  newTab.linkedBrowser.stop()
> +
> +  // Mixed Script Test
> +  gTestBrowser.addEventListener("load", MixedTest1A, true);
> +  var url = "https://example.com/tests/content/base/test/test_bug822367_1.html";

I recommend something like what's going on in https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_pluginnotification.js#3 and https://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_pluginnotification.js#370

@@ +37,5 @@
> +
> +// Mixed Script Test
> +function MixedTest1A() {
> +  gTestBrowser.removeEventListener("load", MixedTest1A, true);
> +  gTestBrowser.addEventListener("load", MixedTest1B, true);

This adding/removing will get cumbersome fast. browser_pluginnotification.js uses pageLoad and prepareTest to do something similar.

@@ +79,5 @@
> +  ok(notification, "Mixed Content Doorhanger doesn't appear for test 3");
> +  notification.mainAction.callback();
> +}
> +function MixedTest3B() {
> +  waitForCondition(function() content.document.getElementById('p1').innerHTML == "hello", MixedTest3C, "Waited too long for mixed script to run in Test 3");

I think this being only one line isn't a problem. However, is there some event you could have the content fire and explicitly notify the test instead of waiting for the innerHTML to be set?

@@ +168,5 @@
> +  ok(content.document.getElementById('f1').contentDocument.getElementById('p1').innerHTML == "hello","Mixed script didn't load in Test 6");
> +  MixedTestsCompleted();
> +}
> +
> +function waitForCondition(condition, nextTest, errorMsg) {

We should probably just hoist this into the test framework, but that would be another bug anyway.

@@ +183,5 @@
> +  }, 100);
> +  var moveOn = function() { clearInterval(interval); nextTest(); };
> +}
> +
> +registerCleanupFunction(function() {

This should probably go at the beginning of the function test().

Comment 35

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v2

Thanks David for your comments!  Here is are updated Mochitests.

Comment 36

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

Comment on attachment 698349 [details] [diff] [review]
Handle document.open case when user allows mixed content v2


>-nsDocShell::GetAllowMixedContentAndConnectionData(bool* aRootHasSecureConnection, bool* aAllowMixedContent)
>+nsDocShell::GetAllowMixedContentAndConnectionData(bool* aRootHasSecureConnection, bool* aAllowMixedContent, bool* isRootDocShell)
aIsRootDocShell

>+  nsCOMPtr<nsIDocShellTreeItem> root;
>+  GetSameTypeRootTreeItem(getter_AddRefs(root));
>+  if (root.get() == static_cast<nsIDocShellTreeItem *>(this)) {
>+    *isRootDocShell = true;
>+  }
Perhaps just *isRootDocShell = root.get() == static_cast<nsIDocShellTreeItem *>(this);

>   nsCOMPtr<nsIDocShellTreeItem> currentDocShellTreeItem = static_cast<nsIDocShellTreeItem *>(this);
>   NS_ASSERTION(currentDocShellTreeItem, "No DocShellTreeItem from docshell");
> 
>   nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot;
>   currentDocShellTreeItem->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot));
>   NS_ASSERTION(sameTypeRoot, "No document shell root tree item from document shell tree item!");
So you get sametyperoot again here.

>@@ -9642,16 +9655,17 @@ nsresult nsDocShell::DoChannelLoad(nsICh
>         break;
> 
>     case LOAD_NORMAL_BYPASS_CACHE:
>     case LOAD_NORMAL_BYPASS_PROXY:
>     case LOAD_NORMAL_BYPASS_PROXY_AND_CACHE:
>     case LOAD_RELOAD_BYPASS_CACHE:
>     case LOAD_RELOAD_BYPASS_PROXY:
>     case LOAD_RELOAD_BYPASS_PROXY_AND_CACHE:
>+    case LOAD_RELOAD_ALLOW_MIXED_CONTENT: //smaug - is this in the right place?
Yes, it is one place to add check

>     LOAD_NORMAL_BYPASS_PROXY_AND_CACHE = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE | nsIWebNavigation::LOAD_FLAGS_BYPASS_PROXY),
>     LOAD_RELOAD_NORMAL = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_RELOAD, nsIWebNavigation::LOAD_FLAGS_NONE),
>     LOAD_RELOAD_BYPASS_CACHE = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_RELOAD, nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE),
>     LOAD_RELOAD_BYPASS_PROXY = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_RELOAD, nsIWebNavigation::LOAD_FLAGS_BYPASS_PROXY),
>+    LOAD_RELOAD_ALLOW_MIXED_CONTENT = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_RELOAD, nsIWebNavigation::LOAD_FLAGS_ALLOW_MIXED_CONTENT),
Shouldn't we add nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE


>   /**
>    * Checks whether the channel associated with the root docShell is equal to
>    * mMixedContentChannel. If they are the same, allowMixedContent is set to true.
>    * Checks if the root document has a secure connection. If it is, sets 
>    * rootHasSecureConnection to true.
>    */
>-  void GetAllowMixedContentAndConnectionData(out boolean rootHasSecureConnection, out boolean allowMixedContent);
>+  void GetAllowMixedContentAndConnectionData(out boolean rootHasSecureConnection, out boolean allowMixedContent, out boolean isRootDocShell);
Add a short comment about the 3rd parameter

Comment 37

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

Comment on attachment 698969 [details] [diff] [review]
Mochitests v2

>@@ -592,16 +592,25 @@ MOCHITEST_FILES_B = \
> 		test_XHR_parameters.html \
> 		test_ipc_messagemanager_blob.html \
> 		test_mixed_content_blocker.html \
> 		file_mixed_content_main.html \
> 		file_mixed_content_server.sjs \
>     test_mixed_content_blocker_bug803225.html \
>     file_mixed_content_main_bug803225.html \
>     file_mixed_content_main_bug803225_websocket_wsh.py \
>+    test_bug822367_1.html \
>+    test_bug822367_1.js \
>+    test_bug822367_2.html \
>+    test_bug822367_3.html \
>+    test_bug822367_4.html \
>+    test_bug822367_4.js \
>+    test_bug822367_4B.html \
>+    test_bug822367_5.html \
>+    test_bug822367_6.html \
These are not test files themselves, so they shouldn't start with test_ prefix.
(I'm surprised you don't get orange when running mochitest)
So, could you change filenames s/test_/file_/

Comment 38

[Tanvi Vyas[:tanvi]]

Attachment: Handle document.open case when user allows mixed content v3

(In reply to Olli Pettay [:smaug] from comment #36)
> Comment on attachment 698349 [details] [diff] [review]
> Handle document.open case when user allows mixed content v2
> 
> 
> >@@ -9642,16 +9655,17 @@ nsresult nsDocShell::DoChannelLoad(nsICh
> >         break;
> > 
> >     case LOAD_NORMAL_BYPASS_CACHE:
> >     case LOAD_NORMAL_BYPASS_PROXY:
> >     case LOAD_NORMAL_BYPASS_PROXY_AND_CACHE:
> >     case LOAD_RELOAD_BYPASS_CACHE:
> >     case LOAD_RELOAD_BYPASS_PROXY:
> >     case LOAD_RELOAD_BYPASS_PROXY_AND_CACHE:
> >+    case LOAD_RELOAD_ALLOW_MIXED_CONTENT: //smaug - is this in the right place?
> Yes, it is one place to add check
> 
> >     LOAD_NORMAL_BYPASS_PROXY_AND_CACHE = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE | nsIWebNavigation::LOAD_FLAGS_BYPASS_PROXY),
> >     LOAD_RELOAD_NORMAL = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_RELOAD, nsIWebNavigation::LOAD_FLAGS_NONE),
> >     LOAD_RELOAD_BYPASS_CACHE = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_RELOAD, nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE),
> >     LOAD_RELOAD_BYPASS_PROXY = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_RELOAD, nsIWebNavigation::LOAD_FLAGS_BYPASS_PROXY),
> >+    LOAD_RELOAD_ALLOW_MIXED_CONTENT = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_RELOAD, nsIWebNavigation::LOAD_FLAGS_ALLOW_MIXED_CONTENT),
> Shouldn't we add nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE
> 
I'm not sure about this.  I call BrowserReloadWithFlags with the LOAD_FLAGS_ALLOW_MIXED_CONTENT.  Should I call it with both flags?

Comment 39

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests v3

Changed test files from test_* to file_*.  Carrying over r+.

Comment 40

[Tanvi Vyas[:tanvi]]

Attachment: Handle document.open case when user allows mixed content v4

Comment 41

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

Comment on attachment 699504 [details] [diff] [review]
Handle document.open case when user allows mixed content v4

remove
//nsCOMPtr<nsIDocShell> rootDocShell = do_GetInterface(sameTypeRoot);

Comment 42

[Tanvi Vyas[:tanvi]]

Attachment: Handle document.open case when user allows mixed content v5

Removed commented code.  Carrying over r+.

Comment 43

[Tanvi Vyas[:tanvi]]

Attachment: Handle document.open case when user allows mixed content v6

(In reply to Tanvi Vyas [:tanvi] from comment #42)
> Created attachment 699510 [details] [diff] [review]
> Handle document.open case when user allows mixed content v5
> 
> Removed commented code.  Carrying over r+.

Comment 44

[Tanvi Vyas[:tanvi]]

Here is a try push for all 5 patches (3 here, 1 in bug 822366 and 1 in bug 822371):  
https://tbpl.mozilla.org/?tree=Try&rev=70d84bddb224

Here is a try push for just two of the patches in this bug (the mochitests don't make sense or work without the patches in the othr bug).

Both tries look good so far.  If we get an r+ on the last remaining patch in bug 822371, I'll land all this at once tomorrow.  If not, I will just land 2 patches from this bug and wait to land the mochitests until the patch in bug 822371 is ready.

Comment 45

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #44)
> Here is a try push for just two of the patches in this bug (the mochitests
> don't make sense or work without the patches in the other bug).
> 
> 
Look like I didn't include the link: https://tbpl.mozilla.org/?tree=Try&rev=907b926f0d51

Comment 46

[Ed Morley [:emorley]]

Hi Tanvi :-)

Not sure if you've seen the tryserver machine hours usage page before (http://people.mozilla.org/~catlee/highscores/highscores.html) but you're in the #1 spot for the last week (mainly for pushes for this bug).

I wouldn't want to make anyone reduce the amount they use Try if it is actually helping - however glancing at the pushes it seems like many of them may have been including unnecessary platforms/testsuites - eg: when iterating to fix oranges that affect all platforms. (All of your pushes are "-p all -u all", which uses in excess of 120 hours of machine time per push - whereas for https://tbpl.mozilla.org/?tree=Try&rev=deafe7e5cc20 you could have used "-p linux64 -u mochitest-browser-chrome" which would have only used 5 hours per iteration).

If possible in the future please consider only testing on one platform and/or a subset of the unit tests when you don't need all of the results, using http://trychooser.pub.build.mozilla.org/

The TryChooser page now also includes the pending job counts - so it can help you pick the platform with the lowest wait time, helping you get your results sooner.

Thank you :-)

Comment 47

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #46)
> could have used "-p linux64 -u mochitest-browser-chrome"

Sorry, "-u mochitest-bc" even.

Comment 48

[Ed Morley [:emorley]]

(Sorry for yet another message)

Meant to say - it's also possibly to cancel Try runs if you no longer need the results for jobs that are still queued/running:
https://groups.google.com/d/msg/mozilla.dev.platform/iengKcyD504/DPJeGxOpGw0J

Comment 49

[Tanvi Vyas[:tanvi]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #46)
> Hi Tanvi :-)
> 
> Not sure if you've seen the tryserver machine hours usage page before
> (http://people.mozilla.org/~catlee/highscores/highscores.html) but you're in
> the #1 spot for the last week (mainly for pushes for this bug).
> 
> I wouldn't want to make anyone reduce the amount they use Try if it is
> actually helping - however glancing at the pushes it seems like many of them
> may have been including unnecessary platforms/testsuites - eg: when
> iterating to fix oranges that affect all platforms. (All of your pushes are
> "-p all -u all", which uses in excess of 120 hours of machine time per push
> - whereas for https://tbpl.mozilla.org/?tree=Try&rev=deafe7e5cc20 you could
> have used "-p linux64 -u mochitest-browser-chrome" which would have only
> used 5 hours per iteration).
> 
> If possible in the future please consider only testing on one platform
> and/or a subset of the unit tests when you don't need all of the results,
> using http://trychooser.pub.build.mozilla.org/
> 
> The TryChooser page now also includes the pending job counts - so it can
> help you pick the platform with the lowest wait time, helping you get your
> results sooner.
> 
> Thank you :-)

Hi Ed,

Sorry for the multiple pushes to try!  I thought I was close to landing and wanted a clean try run first.  There were multiple small changes to bug 822371, and I pushed to try for each one, thinking it was the last change (it wasn't to resolve oranges). I will be more careful in the future and also start by just pushing to one platform if a patch is not ready to land. Thanks!

Comment 50

[Tanvi Vyas[:tanvi]]

Attachment: Don't call OnSecurityChange unless we really need to

Comment 51

[Justin Dolske [:Dolske]]

Comment on attachment 702613 [details] [diff] [review]
Don't call OnSecurityChange unless we really need to

This patch doesn't compile. I don't think it's really necessary, anyway.

Comment 52

[Justin Dolske [:Dolske]]

Attachment: Plumbing for Mixed Content Blocker Doorhanger and User Override v9

Adds 2 new nsIWebProgress flags per discussion in bug 822371.

Comment 53

[Tanvi Vyas[:tanvi]]

Thanks Justin for your help :)  Your patch makes the changes easier than I expected - the hasMixedActiveContentLoaded and hasMixedActiveContentBlocked still exist, and the state flags are updated with more information in addition.  I imagined the two booleans would be removed.  But keeping them makes this much simpler.  I will provide more detailed feedback tomorrow.  Thanks!

Comment 54

[Tanvi Vyas[:tanvi]]

Attachment: Don't call OnSecurityChange unless we really need to v2

(In reply to Justin Dolske [:Dolske] from comment #51)
> Comment on attachment 702613 [details] [diff] [review]
> Don't call OnSecurityChange unless we really need to
> 
> This patch doesn't compile. I don't think it's really necessary, anyway.

I was calling SetHasMixed... instead of GetHasMixed... and without ().  Updated the patch and it compiles now.

Comment 55

[Tanvi Vyas[:tanvi]]

Attachment: Add mixed content state flags to nsIWebProgressListener and Use them

Including mixed content information in the actual Security State, per discussions in bug 822371.

Justin, thanks for your help!  I have separated out the code that has already been reviewed with the new code in uriloader/base/nsIWebProgressListener.idl, /security/manager/boot/src/nsSecureBrowserUIImpl.  And the updates needed in content/base/src/nsMixedContentBlocker.cpp.

r? to smaug for nsIWebProgressListener and nsMixedContentBlocker

feedback? to keeler for the PSM changes.

Comment 56

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

Comment on attachment 705684 [details] [diff] [review]
Add mixed content state flags to nsIWebProgressListener and Use them


>+  uint32_t State;
Could you please initialize this to some value

>-nsSecureBrowserUIImpl::MapInternalToExternalState(uint32_t* aState, lockIconState lock, bool ev)
>+nsSecureBrowserUIImpl::MapInternalToExternalState(uint32_t* aState, lockIconState lock, bool ev, nsWeakPtr aWindow)
this is odd. Passing nsWeakPtr as value. Perhaps nsWeakPtr*
But actually, since you seem to pass mWindow always, why not just use mWindow directly?


>+  nsIDocShell *docShell = piwin->GetDocShell();
nsIDocShell*

>+  if (!docShell)
>+    return NS_ERROR_FAILURE;
This file seems to use this coding style :(  (so no need to change)

The change to GetState handling is rather major because you make atm always succeeding method to
return errors in some cases. I'm worried that the change may cause hard to find regressions in some odd cases.
Could we just always return NS_OK?

That addressed, r=me

Comment 57

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 705684 [details] [diff] [review]
Add mixed content state flags to nsIWebProgressListener and Use them

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

Basically what Olli said - I would use mWindow directly instead of passing it as an argument.

::: security/manager/boot/src/nsSecureBrowserUIImpl.cpp
@@ +237,5 @@
>  nsSecureBrowserUIImpl::GetState(uint32_t* aState)
>  {
>    ReentrantMonitorAutoEnter lock(mReentrantMonitor);
> +
> +  return MapInternalToExternalState(aState, mNotifiedSecurityState, mNotifiedToplevelIsEV, mWindow);

Do we even need to pass mWindow? The is the same class, right?

@@ +1409,5 @@
>  
>    if (temp_ToplevelEventSink)
>    {
>      uint32_t newState = STATE_IS_INSECURE;
> +

If you end up not passing mWindow, you won't need to change the line three below, so you should probably take out this newline as well.

Comment 58

[Tanvi Vyas[:tanvi]]

Attachment: Add mixed content state flags to nsIWebProgressListener and Use them v2

(In reply to Olli Pettay [:smaug] from comment #56)
> Comment on attachment 705684 [details] [diff] [review]
> Add mixed content state flags to nsIWebProgressListener and Use them
> 
> 
> >+  uint32_t State;
> Could you please initialize this to some value
> 
> >-nsSecureBrowserUIImpl::MapInternalToExternalState(uint32_t* aState, lockIconState lock, bool ev)
> >+nsSecureBrowserUIImpl::MapInternalToExternalState(uint32_t* aState, lockIconState lock, bool ev, nsWeakPtr aWindow)
> this is odd. Passing nsWeakPtr as value. Perhaps nsWeakPtr*
> But actually, since you seem to pass mWindow always, why not just use
> mWindow directly?
> 
> 
> >+  nsIDocShell *docShell = piwin->GetDocShell();
> nsIDocShell*
> 
> >+  if (!docShell)
> >+    return NS_ERROR_FAILURE;
> This file seems to use this coding style :(  (so no need to change)
> 
> The change to GetState handling is rather major because you make atm always
> succeeding method to
> return errors in some cases. I'm worried that the change may cause hard to
> find regressions in some odd cases.
> Could we just always return NS_OK?
> 
> That addressed, r=me

Comments addressed.  Carrying over r+ from smaug and separating the PSM code for review by bsmith.  Thanks!

Comment 59

[Tanvi Vyas[:tanvi]]

Attachment: Use new mixed content state flags in PSM v2

Updated after feedback from dkeeler and review from smaug.  Marking r? to bsmith.

Comment 60

[Tanvi Vyas[:tanvi]]

Attachment: Use new mixed content state flags in PSM v3

(In reply to Tanvi Vyas [:tanvi] from comment #59)
> Created attachment 706156 [details] [diff] [review]
> Use new mixed content state flags in PSM v2
> 
> Updated after feedback from dkeeler and review from smaug.  Marking r? to
> bsmith.

Removing commented code.

Comment 61

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 706160 [details] [diff] [review]
Use new mixed content state flags in PSM v3

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

::: security/manager/boot/src/nsSecureBrowserUIImpl.cpp
@@ +292,5 @@
> +
> +  nsIDocShell* docShell = piwin->GetDocShell();
> +  if (!docShell)
> +    return NS_OK;
> +

Above, you just silently ignore errors. Do these errors ever occur in reality? (Dolske might be a better person to ask about this.) It seems like it should be impossible to get here if any of the above functions fail. then I would prefer that we handle them by MOZ_CRASHing.

@@ +302,5 @@
> +  bool haveBlockedMixedActive;
> +  rv = docShell->GetHasMixedActiveContentBlocked(&haveBlockedMixedActive);
> +  if (NS_FAILED(rv))
> +    return NS_OK;
> +

These two methods should be annotated [infallible] in the IDL so that the return types of these functions change to void.

Also, you should mix in the flags for passive content here too.

Nit: Do we always call GetHasMixedActiveContentLoaded and GetHasMixedActiveContentBlocked together like this? If so, then we should combine those functions into one function that returns all the flags, already ORed together.

Comment 62

[Tanvi Vyas[:tanvi]]

Thanks Brian!  See comments inline.

(In reply to Brian Smith (:bsmith) from comment #61)
> Comment on attachment 706160 [details] [diff] [review]
> Use new mixed content state flags in PSM v3
> 
> Review of attachment 706160 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/manager/boot/src/nsSecureBrowserUIImpl.cpp
> @@ +292,5 @@
> > +
> > +  nsIDocShell* docShell = piwin->GetDocShell();
> > +  if (!docShell)
> > +    return NS_OK;
> > +
> 
> Above, you just silently ignore errors. Do these errors ever occur in
> reality? (Dolske might be a better person to ask about this.) It seems like
> it should be impossible to get here if any of the above functions fail. then
> I would prefer that we handle them by MOZ_CRASHing.
> 
I changed these to NS_OK per smaug's comments in comment 56, since GetState() was never returning an error before.  The worst that can happen here if there is an error (no dochsell, or calling GetHasMixedActiveContentLoaded/Blocked fails) is that the mixed active state flag won't get set (which is not good but doesn't necessarily warrant a crash?).

Instead, I can use an assertion or return an error.  Would you prefer MOZ_CRASH() over these other two options?

> @@ +302,5 @@
> > +  bool haveBlockedMixedActive;
> > +  rv = docShell->GetHasMixedActiveContentBlocked(&haveBlockedMixedActive);
> > +  if (NS_FAILED(rv))
> > +    return NS_OK;
> > +
> 
> These two methods should be annotated [infallible] in the IDL so that the
> return types of these functions change to void.
> 
I'll do this in an updated patch.


> Also, you should mix in the flags for passive content here too.
> 
We aren't doing anything with passive content yet and hence we don't have specific document flags for passive content.  I do set these flags in nsMixedContentBlocker.

> Nit: Do we always call GetHasMixedActiveContentLoaded and
> GetHasMixedActiveContentBlocked together like this? If so, then we should
> combine those functions into one function that returns all the flags,
> already ORed together.
We do not always call them at the same time.

Comment 63

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Tanvi Vyas [:tanvi] from comment #62)
> Instead, I can use an assertion or return an error.  Would you prefer
> MOZ_CRASH() over these other two options?

At least MOZ_ASSERT that the functions succeeded.

> We aren't doing anything with passive content yet and hence we don't have
> specific document flags for passive content.  I do set these flags in
> nsMixedContentBlocker.

We do do something for passive content: we change the lock icon to a globe.

We need to make sure that nsSecureBrowserUIImpl sets the BROKEN flag if either ACTIVE_LOADED or the PASSIVE_LOADED flags are set, because we already know that nsSecureBrowserUIImpl's existing logic for setting the BROKEN flag is worse than nsMixedContentBlocker's logic.

Comment 64

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #63)
> We need to make sure that nsSecureBrowserUIImpl sets the BROKEN flag if
> either ACTIVE_LOADED or the PASSIVE_LOADED flags are set,

I mean, we need to add the BROKEN flag if either GetHasMixedActiveContent or GetHasMixedPassiveContent return true.

Comment 65

[Tanvi Vyas[:tanvi]]

Attachment: Add mixed content state flags to nsIWebProgressListener and Use them v3

Olli, per bsmith I have added hasMixedDisplayContentLoaded and hasMixedDisplayContentBlocked flags to the document (see comments 63 and 64).  I've updated this patch and requesting a review.

Here is a diff from the previous version so that you can see what has changed:
http://www.pastebin.mozilla.org/2093369

Thanks!

Comment 66

[Tanvi Vyas[:tanvi]]

Attachment: Use new mixed content state flags in PSM v4

Comments addressed.  bsmith, can I get another review?

Comment 67

[Tanvi Vyas[:tanvi]]

Attachment: Use new mixed content state flags in PSM v5

Talked to bsmith, and updated some nits he pointed out.  He told me to ahead and r+ it.  Thanks Brian!

Comment 68

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

Comment on attachment 707373 [details] [diff] [review]
Add mixed content state flags to nsIWebProgressListener and Use them v3

>+++ b/docshell/base/nsIDocShell.idl
update uuid
(although, maybe it is updated in some other related patch)

Comment 69

[Tanvi Vyas[:tanvi]]

(In reply to Olli Pettay [:smaug] from comment #68)
> Comment on attachment 707373 [details] [diff] [review]
> Add mixed content state flags to nsIWebProgressListener and Use them v3
> 
> >+++ b/docshell/base/nsIDocShell.idl
> update uuid
> (although, maybe it is updated in some other related patch)

Thanks Olli!  Yes, the uuid is updated in the Plumbing for Mixed Content Blocker patch.

Here is a new push to try that includes this bug, bug 822366 and bug 822371:
https://tbpl.mozilla.org/?tree=Try&rev=0ac83df1ce4f

Comment 70

[Tanvi Vyas[:tanvi]]

Attachment: Fix mixed content todo mochitest

Given the new changes in "Use new mixed content state flags in PSM", we end up with a mochitest "failure" where a todo test now works!

Specifically, mixed xhr is categorizes as mixed active content and the state is set to STATE_IS_BROKEN on request.  Yet another patch changing this todo test to an actual test - https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/mochitest/mixedcontent/test_dynDelayedUnsecureXHR.html?force=1#27

Cancelling the previous push to try and created a new one with this change - https://tbpl.mozilla.org/?tree=Try&rev=bd04f60891db

Comment 71

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 707821 [details] [diff] [review]
Fix mixed content todo mochitest

Please remove the todoSecurityState function too, as it is now dead code.

Comment 72

[Tanvi Vyas[:tanvi]]

Attachment: Fix mixed content todo mochitest v2

(In reply to Brian Smith (:bsmith) from comment #71)
> Comment on attachment 707821 [details] [diff] [review]
> Fix mixed content todo mochitest
> 
> Please remove the todoSecurityState function too, as it is now dead code.

Removed.  Thanks Brian!  Carrying over r+.

New push to try (previous one cancelled): https://tbpl.mozilla.org/?tree=Try&rev=21a963539714

Comment 73

[Tanvi Vyas[:tanvi]]

Pushed to Inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/4068ef6c54a6
http://hg.mozilla.org/integration/mozilla-inbound/rev/a704f2af8bf3
http://hg.mozilla.org/integration/mozilla-inbound/rev/0ec7dd6fc516 
http://hg.mozilla.org/integration/mozilla-inbound/rev/6af744ffc384
http://hg.mozilla.org/integration/mozilla-inbound/rev/9c8970c4523f 
http://hg.mozilla.org/integration/mozilla-inbound/rev/6223c1dd15f6
http://hg.mozilla.org/integration/mozilla-inbound/rev/8da4794af394

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4068ef6c54a6
https://hg.mozilla.org/mozilla-central/rev/a704f2af8bf3
https://hg.mozilla.org/mozilla-central/rev/0ec7dd6fc516
https://hg.mozilla.org/mozilla-central/rev/6af744ffc384
https://hg.mozilla.org/mozilla-central/rev/9c8970c4523f
https://hg.mozilla.org/mozilla-central/rev/6223c1dd15f6
https://hg.mozilla.org/mozilla-central/rev/8da4794af394