Closed Bug 906190 Opened 11 years ago Closed 11 years ago

Persist "disable protection" option for Mixed Content Blocker in child tabs

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox23 --- wontfix
firefox24 - wontfix
firefox25 - affected
firefox26 - affected
relnote-firefox --- -

People

(Reporter: tanvi, Assigned: ckerschb)

References

(Blocks 2 open bugs)

Details

(Keywords: ux-efficiency)

Attachments

(4 files, 17 obsolete files)

3.57 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
11.13 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
9.48 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
29.10 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Today we had a meeting where we discussed the persistence of the "disable protection" option of the Mixed Content Blocker.  Bug 902156 will allow the decision to be persisted while a user is on a specific tab and navigating a specific page.  As soon as they navigate to a different domain (or sub-domain) or close the tab, the decision will be lost and no longer persist.

In this bug, we propose that when a user is in one tab and has disabled protection, same-domain links they click from that tab to be opened in a new tab also persist the "disable protection" decision.  For example:
* User goes to https://news.com which has mixed content in one tab
* news.com has mixed active content and the user decides to "disable protection on this page".
* User clicks news articles on the page that open up in new tabs.  These news articles should also allow mixed active content and persist the user decision.

We might be able to do this by looking at the parent of the newly opened tabs.  If they have a parent with the same domain, we would want to persist the disable protection decision (by setting the mixed content channel to the channel associated with the new tab).

We need to investigate how hard or how easy this is to implement.

+++ This bug was initially created as a clone of Bug #902156 +++
Just tested this and wanted to note that Chrome does not have the type of persistence described in comment 0.
Assignee: nobody → mozilla
Attached patch bug_906190_child_tabs.patch (obsolete) — Splinter Review
Please Note, that we don't have channels available yet, hence we use CheckSameOriginURI. Maybe we want to defer the check later.
Attachment #794223 - Flags: feedback?(tanvi)
Comment on attachment 794223 [details] [diff] [review]
bug_906190_child_tabs.patch

># HG changeset patch
># User Christoph Kerschbaumer <mozilla@christophkerschbaumer.com>
># Date 1377200684 25200
># Node ID b7af88f14f9e267bb3b2180994e973034d6a7999
># Parent  fec4a7317a9c905815a93c72fb4d6dc88c28d8eb
>BUG 906190 - Persist 'disable protection' option for Mixed Content Blocker in child tabs
>
>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -1561,16 +1561,19 @@
> 
>               let flags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
>               if (aAllowThirdPartyFixup)
>                 flags |= Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
>               if (aFromExternal)
>                 flags |= Ci.nsIWebNavigation.LOAD_FLAGS_FROM_EXTERNAL;
>               if (aIsUTF8)
>                 flags |= Ci.nsIWebNavigation.LOAD_FLAGS_URI_IS_UTF8;
>+              if (this.docShell && this.docShell.hasMixedActiveContentLoaded)
>+                flags |= Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT;
>+
This won't work exactly right.
Note that without this change, when you navigate to another page on the same domain, this load flag doesn't exist, but we continue to allow mixed active content on the current tab (bug 902156).

Now assume you are on a page with mixed content and the above change has been made.  You disable protection and this load flag gets set.  You navigate to a page on the same domain.  If that page has mixed active content, the mixed active content is allowed and the hasMixedActiveContentLoaded flag is set.  If that page does not have mixed active content, what happens?  I assume that the mixedContentChannel is correctly set to the new channel but the hasMixedActiveContentLoaded flag is false.  The user control-clicks a link on the page that doesn't have mixed active content.  The new tab that opens should have mixed active content allowed, but won't because the previous page did not have the hasMixedActiveContentLoaded state.  Do we want to solve for this case?  If we do, we are going to have to think of another way to persist the disable protection decision instead of the load flags.

The above change also means that a user who has mixed active content allowed by default (via about:config) will have this load flag set when they previously did not.  What are the implications of this?  Maybe nothing, but we should check.


Finally, note that if and when we implement https://bugzilla.mozilla.org/show_bug.cgi?id=893533, we'd have to change this logic to persist the mixed display loaded state too.  Since we don't know when and if we are going to support that, we can ignore that for now and I'll make a note in that bug if we do end up using the hasMixedActiveContentLoaded flag to persist state.


>               try {
>                 b.loadURIWithFlags(aURI, flags, aReferrerURI, aCharset, aPostData);
>               } catch (ex) {
>                 Cu.reportError(ex);
>               }
>             }
> 
>             // We start our browsers out as inactive, and then maintain
>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -4135,23 +4135,41 @@ nsDocShell::LoadURI(const PRUnichar * aU
>         popupState = openOverridden;
>     }
>     nsAutoPopupStatePusher statePusher(popupState);
> 
>     // Don't pass certain flags that aren't needed and end up confusing
>     // ConvertLoadTypeToDocShellLoadInfo.  We do need to ensure that they are
>     // passed to LoadURI though, since it uses them.
>     uint32_t extraFlags = (aLoadFlags & EXTRA_LOAD_FLAGS);
>+    uint32_t mixedContentFlag = (aLoadFlags & LOAD_FLAGS_ALLOW_MIXED_CONTENT);
>     aLoadFlags &= ~EXTRA_LOAD_FLAGS;
> 
>     nsCOMPtr<nsIDocShellLoadInfo> loadInfo;
>     rv = CreateLoadInfo(getter_AddRefs(loadInfo));
>     if (NS_FAILED(rv)) return rv;
>-    
>+
>     uint32_t loadType = MAKE_LOAD_TYPE(LOAD_NORMAL, aLoadFlags);
>+  
>+    /*
>+     * If the user "Disables Protection on This Page", we have to make sure to
>+     * remember the users decision when opening links in new tabs.
>+     * If the mixedContentFlag is set and the new uri passes the same origin
>+     * check with the referring uri, then we can set the loadType to allow
>+     * mixed content.
>+     */
>+    if (mixedContentFlag) {
>+      nsCOMPtr<nsIScriptSecurityManager> secMan = 
>+        do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID);
>+      NS_ENSURE_TRUE(secMan, NS_ERROR_FAILURE);
>+      if (NS_SUCCEEDED(secMan->CheckSameOriginURI(uri, aReferringURI, false))) {
>+        loadType = MAKE_LOAD_TYPE(LOAD_RELOAD_ALLOW_MIXED_CONTENT, aLoadFlags);
>+      }
>+    }
It would be better if we could get the principals and compare them.  We don't have the new channel, but maybe we can get the new principal?

