803225 - WebSockets and about:blank content triggers mixed-active-content blocking

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

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

For the past few weeks I have been unable to "Reconnect" on the IRCCloud website. I bisected on Nightlies today and narrowed it down to this pushlog, http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3f0587ce1774&tochange=0d3b17a88d5f

I suspect that bug 761227 is to blame here. It seems to be a combination with something in my profile and this patch.

I can help test a patch for this bug. I'd also be happy to hook up a debugger here if someone can tell me what to look for.

Comment 1

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

Actually, it turns out that I had the security.mixed_content.block_active_content pref set in about:config.

Maybe this bug is INVALID then, since the UI for showing that the content has been blocked hasn't landed yet.

I'll let Tanvi or Brandon decide here.

Comment 2

[Tanvi Vyas[:tanvi]]

This is because the Mixed Blocker Content patch in FF 18 (the one in bug 62178) decides if a resource is secure by checking if its an https resource.  In fact, other types of resources are secure as well, and should also be allowed (ex: wss).  I've made the following change locally to nsMixedContentBlocker.cpp and it was going to be part of bug 782654:

-  // Check the parent scheme. If it is not an HTTPS page then mixed content
-  // restrictions do not apply.
-  bool parentIsHttps;
-  if (NS_FAILED(aRequestingLocation->SchemeIs("https", &parentIsHttps)) ||
-      !parentIsHttps) {
+  // Get the scheme of the sub-document resource to be requested. If it is
+  // an HTTPS load then mixed content doesn't apply.
+  bool isSchemeSafe;
+  //Note - Open to suggestions on the proper way of formatting this
+  if ( NS_FAILED(aContentLocation->SchemeIs("https", &isSchemeSafe)) || isSchemeSafe ||
+       NS_FAILED(aContentLocation->SchemeIs("about", &isSchemeSafe)) || isSchemeSafe ||
+       NS_FAILED(aContentLocation->SchemeIs("data:", &isSchemeSafe)) || isSchemeSafe ||
+       NS_FAILED(aContentLocation->SchemeIs("javascript:", &isSchemeSafe)) || isSchemeSafe ||
+       NS_FAILED(aContentLocation->SchemeIs("wss:", &isSchemeSafe)) || isSchemeSafe ) {

I think we should separate this out from that patches in bug 782654 and uplift it to FF 18, since without it pages with websockets are unusable with the security.mixed_content.block_active_content pref set to true.

Comment 3

[Tanvi Vyas[:tanvi]]

Here is the attachment that contains the code mentioned in comment 2:
https://bug782654.bugzilla.mozilla.org/attachment.cgi?id=671665

There was some feedback that we shouldn't use the actual strings "https", "about".  I'm open to suggestions on a better way to check these.  Create a separate function that goes through an array of acceptable protocols?

Comment 4

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

(In reply to Tanvi Vyas from comment #2)
> +  if ( NS_FAILED(aContentLocation->SchemeIs("https", &isSchemeSafe)) ||
> +       NS_FAILED(aContentLocation->SchemeIs("about", &isSchemeSafe)) ||
> +       NS_FAILED(aContentLocation->SchemeIs("data:", &isSchemeSafe)) ||
> +       NS_FAILED(aContentLocation->SchemeIs("javascript:", &isSchemeSafe))
> +       NS_FAILED(aContentLocation->SchemeIs("wss:", &isSchemeSafe)) ||

Note that the schemes should not include the trailing colon.

See http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#904 for an example of how this can be done using an array of these strings.

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: Add additional secure schemes to nsMixedContentBlocker.cpp

Jared, I believe this will fix the websockets issue you are having on irccloud.  Please confirm.  If this gets through review, I think we should uplift to 18.  Without this, all websites with websockets will not work currently when a user disables mixed active content.

Comment 6

[Tanvi Vyas[:tanvi]]

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

Comment 7

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

Comment on attachment 673481 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

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

::: content/base/src/nsMixedContentBlocker.cpp
@@ +134,5 @@
> +     "https",
> +     "about",
> +     "data",
> +     "javascript",
> +     "wss",

Can we add "blob" here too, and alphabetize this list?

Comment 8

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

I can confirm that the try build fixes the irccloud issue I was seeing before.

Comment 9

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 673481 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

What about nested urls, like jar:

This needs tests.

Comment 10

[Tanvi Vyas[:tanvi]]

I am updating this to the following:
  static const char *const schemeWhiteList[] = {
     "https",
     "about",
     "blob",
     "chrome",
     "data",
     "jar",
     "javascript",
     "mailto",
     "resource",
     "wss",
     nullptr
  };

Note that https is at the top.  This is the most frequent case so I'd rather than run through https before about, blob, chrome, and data for performance reasons.  Yes that means it is not alphabetized, but does it really have to be?  I will post a patch shortly (testing locally first).

My next step will be to create tests for these cases, before updating the patch anymore.

I know it is not ideal to have a whitelist and it would be ideal to instead use URI flags.  I haven't found existing flags that would cover all the cases though.  Right now I have pseudo-code for some cases that looks something like this:

if(   !NS_URIChainHasFlags(aContentLocation,
       nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &schemeIsSafe)  
   || !NS_URIChainHasFlags(aContentLocation,
       nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT, &schemeIsSafe) )
   return NS_ERROR_FAILURE;

if(isSchemeSafe)
  return NS_OK;

Comment 11

[Tanvi Vyas[:tanvi]]

Attachment: Add additional secure schemes to nsMixedContentBlocker.cpp

Try: https://tbpl.mozilla.org/?tree=Try&rev=55796e46b542

Comment 12

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Well, jar can be safe or unsafe, I think.
(Though, I don't know whether http and https is supported with it.)

Comment 13

[Tanvi Vyas[:tanvi]]

Attachment: Add additional secure schemes to nsMixedContentBlocker.cpp

This version of the code has a bunch of comments and isn't ready to land.  Right now it includes whitelist of protocols allowed on https pages.  I've included commented code that would use protocol flags instead of a whitelist, that I will try out next.

Comment 14

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests for data, about, mailto, and javascript protocols

I've added four mochitests for four different protocols.  Two fall under the URI_IS_LOCAL_RESOURCE protocol flag.  One falls under URI_DOES_NOT_RETURN_DATA.  And the last one (about) doesn't fall into either of these yet, but should probably be in URI_IS_LOCAL_RESOURCE.

What additional tests should I write?  There are numerous protocols, and I'm not sure if we need a test for each one.

Thanks!

Comment 15

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 675773 [details] [diff] [review]
Mochitests for data, about, mailto, and javascript protocols

I believe new patch is coming.

Comment 16

[Tanvi Vyas[:tanvi]]

Attachment: Add additional secure schemes to nsMixedContentBlocker.cpp

Ready for review.  This is now using protocol flags instead of a whitelist of schemes.  This way, if additional schemes are added with one of the following flags, it will not be blocked by the mixed content blocker:
URI_IS_LOCAL_RESOURCE
URI_DOES_NOT_RETURN_DATA
URI_INHERITS_SECURITY_CONTEXT

I'm still working on finishing the tests.  Thanks!

Comment 17

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 677704 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

>+  /* Check Protocl Flags:
>+  * URI_INHERITS_SECURITY_CONTEXT - i.e:
>+  *  "javascript",
>+  * URI_IS_LOCAL_RESOURCE - i.e:
>+  *   "blob",
>+  *   "chrome",
>+  *   "data",
>+  *   "resource",
>+  *   "moz-icon",
>+  *   "moz-safe-about"
>+  *
>+  * URI_DOES_NOT_RETURN_DATA - i.e:
>+  *   "mailto"
>+  */
>+  bool isSchemeLocal = false;
>+  bool isSchemeExternal = false;
>+  bool isSchemeInherits = false;
>+  if (  NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &isSchemeLocal))  ||
>+        NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA, &isSchemeExternal)) ||
So this would allow not only wss: loads but also ws: loads?


Coding style is
if (expression) {
  statement;
}
i.e. always use curly brackets and don't add space before nor after expression.

>+
>+  if(isSchemeLocal || isSchemeExternal || isSchemeInherits)
>+     return NS_OK;
>+
> NS_IMETHODIMP
> nsSafeAboutProtocolHandler::GetProtocolFlags(uint32_t *result)
> {
>-    *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE;
>+    *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE | URI_IS_LOCAL_RESOURCE;
I don't quite understand this one.
Why certain about:* uris would have URI_IS_LOCAL_RESOURCE set but not all.
That is rather odd inconsistency.


I wonder... should we add a new protocol flag for this case. Or just whitelist protocols for now.

Comment 18

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

> >+  if (  NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_IS_LOCAL_RESOURCE , &isSchemeLocal))  ||
> >+        NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA, &isSchemeExternal)) ||
> So this would allow not only wss: loads but also ws: loads?
Er, I missed if (aContentType == nsIContentPolicy::TYPE_DOCUMENT || aContentType == nsIContentPolicy::TYPE_WEBSOCKET) {
which I don't understand.
Why is ws: load ok? (wss: sure is)

Comment 19

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Ah, it is per websocket spec.
Could you replace the comment // Websockets code requires same scheme
with something like
// Creating unsecure websocket connections in a secure page is blocked already in websocket constructor.

Comment 20

[Tanvi Vyas[:tanvi]]

Attachment: Add additional secure schemes to nsMixedContentBlocker.cpp

I have updated the patch with the review comments with the coding style change for the if() statement and the comment change for allowing TYPE_WEBSOCKET.

For the about protocol, there are some about: resources that are allowed and some others (where the safe ones are distinguished by moz-about-safe).  More details on this can be found at https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#82.  A resource isSafe if it has the about specific protocol flag nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT.  The change to nsAboutProtocolHandler adds the URI_IS_LOCAL_RESOURCE flag to nsSafeAboutProtocolHandler::GetProtocolFlags (not nsAboutProtocolHandler::GetProtocolFlags)

Comment 21

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests

I've written 8 tests.

wss - allowed in nsMixedContentBlocker.cpp by checking for TYPE_WEBSOCKET

about (ex: about:blank)
unsafe about (ex: about:config) - Should not load since it doesn't have the URI_IS_LOCAL_RESOURCE flag

data, resource, and moz-icon - all have the URI_IS_LOCAL_RESOURCE flag

mailto - has the URI_DOES_NOT_RETURN_DATA flag

javascript - has the URI_INHERITS_SECURITY_CONTEXT flag.

Olli - I have one question about using a for loop for the about and javascript tests which can be done in a generic_iframe, or separating them out.  I have the separated out tests commented in the code and can remove them once I hear your preference.  Thanks!

Comment 22

[Tanvi Vyas[:tanvi]]

Attachment: Add additional secure schemes to nsMixedContentBlocker.cpp

Add additional secure schemes to nsMixedContentBlocker.cpp

I have updated the patch with the review comments with the coding style change for the if() statement and the comment change for allowing TYPE_WEBSOCKET.

For the about protocol, there are some about: resources that are allowed and some others (where the safe ones are distinguished by moz-about-safe).  More details on this can be found at https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#82.  A resource isSafe if it has the about specific protocol flag nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT.  The change to nsAboutProtocolHandler adds the URI_IS_LOCAL_RESOURCE flag to nsSafeAboutProtocolHandler::GetProtocolFlags (not nsAboutProtocolHandler::GetProtocolFlags)

Comment 23

[Tanvi Vyas[:tanvi]]

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

Comment 24

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas from comment #23)
> Push to try: https://tbpl.mozilla.org/?tree=Try&rev=739b3b326556

Something is wrong with my websocket server, but I can't figure out what.  file_mixed_content_main_bug803225_websocket_wsh.py is almost identical to https://mxr.mozilla.org/mozilla-central/source/content/base/test/file_websocket_hello_wsh.py.  Is there another file that needs to be modified in addition to creating the websocket server file?

Comment 25

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas from comment #24)
> (In reply to Tanvi Vyas from comment #23)
> > Push to try: https://tbpl.mozilla.org/?tree=Try&rev=739b3b326556
> 
> Something is wrong with my websocket server, but I can't figure out what. 
> file_mixed_content_main_bug803225_websocket_wsh.py is almost identical to
> https://mxr.mozilla.org/mozilla-central/source/content/base/test/
> file_websocket_hello_wsh.py.  Is there another file that needs to be
> modified in addition to creating the websocket server file?

The websocket test passes locally, but not on try.

Comment 26

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 678937 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

>   // Get the scheme of the sub-document resource to be requested. If it is
>-  // an HTTPS load then mixed content doesn't apply.
>-  bool isHttps;
>+  // an HTTPS or secure load then mixed content doesn't apply  Note that we do
Missing . after apply.

>NS_FAILED(NS_URIChainHasFlags(aContentLocation, nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA, &isSchemeExternal)) ||
URI_DOES_NOT_RETURN_DATA doesn't mean external, so could you change the variable name.

> NS_IMETHODIMP
> nsSafeAboutProtocolHandler::GetProtocolFlags(uint32_t *result)
> {
>-    *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE;
>+    *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE | URI_IS_LOCAL_RESOURCE;
>     return NS_OK;
> }
This still feels like a misuse of URI_IS_LOCAL_RESOURCE to me.


Also, I think this patch is still missing jar: handling.
That requires dealing with nested urls.

Comment 27

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Oh, hmm, is jar: handling not supported anymore.
At least jar:http:// gives security error, but what about jar:https://

Comment 28

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Looks like jar:https://people.mozilla.org/~opettay/jartest.jar!/jartest.html doesn't work either.

Comment 29

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

So then the r- is really about use of URI_IS_LOCAL_RESOURCE.
Could we add a new flag to explicitly allow mixed content?

Comment 30

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 678932 [details] [diff] [review]
Mochitests

># HG changeset patch
># Parent 5b56e1a8686db262b9027eaec7d7f1a3645e0545
># User Tanvi Vyas <[e-mail]>
>Bug 803225 - Test different protocols that are allowed in nsMixedContentBlocker.cpp
>
>diff --git a/content/base/test/Makefile.in b/content/base/test/Makefile.in
>--- a/content/base/test/Makefile.in
>+++ b/content/base/test/Makefile.in
>@@ -578,16 +578,20 @@ MOCHITEST_FILES_B = \
> 		test_XHR_anon.html \
> 		file_XHR_anon.sjs \
> 		test_XHR_system.html \
> 		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 \
>+		bug803225_test_mailto.html \
> 		test_bug789856.html \
> 		$(NULL)
> 
> # OOP tests don't work on Windows (bug 763081) or native-fennec
> # (see Bug 774939)
> ifneq ($(OS_ARCH),WINNT)
> ifndef MOZ_ANDROID_OMTC
> MOCHITEST_FILES_B += \
>diff --git a/content/base/test/bug803225_test_mailto.html b/content/base/test/bug803225_test_mailto.html
>new file mode 100644
>--- /dev/null
>+++ b/content/base/test/bug803225_test_mailto.html
>@@ -0,0 +1,13 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+Tests for Mixed Content Blocker - Mailto Protocol Compose Page
>+https://bugzilla.mozilla.org/show_bug.cgi?id=803225
>+-->
>+<head> <meta charset="utf-8"> 
>+</head>
>+<body>
>+Hello
>+<script>window.close();</script>
>+</body>
>+</html>
>diff --git a/content/base/test/file_mixed_content_main_bug803225.html b/content/base/test/file_mixed_content_main_bug803225.html
>new file mode 100644
>--- /dev/null
>+++ b/content/base/test/file_mixed_content_main_bug803225.html
>@@ -0,0 +1,217 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+Tests for Mixed Content Blocker - Allowed Protocols
>+https://bugzilla.mozilla.org/show_bug.cgi?id=803225
>+-->
>+<head>
>+  <meta charset="utf-8">
>+  <title>Tests for Bug 62178</title>
>+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>+</head>
>+<body>
>+<div id="testContent"></div>
>+
>+<!-- Test additional schemes the Mixed Content Blocker should not block
>+     "about" protocol URIs that are URI_SAFE_FOR_UNTRUSTED_CONTENT (moz-safe-about; see nsAboutProtocolHandler::NewURI
>+     "data",
>+     "javascript",
>+     "mailto",
>+     "resource",
>+     "moz-icon",
>+     "wss"
>+-->
>+
>+<script>
>+
>+  //For tests that require setTimeout, set the maximum polling time to 100 x 100ms = 10 seconds.
>+  var MAX_COUNT = 100;
>+  var TIMEOUT_INTERVAL = 100;
>+
>+  var testContent = document.getElementById("testContent");
>+
>+  // Test 1 & 2: about and javascript protcols within an iframe
>+  var data = Array(2,2);
>+  var protocols = [
>+                    ["about", ""], //When no source is specified, the frame gets a source of about:blank
>+                    ["javascript", "javascript:document.open();document.write='<h1>SUCCESS</h1>';document.close();"],
>+                  ];
>+  for(var i=0; i < 2; i++)
s/2/protocols.length/
That would make the code easier to read I think


>+  /* smaug - do you prefer using the above for loop, or would you like individual tests instead?
Loop is ok

>+  // Test 1: about protocol within an iframe
>+  var about_frame = document.createElement("iframe");
>+  //When no source is specified, the frame gets a source of about:blank
>+  about_frame.src = "";
>+  about_frame.name="about_protocol";
>+  about_frame.onload = function() {
>+    parent.postMessage({"test": "about", "msg": "resource with about protocol loaded"}, "http://mochi.test:8888");
>+  }
>+  about_frame.onerror = function() {
>+    parent.postMessage({"test": "about", "msg": "resource with about protocol did not load"}, "http://mochi.test:8888");
>+  }
>+  testContent.appendChild(about_frame);
>+  */
>+  /*
>+  // Test 2: javascript protocol within an iframe
>+  var javascript_frame = document.createElement("iframe");
>+  javascript_frame.src = "javascript:document.open();document.write='<h1>SUCCESS</h1>';document.close();";
>+  //Do we need to open and close the document?  We could do this instead ->
>+  //javascript_frame.src = "javascript:document.write='<h1>SUCCESS</h1>';";
>+  javascript_frame.name="javascript_protocol";
>+  javascript_frame.onload = function() {
>+    parent.postMessage({"test": "javascript", "msg": "resource with javascript protocol loaded"}, "http://mochi.test:8888");
>+  }
>+  javascript_frame.onerror = function() {
>+    parent.postMessage({"test": "javascript", "msg": "resource with javascript protocol did not load"}, "http://mochi.test:8888");
>+  };
>+  testContent.appendChild(javascript_frame);
>+  */
Hmm, does the error event actually work in these cases? If we would
block javascript: or about: I don't think error event would be dispatched.
So I think onerror cases can be removed, probably everywhere.
checkTestsCompleted() should anyway check that all the tests were run successfully.


That thing changed, r=me

>+    //if the testsToRun are all completed, chnage the pref and run the tests again until we have cycled through all the prefs.
change

Comment 31

[Tanvi Vyas[:tanvi]]

Attachment: Add additional secure schemes to nsMixedContentBlocker.cpp

Fixed Olli's comments in comment 26.  Added a new URI Protocol Flag called URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT that can be used in case we want to force a scheme to be secure that does not fall into URI_DOES_NOT_RETURN_DATA, URI_IS_LOCAL_RESOURCE, or URI_INHERITS_SECURITY_CONTEXT (ex: https and moz-safe-about).

Resubmitting for review.

Comment 32

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests

Fixed the review comments in comment 30, except for removing the onerror events...

checkTestsCompleted is dependent on file_mixed_content_main_bug803225.html to send a post-message.  The onerror event does fire, as you can see with the unsafe about test, where we try to load an iframe with src about:config.  The onerror event fires, and sends a message "resource with unsafe about protocol did not load" to the parent.  The parent then marks that test as completed by setting unsafe_about to true in testsToRun.  (Note that it also goes through a loop and runs this test 4 times, once for each setting combination.)

I tested that moz-icon, safe about, javascript, resource do fire the onerror when the resource does not load.  I did this by removing my patch to nsMixedContentBlocker and then running the tests.  Hence, I'm leaving in the onerror events.

I found a few other quirks while testing (don't need to append child for moz-icon, replaced name of data test with data_protocol, to avoid overloading "data".  I also found that mailto: URIs seem to have a content type of TYPE_DOCUMENT, and hence are allowed with or without the patch to nsMixedContentBlocker.  

Can I carry over the r+?

Comment 33

[Tanvi Vyas[:tanvi]]

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

Comment 34

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 679498 [details] [diff] [review]
Mochitests

Make sure to run this in tryserver few time to ensure we don't
cause random orange.

Comment 35

[Olli Pettay [:smaug][[e-mail]] (vacation-ish -> Aug 1)]

Comment on attachment 679419 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp


>+
>+  if(isSchemeLocal || isSchemeNoReturnData || isSchemeInherits || isSchemeSecure)
>+     return NS_OK;

if (expr) {
  stmt;
}

Always use {} and 2 space indentation.

>+    /**
>+     * URI is secure to load in an https page and should not be blocked
>+     * by nsMixedContentBlocker
>+     */
>+    const unsigned long URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT = (1<<17);
>+
17 has been used already. Use 18.
nsIProtocolHandler and nsAboutProtocolHandler.cpp changes need Necko peer review.

Comment 36

[Tanvi Vyas[:tanvi]]

Attachment: Add additional secure schemes to nsMixedContentBlocker.cpp

Rebased addressing Olli's comment 35.  Carrying over r+.

Comment 37

[Tanvi Vyas[:tanvi]]

(In reply to Olli Pettay [:smaug] from comment #34)
> Comment on attachment 679498 [details] [diff] [review]
> Mochitests
> 
> Make sure to run this in tryserver few time to ensure we don't
> cause random orange.

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

Comment 38

[Tanvi Vyas[:tanvi]]

Comment on attachment 679419 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

Per Olli, requesting Necko Peer Review for the new flag in nsIProtocolHandler.idl.

Thanks Olli for your reviews!

Comment 39

[Boris Zbarsky [:bzbarsky]]

Er... do you really want review from me on the obsolete patch?  Or on something else?

Comment 40

[Tanvi Vyas[:tanvi]]

Comment on attachment 680145 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

(In reply to Boris Zbarsky (:bz) from comment #39)
> Er... do you really want review from me on the obsolete patch?  Or on
> something else?

Oops!  Sorry about that; I marked the wrong patch.  This is the patch I'd like you to review, specifically for the change in nsIProtocolHandler.idl.  Thanks!

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 680145 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

s/unsecure/insecure/ ?

s/aRequestLocation\.\./aRequestLocation./

The comment about "avoid calculating" makes no sense to me.  The check for aContentLocation happens _after_ we already checked aRequestLocation!

s/Protcl/Protocol/

All your "i.e" should be "e.g."

Why are we leaving the "https" special-case?  Seems like it would be covered by the protocol flag checks, no?

You need a space after your "if".

The booleans on the stack there should probably be "schemeLocal" and so forth, without the extra "is".

r=me with those nits picked, though I'd like to see the updated patch.

Comment 42

[Tanvi Vyas[:tanvi]]

Attachment: Add additional secure schemes to nsMixedContentBlocker.cpp

(In reply to Boris Zbarsky (:bz) from comment #41)
> Comment on attachment 680145 [details] [diff] [review]
> Add additional secure schemes to nsMixedContentBlocker.cpp
> 
> s/unsecure/insecure/ ?
>
Fixed

> s/aRequestLocation\.\./aRequestLocation./
>
Removed
 
> The comment about "avoid calculating" makes no sense to me.  The check for
> aContentLocation happens _after_ we already checked aRequestLocation!
> 
Sorry, that will happen in a separate patch, and I accidentally left in that comment.

> s/Protcl/Protocol/
> 
Fixed

> All your "i.e" should be "e.g."
>
Done
 
> Why are we leaving the "https" special-case?  Seems like it would be covered
> by the protocol flag checks, no?
> 
Yes, you are right.  We don't need a separate case for https since it is covered in URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT.  I took the https special case out and updated the comments.

> You need a space after your "if".
> 
Fixed in 3 places.

> The booleans on the stack there should probably be "schemeLocal" and so
> forth, without the extra "is".
> 
Done

> r=me with those nits picked, though I'd like to see the updated patch.
Updated patch attached.  Carrying over r+ from smaug.  BZ, let me know if you are okay with this patch and then we can land it :)

Comment 43

[Tanvi Vyas[:tanvi]]

Comment on attachment 679498 [details] [diff] [review]
Mochitests

Per comment 41 and 42, waiting for BZ's final approval.

Fresh try run - https://tbpl.mozilla.org/?tree=Try&rev=9719ade02d5e

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 681289 [details] [diff] [review]
Add additional secure schemes to nsMixedContentBlocker.cpp

Looks good.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 679498 [details] [diff] [review]
Mochitests

Why do you need to limit the polling more than the normal test harness timeout does?  Seems like that will just trigger random orange, no?

r=me with that removed or very clearly explained.

Comment 46

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests

(In reply to Boris Zbarsky (:bz) from comment #45)
> Comment on attachment 679498 [details] [diff] [review]
> Mochitests
> 
> Why do you need to limit the polling more than the normal test harness
> timeout does?  Seems like that will just trigger random orange, no?
> 
> r=me with that removed or very clearly explained.

There are 8 mochitests that are tested 4 times each (with 4 different setting combinations for blocking/allowing display and active content).  For situations where an error event is not an available option, we need to set a polling timeout to determine whether a resource loaded or not.  If we do not set a polling time limit, the test will get stuck on a specific resource that doesn't load and the remaining tests won't execute.

This is what we used in the mochitest for bug 62178 as well.  In bug 62178 it was necessary to poll since sometimes the failure of a resource to load would result in a Passing Test (since that resource wasn't supposed to load) and we would want to continue on to the next test.

For this bug, we are using polling for the mailto and data protocols, both which are expected to load.  If we do not poll and the data protocol fails to load, the test will timeout and the remaining 24+ tests won't execute.

The exact max time limit (10 seconds) was proposed by Olli.  You can also see more details on our discussion about polling in https://bugzilla.mozilla.org/show_bug.cgi?id=62178#c229 and comment 230. 

I have added a couple comments to the code in the places where we poll to indicate why polling is necessary.

Carrying over the r+'s.  Can we land this?

Comment 47

[Boris Zbarsky [:bzbarsky]]

> If we do not set a polling time limit, the test will get stuck on a specific resource
> that doesn't load and the remaining tests won't execute.

And then eventually the harness timeout will kick in and this test will be marked as failing and turn the tree orange.  Why is this a problem?  If we were expecting tests to pass on not loading it would be, but as you said, that's not the case here.

> If we do not poll and the data protocol fails to load, the test will timeout and the
> remaining 24+ tests won't execute.

Sure.  Why is that a problem?

Given your description above, I think you should take the polling out.  It'll just lead to random oranges, with no perceptable benefit I see.

Comment 48

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests

(In reply to Boris Zbarsky (:bz) from comment #47)
> Given your description above, I think you should take the polling out. 
> It'll just lead to random oranges, with no perceptable benefit I see.

Okay, removed the polling.

I think this is ready to land.  Any volunteers to land it?

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 682117 [details] [diff] [review]
Mochitests

So I'm also a bit confused about the dataProtocolStatus bit.  It's polling with a comment about a bug about onerror... but that bug is fixed.  Does that bit still need to poll at all?

Comment 50

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas from comment #48)
> Created attachment 682117 [details] [diff] [review]
> Mochitests
> 
> (In reply to Boris Zbarsky (:bz) from comment #47)
> > Given your description above, I think you should take the polling out. 
> > It'll just lead to random oranges, with no perceptable benefit I see.
> 
> Okay, removed the polling.
> 
Sorry, I meant that I removed the MAX_TIMEOUT for the polling.  So now it will poll until success or timeout.

Comment 51

[Tanvi Vyas[:tanvi]]

(In reply to Boris Zbarsky (:bz) from comment #49)
> Comment on attachment 682117 [details] [diff] [review]
> Mochitests
> 
> So I'm also a bit confused about the dataProtocolStatus bit.  It's polling
> with a comment about a bug about onerror... but that bug is fixed.  Does
> that bit still need to poll at all?

I am looking into this.

Comment 52

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests

(In reply to Boris Zbarsky (:bz) from comment #49)
> Comment on attachment 682117 [details] [diff] [review]
> Mochitests
> 
> So I'm also a bit confused about the dataProtocolStatus bit.  It's polling
> with a comment about a bug about onerror... but that bug is fixed.  Does
> that bit still need to poll at all?

You are right, since bug 789856 is now fixed, we don't need to poll for the data_protocol <script> and can now use the error event.  I've changed the data protocol tests accordingly.

Comment 53

[Tanvi Vyas[:tanvi]]

Attachment: Mochitests

Fixing a typo.

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df27137cb7d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bdf8bbcd02f

Comment 55

[Ryan VanderMeulen [:RyanVM]]

http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound

Backed out for mochitest failures. I knew I should have run this through Try first...
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4a9a61a335

https://tbpl.mozilla.org/php/getParsedLog.php?id=17088443&tree=Mozilla-Inbound

40393 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_mixed_content_blocker_bug803225.html | resource with data protocol did not load
40401 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_mixed_content_blocker_bug803225.html | resource with data protocol did not load
40409 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_mixed_content_blocker_bug803225.html | resource with data protocol did not load
40417 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_mixed_content_blocker_bug803225.html | resource with data protocol did not load

Please run this through Try before requesting checkin again.

Comment 56

[reuseme2600]

The patch that was pushed was not the most recent patch, it is for an obsoleted patch. Here is the try push for the most recent patch on this bug: https://tbpl.mozilla.org/?tree=Try&rev=47a49ff238f5

Comment 57

[Ian Melven :imelven]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87e53d2e1e53
https://hg.mozilla.org/integration/mozilla-inbound/rev/158f2345bf97

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87e53d2e1e53
https://hg.mozilla.org/mozilla-central/rev/158f2345bf97

Sorry about the mix-up yesterday. No clue how I managed to land the wrong mochitest patch.