>+
>     loadInfo->SetLoadType(ConvertLoadTypeToDocShellLoadInfo(loadType));
>     loadInfo->SetPostDataStream(postStream);
>     loadInfo->SetReferrer(aReferringURI);
>     loadInfo->SetHeadersStream(aHeaderStream);
> 
>     rv = LoadURI(uri, loadInfo, extraFlags, true);
> 
>     // Save URI string in case it's needed later when
Attachment #794223 - Flags: feedback?(tanvi)
(In reply to Tanvi Vyas [:tanvi] from comment #3)
> Comment on attachment 794223 [details] [diff] [review]
> bug_906190_child_tabs.patch
> 

> >+              if (this.docShell && this.docShell.hasMixedActiveContentLoaded)
> >+                flags |= Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT;
> >+
> This won't work exactly right.
> Note that without this change, when you navigate to another page on the same
> domain, this load flag doesn't exist, but we continue to allow mixed active
> content on the current tab (bug 902156).

What if we use the mixedContentChannel?

 if (this.docShell && this.docShell.mixedContentChannel)


> The above change also means that a user who has mixed active content allowed
> by default (via about:config) will have this load flag set when they
> previously did not.  What are the implications of this?  Maybe nothing, but
> we should check.

I based the patch on the setup on the idea to allow mixedContent when the user "Disables Protection on This Page". Well sort of, now when we use the mixedContentChannel instead of the LOAD_FLAGS_ALLOW_MIXED_CONTENT this flag should only be set if the user explicitly allows mixed content on the site.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> (In reply to Tanvi Vyas [:tanvi] from comment #3)
> > Comment on attachment 794223 [details] [diff] [review]
> > bug_906190_child_tabs.patch
> > 
> 
> > >+              if (this.docShell && this.docShell.hasMixedActiveContentLoaded)
> > >+                flags |= Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT;
> > >+
> > This won't work exactly right.
> > Note that without this change, when you navigate to another page on the same
> > domain, this load flag doesn't exist, but we continue to allow mixed active
> > content on the current tab (bug 902156).
> 
> What if we use the mixedContentChannel?
> 
>  if (this.docShell && this.docShell.mixedContentChannel)
> 
> 
Is there a situation where a mixedContetChannel exists but isn't for the same domain?  Most of the time, we clear it to null.  But is there an edge case we are missing?
I spoke to Olli about the patch.  Instead of propagating the LOAD_FLAGS_ALLOW_MIXED_CONTENT to a load (that may not end up with allowed mixed content) and then later checking if the referrerURI and the new URI match, lets do that check in tabbrowser.xml itself.  Looks like you can use checkSameOriginURI():
http://mxr.mozilla.org/mozilla-central/source/caps/idl/nsIScriptSecurityManager.idl#197
It doesn't seem right to me for tabbrowser to be involved in this at all. If we want to propagate the flag across new tab links, seems like you should add a flag to do so to nsIBrowserDOMWindow.
well ctrl+click and such are all browser chrome features, and if such code wants to
inherit something from the original tab, it should, IMO, happen in the code dealing with opening
the new tab.
Sure, if we want to handle the Ctrl+Click case then we'll need some front-end logic to make the connection. But the target="_blank" case should work without UI involvement, ideally.
Attached patch bug_906190_child_tabs.patch (obsolete) — Splinter Review
Attachment #794223 - Attachment is obsolete: true
Attachment #797516 - Flags: review?(bugs)
Attachment #797517 - Flags: review?(bugs)
Comment on attachment 797516 [details] [diff] [review]
bug_906190_child_tabs.patch

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
This part needs a review also from a browser/ peer.


>+    /*
>+     * If the user "Disables Protection on This Page", we have to make sure to
>+     * remember the users decision when opening links in child tabs [Bug 906190]
>+     */
>+    uint32_t loadType;
>+    if (mixedContentFlag) {
>+      loadType = MAKE_LOAD_TYPE(LOAD_RELOAD_ALLOW_MIXED_CONTENT, aLoadFlags);
>+    } else {
>+      loadType = MAKE_LOAD_TYPE(LOAD_NORMAL, aLoadFlags);
>+    }
>+
LOAD_RELOAD_ALLOW_MIXED_CONTENT isn't quite just LOAD_NORMAL + allow mixed content.


Need to probably change LOAD_RELOAD_ALLOW_MIXED_CONTENT usage a bit so what we don't actually compare to
LOAD_RELOAD_ALLOW_MIXED_CONTENT but nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT
Attachment #797516 - Flags: review?(bugs) → review-
Comment on attachment 797517 [details] [diff] [review]
bug_906190_child_tabs_tests.patch

I'd like to review this after the patch.
(just trying to manage my review queue)
Attachment #797517 - Flags: review?(bugs)
Too late to get into FF24
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Sure, if we want to handle the Ctrl+Click case then we'll need some
> front-end logic to make the connection. But the target="_blank" case should
> work without UI involvement, ideally.

The target=_blank case was already handled in bug 841850 with code written in /docshell.
Talked to Olli and Christoph.  Let's create a new load flag in nsDocShellTypes.h:

LOAD_NORMAL_ALLOW_MIXED_CONTENT = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_ALLOW_MIXED_CONTENT | nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE)
IMHO, if we go down that route and create a new flag, we also have to update the current inconsistency between LOAD and LOAD_RELOAD. I think in
  http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShellLoadInfo.idl#64
we should rename { loadMixedContent = 19; } to {loadReloadMixedContent = 19; } and then be extended by { loadMixedContent = 20; }.

This change in turn requires us to update
  http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1135
accordingly to reflect the difference between loadMixedContent and loadReloadMixedContent.
Attached patch bug_906190_child_tabs.patch (obsolete) — Splinter Review
Attachment #797516 - Attachment is obsolete: true
Attachment #800307 - Flags: review?(tanvi)
Attachment #797517 - Flags: review?(tanvi)
Attachment #800303 - Flags: review?(tanvi) → review+
Comment on attachment 800307 [details] [diff] [review]
bug_906190_child_tabs.patch

>BUG 906190 - Persist 'disable protection' option for Mixed Content Blocker in child tabs
>
>diff --git a/accessible/src/base/Logging.cpp b/accessible/src/base/Logging.cpp
>--- a/accessible/src/base/Logging.cpp
>+++ b/accessible/src/base/Logging.cpp
>@@ -239,16 +239,19 @@ LogShellLoadType(nsIDocShell* aDocShell)
>   printf("load type: ");
> 
>   uint32_t loadType = 0;
>   aDocShell->GetLoadType(&loadType);
>   switch (loadType) {
>     case LOAD_NORMAL:
>       printf("normal; ");
>       break;
>+    case LOAD_NORMAL_ALLOW_MIXED_CONTENT:
>+      printf("normal allow mixed content; ");
>+      break;
>     case LOAD_NORMAL_REPLACE:
>       printf("normal replace; ");
>       break;
>     case LOAD_NORMAL_EXTERNAL:
>       printf("normal external; ");
>       break;
>     case LOAD_HISTORY:
>       printf("history; ");
We should move this case to the bottom of the LOAD_NORMAL_* flags, since it is newly added.

>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -631,16 +631,17 @@ ConvertLoadTypeToNavigationType(uint32_t
>   // Not initialized, assume it's normal load.
>   if (aLoadType == 0) {
>     aLoadType = LOAD_NORMAL;
>   }
> 
>   nsDOMPerformanceNavigationType result = dom::PerformanceNavigation::TYPE_RESERVED;
>   switch (aLoadType) {
>     case LOAD_NORMAL:
>+    case LOAD_NORMAL_ALLOW_MIXED_CONTENT:
>     case LOAD_NORMAL_EXTERNAL:
>     case LOAD_NORMAL_BYPASS_CACHE:
>     case LOAD_NORMAL_BYPASS_PROXY:
>     case LOAD_NORMAL_BYPASS_PROXY_AND_CACHE:
>     case LOAD_NORMAL_REPLACE

Same thing; should add the flag to the end of the LOAD_NORMAL_* flags.

And the same comment for the rest of the patch as well.


>@@ -4132,23 +4139,34 @@ nsDocShell::LoadURI(const PRUnichar * aU
>         popupState = openOverridden;
>     }
>     nsAutoPopupStatePusher statePusher(popupState);
> 
>     // Don't pass certain flags that aren't needed and end up confusing
>     // ConvertLoadTypeToDocShellLoadInfo.  We do need to ensure that they are
>     // passed to LoadURI though, since it uses them.
>     uint32_t extraFlags = (aLoadFlags & EXTRA_LOAD_FLAGS);
>+    uint32_t mixedContentFlag = (aLoadFlags & LOAD_FLAGS_ALLOW_MIXED_CONTENT);
We don't need this extra variable.

>     aLoadFlags &= ~EXTRA_LOAD_FLAGS;
> 
>     nsCOMPtr<nsIDocShellLoadInfo> loadInfo;
>     rv = CreateLoadInfo(getter_AddRefs(loadInfo));
>     if (NS_FAILED(rv)) return rv;
>-    
>-    uint32_t loadType = MAKE_LOAD_TYPE(LOAD_NORMAL, aLoadFlags);
>+  
>+    /*
>+     * If the user "Disables Protection on This Page", we have to make sure to
>+     * remember the users decision when opening links in child tabs [Bug 906190]
>+     */
>+    uint32_t loadType;
>+    if (mixedContentFlag) {
Change to:
if (aLoadFlags & LOAD_FLAGS_ALLOW_MIXED_CONTENT) {

>+      loadType = MAKE_LOAD_TYPE(LOAD_NORMAL_ALLOW_MIXED_CONTENT, aLoadFlags);
>+    } else {
>+      loadType = MAKE_LOAD_TYPE(LOAD_NORMAL, aLoadFlags);
>+    }
>+
>     loadInfo->SetLoadType(ConvertLoadTypeToDocShellLoadInfo(loadType));
>     loadInfo->SetPostDataStream(postStream);

>@@ -9830,16 +9849,17 @@ nsresult nsDocShell::DoChannelLoad(nsICh
>     case LOAD_RELOAD_NORMAL:
>     case LOAD_REFRESH:
>         loadFlags |= nsIRequest::VALIDATE_ALWAYS;
>         break;
> 
>     case LOAD_NORMAL_BYPASS_CACHE:
>     case LOAD_NORMAL_BYPASS_PROXY:
>     case LOAD_NORMAL_BYPASS_PROXY_AND_CACHE:
>+    case LOAD_NORMAL_ALLOW_MIXED_CONTENT:
>     case LOAD_RELOAD_BYPASS_CACHE:
>     case LOAD_RELOAD_BYPASS_PROXY:
>     case LOAD_RELOAD_BYPASS_PROXY_AND_CACHE:
>     case LOAD_RELOAD_ALLOW_MIXED_CONTENT:
>     case LOAD_REPLACE_BYPASS_CACHE:
>         loadFlags |= nsIRequest::LOAD_BYPASS_CACHE |
>                      nsIRequest::LOAD_FRESH_CONNECTION;
>         break;
In the reload case, we want to bypass the cache.  In this load normal case where we are opening a new link in a new tab, do we want to bypass cache?  This is a good question for smaug.  I asked him the same question when I added LOAD_RELOAD_ALLOW_MIXED_CONTENT here (https://bugzilla.mozilla.org/show_bug.cgi?id=822367#c36).  It would be good to understand why we should/should not add it.


>diff --git a/docshell/base/nsDocShellLoadTypes.h b/docshell/base/nsDocShellLoadTypes.h
>--- a/docshell/base/nsDocShellLoadTypes.h
>+++ b/docshell/base/nsDocShellLoadTypes.h
>@@ -37,16 +37,17 @@
> /* load types are legal combinations of load commands and flags 
>  *  
>  * NOTE:
>  *  Remember to update the IsValidLoadType function below if you change this
>  *  enum to ensure bad flag combinations will be rejected.
>  */
> enum LoadType {
>     LOAD_NORMAL = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_NONE),
>+    LOAD_NORMAL_ALLOW_MIXED_CONTENT = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_ALLOW_MIXED_CONTENT | nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE),
Same question as above.  I'm pretty sure we want to include the bypass cache flag here, but I don't know why.

>     LOAD_NORMAL_REPLACE = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_REPLACE_HISTORY),
>     LOAD_NORMAL_EXTERNAL = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_FROM_EXTERNAL),
Attachment #800307 - Flags: review?(tanvi) → review+
Comment on attachment 800307 [details] [diff] [review]
bug_906190_child_tabs.patch

Also update http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.h#739 to:
// Set in DoURILoad when the LOAD_RELOAD_ALLOW_MIXED_CONTENT or LOAD_NORMAL_ALLOW_MIXED_CONTENT flag is set.
Comment on attachment 797517 [details] [diff] [review]
bug_906190_child_tabs_tests.patch

Bug 906190 - Tests: Persist 'disable protection' option for Mixed Content Blocker in child tabs

>diff --git a/browser/base/content/test/browser_bug906190.js b/browser/base/content/test/browser_bug906190.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/browser_bug906190.js
>+function test2D() {
>+  gTestBrowser2.removeEventListener("load", test2D, true);
>+
>+  // The Doorhanger should not appear, because our decision of disabling the
The Doorhanger SHOULD appear

>+  // mixed content blocker should only persistent if pages are from the same domain.
>+  var notification = PopupNotifications.getNotification("mixed-content-blocked", gTestBrowser2);
>+  ok(notification, "OK: Mixed Content Doorhanger did appear again in Test 2D!");


These look good.  We also need tests for redirects - What happens when a child-tab gets the flag but is immediately redirected to another domain?  Per smaug, we should test with both a 302 redirect and a meta-refresh.
Attachment #797517 - Flags: review?(tanvi) → review-
Attached patch bug_906190_child_tabs.patch (obsolete) — Splinter Review
I left two comments for you in the code, look out for @smaug, can you answer those questions in particular?
Attachment #800307 - Attachment is obsolete: true
Attachment #801779 - Flags: review?(bugs)
Attached patch bug_906190_child_tabs.patch (obsolete) — Splinter Review
we are separating the tabbrowser part to simplify the review process for one of our browser peers.
Attachment #801779 - Attachment is obsolete: true
Attachment #801779 - Flags: review?(bugs)
Attachment #801793 - Flags: review?(bugs)
Attachment #801794 - Flags: review?(gavin.sharp)
If the server responds with a 302 redirect, the current patch does not remember the users decision. See follow up bug 914860.
Attachment #797517 - Attachment is obsolete: true
Attachment #802608 - Flags: review?(tanvi)
Attachment #800303 - Flags: review?(bugs)
Comment on attachment 802608 [details] [diff] [review]
bug_906190_child_tabs_tests.patch

Still reviewing, but wanted to clarify tests 3-6.

>Bug 906190 - Tests: Persist 'disable protection' option for Mixed Content Blocker in child tabs

>diff --git a/browser/base/content/test/browser_bug906190.js b/browser/base/content/test/browser_bug906190.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/browser_bug906190.js
>@@ -0,0 +1,480 @@
>+ * 3. [meta-refresh: same origin] Open a page from the same domain in a child tab
>+ *    - Load a html page which has mixed content
>+ *    - Doorhanger to disable protection appears - we disable it
>+ *    - Redirect the page to a page from the same origin using a meta-refresh
Should be:

- Load a new page from the same origin in a new tab simulating a click
- Redirect that page to another page from the same origin using meta-refresh

>+ *    - Doorhanger should >> NOT << appear again!
>+ *
>+ * 4. [meta-refresh: different origin] Open a page from a different domain in a child tab
>+ *    - Load a html page which has mixed content
>+ *    - Doorhanger to disable protection appears - we disable it
>+ *    - Redirect the page to a page from a different origin using a meta-refresh
- Load a new page from the same origin in a new tab simulating a click
- Redirect that page to another page from a different origin using meta-refresh

>+ *    - Doorhanger >> SHOULD << appear again!
>+ *
>+ * 5. [302 redirect: same origin] Open a page from the same domain in a child tab
>+ *    - Load a html page which has mixed content
>+ *    - Doorhanger to disable protection appears - we disable it
>+ *    - Redirect the page to a page from the same origin using 302 redirect
- Load a new page from the same origin in a new tab simulating a click
- Redirect that page to another page from the same origin using 302 redirect

>+ *    - Doorhanger >> SHOULD << appear again!
>+ *
>+ * 6. [302 redirect: different origin] Open a page from the same domain in a child tab
>+ *    - Load a html page which has mixed content
>+ *    - Doorhanger to disable protection appears - we disable it
>+ *    - Redirect the page to a page from a different origin using 302 redirect
- Load a new page from the same origin in a new tab simulating a click
- Redirect that page to another page from a different origin using 302 redirect
>+ *    - Doorhanger >> SHOULD << appear again!
>+ *
Blocks: 914860
Comment on attachment 802608 [details] [diff] [review]
bug_906190_child_tabs_tests.patch

Looks good!  Some changes below.  r- for now but can r+ with these changes.

>Bug 906190 - Tests: Persist 'disable protection' option for Mixed Content Blocker in child tabs

>diff --git a/browser/base/content/test/browser_bug906190.js b/browser/base/content/test/browser_bug906190.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/browser_bug906190.js
>+ *
>+ * 5. [302 redirect: same origin] Open a page from the same domain in a child tab
>+ *    - Load a html page which has mixed content
>+ *    - Doorhanger to disable protection appears - we disable it
>+ *    - Redirect the page to a page from the same origin using 302 redirect
>+ *    - Doorhanger >> SHOULD << appear again!
>+ *
Technically, the doorhanger should not appear.  It does appear and we are okay with this behavior.  Hence, maybe we should make this a todo() test.  We can change the todo() to an ok() test after and if bug 914860 is fixed.
todo tests: https://developer.mozilla.org/en-US/docs/Mochitest#Test_functions

>+
>+function test4C() {
>+  gTestBrowser.removeEventListener("load", test4B, true);
>+  var actual = content.document.getElementById('mctestdiv').innerHTML;
>+  is(actual, "Mixed Content Blocker disabled", "OK: Executed mixed script in Test 4C");
>+
>+  // file_bug906190_4.html redirects to page test1.example.com/* using meta-refresh
>+  var tabURL = gHttpTestRoot2 + "file_bug906190_4.html";
Use file_bug906190_3 instead.

>+function test5D() {
>+  gTestBrowser2.removeEventListener("load", test5D, true);
>+
>+  // The Doorhanger should >> APPEAR <<!
should NOT appear
>+  var notification = PopupNotifications.getNotification("mixed-content-blocked", gTestBrowser2);
>+  ok(notification, "OK: Mixed Content Doorhanger did appear again in Test 5D!");
>+
// todo test until bug 914860 is fixed
todo(!notification, "OK: Mixed Content Doorhanger did not appear again in Test 5D!")

>+  var actual = content.document.getElementById('mctestdiv').innerHTML;
>+  is(actual, "Mixed Content Blocker enabled", "OK: Blocked mixed script in Test 5D");
>+
// todo test until bug 914860 is fixed
todo_is(actual, "Mixed Content Blocker disabled", "OK: Executed mixed script in Test 5D");

>+  gBrowser.removeCurrentTab();
>+  test6();
>+}
>+


>diff --git a/browser/base/content/test/file_bug906190_3.html b/browser/base/content/test/file_bug906190_3.html
>new file mode 100644
>diff --git a/browser/base/content/test/file_bug906190_4.html b/browser/base/content/test/file_bug906190_4.html
>new file mode 100644

These 2 files look exactly the same.  You can just use 1 (file_bug906190_3.html) for both tests 3 and 4, and then delete file_bug906190_4.
Attachment #802608 - Flags: review?(tanvi) → review-
Incorporated Tanvis suggestions - making the 302 redirect testcase (test 5) a todo() which we can update once bug 914860 lands!
Attachment #802608 - Attachment is obsolete: true
Attachment #804119 - Flags: review?(bugs)
Attachment #804119 - Flags: review?(tanvi)
Comment on attachment 804119 [details] [diff] [review]
bug_906190_child_tabs_tests.patch

Why did you remove this part from test5D():

>+  var actual = content.document.getElementById('mctestdiv').innerHTML;
>+  is(actual, "Mixed Content Blocker enabled", "OK: Blocked mixed script in Test 5D");
>+
// todo test until bug 914860 is fixed
todo_is(actual, "Mixed Content Blocker disabled", "OK: Executed mixed script in Test 5D");

Can we do a todo_is test for when the doorhanger is not there and the mixed script runs?
Attachment #804119 - Flags: review?(tanvi) → review+
Comment on attachment 800303 [details] [diff] [review]
bug_906190_child_tabs_update_load_reload_inconsistency.patch

(sorry about the delay. I've had quite some other patches to review.)
Attachment #800303 - Flags: review?(bugs) → review+
Comment on attachment 801793 [details] [diff] [review]
bug_906190_child_tabs.patch


> 
>     case LOAD_NORMAL_BYPASS_CACHE:
>     case LOAD_NORMAL_BYPASS_PROXY:
>     case LOAD_NORMAL_BYPASS_PROXY_AND_CACHE:
>+    case LOAD_NORMAL_ALLOW_MIXED_CONTENT: //@smaug: do we need this?
Hmm, for consistency, yes.

>     LOAD_NORMAL_BYPASS_PROXY_AND_CACHE = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE | nsIWebNavigation::LOAD_FLAGS_BYPASS_PROXY),
>+    LOAD_NORMAL_ALLOW_MIXED_CONTENT = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_ALLOW_MIXED_CONTENT | nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE),
>+    //@smaug: do we need nsIWebNavigation::LOAD_FLAGS_BYPASS_CACHE
I think for consistency with the other ALLOW_MIXED_ yes.
Attachment #801793 - Flags: review?(bugs) → review+
Comment on attachment 804119 [details] [diff] [review]
bug_906190_child_tabs_tests.patch

I think tanvi's review is enough for this.
Make sure these tests are run for few times on tryserver before pushing to m-i
Attachment #804119 - Flags: review?(bugs)
With Firefox 24 ESR (on all 3 platforms), following the next steps (from test #8086 in Moztrap):

1.Launch Firefox.
Load a page with mixed active content (e.g. https://people.mozilla.com/~bsterne/tests/62178/test.html)
> The shield and lock icons should be displayed in the location bar. The script shouldn't load.
 
2.Click the shield icon.
 
3.Click the arrow next to "Keep blocking"
 
4.Select "Disable protection on this page"
>The yellow triangle icon should be displayed in the location bar.The Insecure script should be loaded.
 
5.Reload the page.
>The shield and lock icons should be displayed again and the script shouldn't be loaded.

I receive the following results, after step 5:

i) after reloading the page (step 5) the yellow triangle icon is still displaying (and not the shield and lock icons, as mentioned in the test case)

ii) also after reloading, I can still see the yellow triangle, and in the web console, I don't see anymore the message saying the page is blocked by mixed content

Is this expected? Thanks!
Flags: needinfo?(mozilla)
(In reply to Manuela Muntean [:Manuela] [QA] from comment #34)
> i) after reloading the page (step 5) the yellow triangle icon is still
> displaying (and not the shield and lock icons, as mentioned in the test case)

that is correct!
 
> ii) also after reloading, I can still see the yellow triangle, and in the
> web console

correct - in the web console you should see:

Blocked loading mixed active content "http://people.mozilla.com/~bsterne/tests/62178/static/script/script3.js" @ https://people.mozilla.org/~bsterne/tests/62178/static/content/iframe.html:27

> I don't see anymore the message saying the page is blocked by mixed content
> Is this expected? Thanks!

But you should still see the warning:

Loading mixed (insecure) active content on a secure page "http://people.mozilla.com/~bsterne/tests/62178/static/script/script3.js" @ https://people.mozilla.org/~bsterne/tests/62178/static/content/iframe.html:27


I am not sure if your question is child-tab specific, therefore this bug probably is not the only one where it applies. It seems like this is a more general question regarding the behavior of MCB. Requesting more info from Tanvi to make sure.
Flags: needinfo?(mozilla) → needinfo?(tanvi)
With bug 902156, we changed the behavior of MCB in FF24+.  Now the decision to disable protection will persist while you are navigating a specific page on a specific tab.  Once you move to another tab or another domain, the persistence will stop.  See her for more details and testing info: https://bugzilla.mozilla.org/show_bug.cgi?id=902156#c68
Flags: needinfo?(tanvi)
Comment on attachment 801794 [details] [diff] [review]
bug_906190_child_tabs_tabbrowser_part.patch

Sorry for the response delay, I was traveling Sep 3-13.

What use cases is this meant to cover, specifically? I.e. which specific "open in new tab" cases do we want to extend this "persist disable protection" option to, and which of those cases requires this patch?

There are many front-end ways to trigger new tabs, and it's not clear that this patch is necessary and/or sufficient.

For example, I think extending the "disable protection" behavior for Ctrl+clicked links but not for Ctrl+T+"type URL" seems somewhat arbitrary, and I'm not sure that the Ctrl+click case is worth worrying about in particular.

Is the rationale for making this tab-based rather than origin-based summarized anywhere?
Hi Gavin,

In bug 902156, we added some persistence to the Mixed Content Blocker.  When you are navigating through a specific domain on a specific tab and you disable protection, that decision will persist while you are on that tab and on that domain.  As soon as you leave the domain or close the tab, the persistence is gone.

When discussing this with release management, they proposed we also persist across child tabs.  For example, assume you are on the nytimes.com and you are reading articles.  You may control-click on various articles for them to be opened in new tabs so that you can read them one by one.  With this patch, if the user disables protection on the first page, the subsequent tabs will also have disabled protection.  The user will not have to disable protection on each of them individually.

On another scenario is if you are on a shopping site with mixed content.  You are purchasing multiple items and you may control-click them so that you can read about the details and decide if you want to add them to your cart.  The disable protection decision will persist across these child tabs as well.

If a user opens a new tab and starts typing a new url, we do not try to persist the decision.  We are not storing all the domains that a user has "disabled protection" on.  Moreover, we do not want to persist this decision for the entire session because there is currently no way for a user to re-enable protection.  There is discussion about persisting the decision for the entire session in bug 860712.  That bug is not something we can fix in the short term because it will require UX work first.

Is there any concern with the code added in tabbrowser.xml?  Is there a performance issue or something else that is causing you to question adding code to that file?
Flags: needinfo?(gavin.sharp)
I understand why you see this is beneficial as the next logical step down the "enable only for one tab" road, but what I don't understand is why we think it's a good idea to go down that road.

(In reply to Tanvi Vyas [:tanvi] from comment #38)
> If a user opens a new tab and starts typing a new url, we do not try to
> persist the decision.  We are not storing all the domains that a user has
> "disabled protection" on.

Why not?

> Moreover, we do not want to persist this decision
> for the entire session because there is currently no way for a user to
> re-enable protection.

Assuming we do want to avoid persisting the whole session, why not address that by just persisting for X seconds? Or X page loads, or X seconds since the last page load, or some such. Plug-in CTP uses a time-based mechanism, as I understand it, and that's a very similar scenario.
Flags: needinfo?(gavin.sharp)
Copying an excerpt from email sent to Tanvi/Christoph:

My specific concern with the patch in bug 906190 is that you're adding the code to tabbrowser's addTab, which runs any time a new tab is added to the browser. It sounds like you intend to only have this code run when a tab is opened with Ctrl+Click, or in other cases when the new tab's load is "related" to the current tab (is that true?). Assuming that's the case, the code should live closer to those specific cases, where the intent is clearer. For example, in browser.js's handleLinkClick. That would require adding a way to propagate the load flag all the way to addTab, but maybe we can re-purpose openLinkIn's existing "relatedToCurrent" flag (we don't currently send it for Ctrl+Click, for some reason...). We should discuss this further in the bug. A clear statement of the exact use cases we intend to address (i.e. a complete definition of "child tabs") would also be useful.
Not putting this in release notes since we have not had a significant amount of pushback from users so I think we can release this enhancement without noting.
Comment on attachment 801794 [details] [diff] [review]
bug_906190_child_tabs_tabbrowser_part.patch

I'll r+ a patch that addresses comment 40, let me know if anything isn't clear.
Attachment #801794 - Flags: review?(gavin.sharp) → review-
Tracking flags came from a clone, no need to track this bug for upcoming releases from what I can tell.
added reviewer to comment and final modifications.
carrying over r+ from tanvi and bugs.
Attachment #800303 - Attachment is obsolete: true
Attachment #815968 - Flags: review+
added reviewer to comment and final modifications.
carrying over r+ from tanvi.
Attachment #804119 - Attachment is obsolete: true
Attachment #815969 - Flags: review+
added reviewer to comment and final modifications.
carrying over r+ from bugs.
Attachment #801793 - Attachment is obsolete: true
Attachment #815970 - Flags: review+
Hi Gavin,
I took the logic out of the tabbrowser.xml, so |addTab| only has to check for the specific flag and does not have to perform the full check for every tab that is opened. I hope this is what you expected!
Thanks, Christoph
Attachment #801794 - Attachment is obsolete: true
Attachment #815976 - Flags: review?(gavin.sharp)
Comment on attachment 815976 [details] [diff] [review]
bug_906190_child_tabs_tabbrowser_part.patch

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+            var aPersistDisableMCB;

naming nit: "persistDisableMCB" seems like a confusing name, since it's only "persistence" if you associate this new tab with the old one. I would just call this "aDisableMCB" (and rename things down the line accordingly).

>diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js

>   case "tab":
>     let browser = w.gBrowser;
>+
>+    // if the mixedContentChannel is present and the referring URI passes
>+    // a same origin check with the target URI, we can preserve the users
>+    // decision of disabling MCB on a page for it's child tabs.
>+    var persistDisableMCBInChildTab = false;
>+
>+    if (browser.docShell.mixedContentChannel) {

openLinkIn is a general utility function, I don't think it's safe to assume that the current browser's mixedContentState should be persisted here (the link opening might have nothing to do with the current tab, e.g. if you drop something on the new tab button, or click a link in a pref window, or try to open the addons manager while viewing a website). This should move to the contentAreaClick code and be passed through the options to this function, I think.

>+      try {
>+        var targetURI = NetUtil.newURI(url);

fwiw the "makeURI" helper is already defined in this scope (and browser.js')

r- for the issue described above, but this looks good otherwise
Attachment #815976 - Flags: review?(gavin.sharp) → review-
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #48)
> naming nit: "persistDisableMCB" seems like a confusing name, since it's only
> "persistence" if you associate this new tab with the old one. I would just
> call this "aDisableMCB" (and rename things down the line accordingly).

Done!

> >diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js
> 
> >   case "tab":
> >     let browser = w.gBrowser;
> >+
> >+    // if the mixedContentChannel is present and the referring URI passes
> >+    // a same origin check with the target URI, we can preserve the users
> >+    // decision of disabling MCB on a page for it's child tabs.
> >+    var persistDisableMCBInChildTab = false;
> >+
> >+    if (browser.docShell.mixedContentChannel) {
> 
> openLinkIn is a general utility function, I don't think it's safe to assume
> that the current browser's mixedContentState should be persisted here (the
> link opening might have nothing to do with the current tab, e.g. if you drop
> something on the new tab button, or click a link in a pref window, or try to
> open the addons manager while viewing a website). This should move to the
> contentAreaClick code and be passed through the options to this function, I
> think.

What if implement the functionality in |openLinkInTab| and propagate the flag all the way? This seems to be an elegant solution to me, what do you think?

> >+      try {
> >+        var targetURI = NetUtil.newURI(url);
> 
> fwiw the "makeURI" helper is already defined in this scope (and browser.js')

Nice catch - using makeURI of course!
Attachment #815976 - Attachment is obsolete: true
Attachment #823598 - Flags: review?(gavin.sharp)
Hi Gavin, after discussing the problem with Tanvi again, we have two use cases where we want to persist the users decision about disabling MCB across child tabs. Please compare the following stack traces:

a) |RIGHT-CLICK OPEN IN TAB|

0 addTab() ["chrome://browser/content/tabbrowser.xml":1432]
1 loadOneTab() ["chrome://browser/content/tabbrowser.xml":1268]
2 openLinkIn() ["chrome://browser/content/utilityOverlay.js":321]
3 anonymous() ["chrome://browser/content/nsContextMenu.js":864]
4 oncommand() ["chrome://browser/content/browser.xul":1]

b) |CTRL CLICK|

0 addTab() ["chrome://browser/content/tabbrowser.xml":1432]
1 loadOneTab() ["chrome://browser/content/tabbrowser.xml":1268]
2 openLinkIn() ["chrome://browser/content/utilityOverlay.js":321]
3 handleLinkClick() ["chrome://browser/content/browser.js":13664]
4 contentAreaClick() ["chrome://browser/content/browser.js":13628]

In the attached patch, we therefore add logic for the persistence to a) openLinkInTab and b) handleLinkClick.

Thanks,
  Christoph
Attachment #823598 - Attachment is obsolete: true
Attachment #823598 - Flags: review?(gavin.sharp)
Attachment #823674 - Flags: review?(gavin.sharp)
If a user's disables MCB on a page and |CTRL-DRAG|s a link from within that page into a new child-tab (new tab) this patch persists the users decision and disables MCB for the newly created (child) tab.
See, the following stack trace:

  0 addTab() ["chrome://browser/content/tabbrowser.xml":1436]
  1 loadOneTab() ["chrome://browser/content/tabbrowser.xml":1268]
  2 onxbldrop() ["chrome://browser/content/tabbrowser.xml":4431]

This patch does not preserve a user's decision of disabling MCB for |CTRL-DRAG| (duplicate) a tab (see Bug 925680), which follows a different path in the code, see the following stack trace:

  0 addTab() ["chrome://browser/content/tabbrowser.xml":1436]
  1 ssi_duplicateTab() ["resource:///modules/sessionstore/SessionStore.jsm":1520]
  2 ss_duplicateTab() ["resource:///modules/sessionstore/SessionStore.jsm":191]
  3 duplicateTab() ["chrome://browser/content/tabbrowser.xml":2618]
  4 onxbldrop() ["chrome://browser/content/tabbrowser.xml":4359]

Tanvi, should we include this patch in this bug? I think it would make sense in terms of consistency. What do you think?
Attachment #824152 - Flags: feedback?(tanvi)
Blocks: 925680
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #51)
> Tanvi, should we include this patch in this bug? I think it would make sense
> in terms of consistency. What do you think?

I am hesitant to expand the scope of this bug.  It was not one of the use cases that release management and Dan Veditz brought up specifically when they asked us to create this bug.  You could ask them what they think if you'd like, but I am inclined to leave ctrl+drag in Bug 925680.
(In reply to Tanvi Vyas [:tanvi] from comment #52)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #51)
> > Tanvi, should we include this patch in this bug? I think it would make sense
> > in terms of consistency. What do you think?
> 
> I am hesitant to expand the scope of this bug.  It was not one of the use
> cases that release management and Dan Veditz brought up specifically when
> they asked us to create this bug.  You could ask them what they think if
> you'd like, but I am inclined to leave ctrl+drag in Bug 925680.

I am with you Tanvi, lets leave this patch for 925680. Thanks.
Comment on attachment 824152 [details] [diff] [review]
bug_906190_child_tabs_ctrl_drag_part.patch

Since we decided that |CTRL-DRAG| of links is not part of this bug, I am moving this patch over to Bug 925680 and marking it as obsolete here.
Attachment #824152 - Attachment is obsolete: true
Attachment #824152 - Flags: feedback?(tanvi)
In this patch I am calling |openLinkIn| which tests only half of the newly introduced feature. It would be better if we can call |handleLinkClick| and also |openLinkInTab| (compare Comment 50).

Gavin, it seems that you know how to do this efficiently. Can you provide some hints? Also, in test 2, we currently load a new page from a different origin in a new tab simulating a click, which seems kind of hard to simulate through |handleLinkClick| and |openLinkInTab|.
Attachment #815969 - Attachment is obsolete: true
Attachment #824792 - Flags: feedback?(gavin.sharp)
Comment on attachment 823674 [details] [diff] [review]
bug_906190_child_tabs_tabbrowser_part.patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>@@ -5095,19 +5095,37 @@ function handleLinkClick(event, href, li

>+  if (where == "tab" && gBrowser.docShell.mixedContentChannel) {
>+    const sm = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
>+                                 .getService(Components.interfaces.nsIScriptSecurityManager);

Just use Services.scriptSecurityManager

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>   openLinkInTab: function() {

>+    if (this.browser.docShell.mixedContentChannel) {
>+      const sm = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
>+                                    .getService(Components.interfaces.nsIScriptSecurityManager);

Use Services.jsm here too.

>+        var targetURI = makeURI(this.linkURL);

you can just use this.linkURI directly.
Attachment #823674 - Flags: review?(gavin.sharp) → review+
Comment on attachment 824792 [details] [diff] [review]
bug_906190_child_tabs_tests.patch

Drew, can you give Christoph some feedback on this test?
Attachment #824792 - Flags: feedback?(gavin.sharp) → feedback?(adw)
Comment on attachment 824792 [details] [diff] [review]
bug_906190_child_tabs_tests.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #55)
> In this patch I am calling |openLinkIn| which tests only half of the newly
> introduced feature. It would be better if we can call |handleLinkClick| and
> also |openLinkInTab| (compare Comment 50).

Can you not just call handleLinkClick directly?

For openLinkInTab, there are a couple of approaches.  You could create an nsContextMenu instance, set document.popupNode, init the nsContextMenu, and then call openLinkInTab on it.  Some tests do this, like this one: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug734076.js#105

Or you could synthesize a right-click like the primary context menu tests do: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/contextmenu_common.js#3

> some hints? Also, in test 2, we currently load a new page from a different
> origin in a new tab simulating a click, which seems kind of hard to simulate
> through |handleLinkClick| and |openLinkInTab|.

For handleLinkClick, can't you pass whatever href and linkNode you want?  For openLinkInTab, either of the two aforementioned methods should work.
Incorporated Gavin's suggestion from Comment 56 and carrying over the r+.
Attachment #823674 - Attachment is obsolete: true
Attachment #826095 - Flags: review+
Following Gavin's advice we changed where the code for this bug should live, therefore I updated the testcases to reflect that change.

(In reply to Drew Willcoxon :adw from comment #58)
> Comment on attachment 824792 [details] [diff] [review]
> bug_906190_child_tabs_tests.patch
> 
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #55)
> > In this patch I am calling |openLinkIn| which tests only half of the newly
> > introduced feature. It would be better if we can call |handleLinkClick| and
> > also |openLinkInTab| (compare Comment 50).
> 
> Can you not just call handleLinkClick directly?

Calling ContentAreaClick instead.

> 
> For openLinkInTab, there are a couple of approaches.  You could create an
> nsContextMenu instance, set document.popupNode, init the nsContextMenu, and
> then call openLinkInTab on it.  Some tests do this, like this one:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> general/browser_bug734076.js#105
> 
> Or you could synthesize a right-click like the primary context menu tests

That's what I ended up doing. Thanks for the hint. Christoph
Attachment #824792 - Attachment is obsolete: true
Attachment #824792 - Flags: feedback?(adw)
Attachment #826101 - Flags: review?(tanvi)
Comment on attachment 826101 [details] [diff] [review]
bug_906190_child_tabs_tests.patch

Feedback +, with some questions and comments below.  Please update and then r? to smaug.

>Bug 906190 - Persist 'disable protection' option for Mixed Content Blocker in child tabs - tests (r=tanvi)
>diff --git a/browser/base/content/test/general/browser_bug906190.js b/browser/base/content/test/general/browser_bug906190.js
>+let clickHandler = function (aEvent, aFunc) {
Add a comment mentioning that the purpose of this function is to simulate CTRL+CLICK

>+let contextMenuOpenHandler = function(aEvent, aFunc) {
Add a comment mentioning that the purpose of this function is to simulate RIGHT CLICK - OPEN LINK IN NEW TAB

>+  gTestWin.document.removeEventListener("popupshown", curContextMenu, false);
>+  gTestWin.gBrowser.addEventListener("load", aFunc, true);
>+
>+  // Select "Open Link In Tab" option from context menu which
>+  // dispatches to openLinkInTab
>+  var saveVideoCommand = gTestWin.document.getElementById("context-openlinkintab");
>+  saveVideoCommand.doCommand();
You should rename this variable.

>+  aEvent.target.hidePopup();
What is this for?  Does the popup not automatically go away when you click or doCommand() on context-openlinkintab?


>+function reloadTabAfterDisablingMCB() {
This function seems poorly named.  I believe this is the tab after mixed content blocking has been disabled via the doorhanger.  Disabling prompts a reload and hence the name "reloadTab".  But that makes it sound as if this function itself is going to do a reload, which it isn't.  The best alternative I can think of is "reloadedTabAfterDisablingMCB", but admittedly, even that is not great.

>+  gTestWin.gBrowser.removeEventListener("load", reloadTabAfterDisablingMCB, true);
>+
>+  var expected = "Mixed Content Blocker disabled";
>+  waitForCondition(
>+    function() gTestWin.content.document.getElementById('mctestdiv').innerHTML == expected,
>+    makeSureMCBisDisabled, "Error: Waited too long for mixed script to run in " + curTestName + "!");
>+}
>+

>+function makeSureMCBisDisabled() {
>+  var actual = gTestWin.content.document.getElementById('mctestdiv').innerHTML;
>+  is(actual, "Mixed Content Blocker disabled", "OK: Executed mixed script in Test 1C");
Why does this say Test1C?  What should it say?


>+function test1A() {
>+  gTestWin.gBrowser.removeEventListener("load", test1A, true);
>+  gTestWin.gBrowser.selectTabAtIndex(2);
>+
>+  // The Doorhanger should >> NOT << appear, because our decision of disabling the
>+  // mixed content blocker is persistent across tabs.
>+  var notification = PopupNotifications.getNotification("mixed-content-blocked", gTestWin.gBrowser.selectedBrowser);
>+  ok(!notification, "OK: Mixed Content Doorhanger did not appear again in Test 1A!");
>+
>+  var actual = gTestWin.content.document.getElementById('mctestdiv').innerHTML;
>+  is(actual, "Mixed Content Blocker disabled", "OK: Executed mixed script in Test 1A");
>+
>+  gTestWin.gBrowser.removeCurrentTab();
>+  test1C();
I believe this should be a call to test1B().


>+}
>+
>+function test1B() {
>+  curContextMenu = function (e) { contextMenuOpenHandler(e, test1C) };
>+  gTestWin.document.addEventListener("popupshown", curContextMenu, false);
>+
>+  // simulating |RIGHT-CLICK -> OPEN LINK IN TAB|
>+  let targetElt = gTestWin.content.document.getElementById("Test1");
>+  EventUtils.synthesizeMouseAtCenter(targetElt, { type : "contextmenu", button : 2 } , gTestWin.content);
Add a comment that button 2 is a right click, and hence you get the context menu popup.


>+function startTests() {
>+  mainTab = gTestWin.gBrowser.selectedTab;
>+  var link = gHttpTestRoot1 + "file_bug906190_2.html";
>+  setUpTest("Test1", "linkForTest1", test1C, link);
This is supposed to start the first test, right?
Why is this test1C and not test1 or test1A?

Why is this file_bug906190_2 and not file_bug906190_1?

>diff --git a/browser/base/content/test/general/file_bug906190.sjs b/browser/base/content/test/general/file_bug906190.sjs
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/general/file_bug906190.sjs
>@@ -0,0 +1,14 @@
>+function handleRequest(request, response) {
>+  var page = "<!DOCTYPE html><html><body>bug 906190</body></html>";
>+  var path = "https://test1.example.com/browser/browser/base/content/test/general/";
>+  var url = path + "file_bug906190_redirected.html";
>+
>+  dump("\n\n");
>+  dump("SJS -> " + url + "\n");
Remove dumps
Attachment #826101 - Flags: review?(tanvi) → feedback+
Tanvi, I incorporated and addressed all your concerns in the patch and answered the additional questions below.

(In reply to Tanvi Vyas [:tanvi - on leave] from comment #61)
> >+  aEvent.target.hidePopup();
> What is this for?  Does the popup not automatically go away when you click
> or doCommand() on context-openlinkintab?

No, unfortunately the contextmenu does not go away automatically - therefore we have to hide it explicitly.
 
> >+function makeSureMCBisDisabled() {
> >+  var actual = gTestWin.content.document.getElementById('mctestdiv').innerHTML;
> >+  is(actual, "Mixed Content Blocker disabled", "OK: Executed mixed script in Test 1C");
> Why does this say Test1C?  What should it say?

I should really stop doing copy/paste :-). Good catch Tanvi, it shouldn't say Test 1C, I fixed this in the patch.
 
> >+  gTestWin.gBrowser.removeCurrentTab();
> >+  test1C();
> I believe this should be a call to test1B().

Yes, it should be test1B() - updated!

> >+function startTests() {
> >+  mainTab = gTestWin.gBrowser.selectedTab;
> >+  var link = gHttpTestRoot1 + "file_bug906190_2.html";
> >+  setUpTest("Test1", "linkForTest1", test1C, link);
> This is supposed to start the first test, right?
> Why is this test1C and not test1 or test1A?

It should be test1C - updated!

> >+function startTests() {
> >+  mainTab = gTestWin.gBrowser.selectedTab;
> >+  var link = gHttpTestRoot1 + "file_bug906190_2.html";
> Why is this file_bug906190_2 and not file_bug906190_1?

It is the link to the child tab. I renamed it to |childTabLink|, which should make its usage clearer.

Thanks for the feedback.
Attachment #826101 - Attachment is obsolete: true
Attachment #828386 - Flags: review?(bugs)
Comment on attachment 828386 [details] [diff] [review]
bug_906190_child_tabs_tests.patch

Mostly rs+, given that tanvi looked at this too.
Make sure to run this on tryserver few times (both opt and debug builds and on different OSes)
Attachment #828386 - Flags: review?(bugs) → review+
Thanks smaug - pushed to try:
  https://tbpl.mozilla.org/?tree=Try

This is ready to be checked in!
Keywords: checkin-needed
The attachments give no indication of order and you didn't link to your actual Try run. Please give me some sort of clue as to what order these are supposed to be applied in when requesting checkin.
Keywords: checkin-needed
Facepalm - here is the actual link:
  https://tbpl.mozilla.org/?tree=Try&rev=17c7698128a2

Also, please apply them in this order:
  1 - bug_906190_child_tabs_update_load_reload_inconsistency.patch
  2 - bug_906190_child_tabs.patch
  3 - bug_906190_child_tabs_tabbrowser_part.patch
  4 - bug_906190_child_tabs_tests.patch
Keywords: checkin-needed
Depends on: 1093642
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: