737873 - Implement mixed content highlighting in the web console

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Mark Goodwin [:mgoodwin]]

Implement mixed content highlighting in the web console, as per suggestions here: http://people.mozilla.org/~mgoodwin/devtools_ideas/

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch

Attaching a patch.  Still need to write a test case for it.

Makes the mixed content Get/Post red in the web console and adds a SEVERITY_WARNING

Comment 2

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch Take 2

Had to change where severity was declared, because it was defined inside the scope of the if statement, and wouldn't work when we called createMessageNode.

We are testing.

Comment 3

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch Take 2

Trying to attach again.

Comment 4

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch Take 2

Sorry, one more time.

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch Take 3

Had to remove an extra let.

Comment 6

[Paul Rouget [:paul]]

Marking the line as a warning is something we can do, but we will also need to explain why we mark this line as a warning. I suggest that we add a message at the end of the line ("[http content in https content]") with a link to a MDN page.

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch Take 4

Thanks for the pointers Paul.  We've updated our patch to include a message.  We also changed the setAttribute so that we are only changing the color, and not cloberring additional style changes.

We still need a mochitest.

Comment 8

[Mihai Sucan [:msucan]]

Thanks for the really good work Tanvi (and Mark)!

Patch looks good - I may only suggest that the urlNode is given a class name, so themes can change the color as needed (instead of hard-coding urlNode.style.color).

Looking forward for the updated patch. Thanks!

(I hope it's OK that I am updating the bug state to reflect you are working on it.)

Comment 9

[Paul Rouget [:paul]]

I'd be very interested to review/feedback this code. Please f? or r? me once you get a near-final version of the patch. Thanks!

Comment 10

[Tanvi Vyas[:tanvi]]

http://productforums.google.com/forum/#!msg/chrome/pPjI-1ojleg/Km2X-2uPZ34J

Comment 11

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

Comment on attachment 608305 [details] [diff] [review]
Mixed Content Webconsole Patch Take 4

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

::: browser/devtools/webconsole/HUDService.jsm
@@ +2757,5 @@
> +    let contentWindowURIScheme = contentWindow.location.protocol;
> +
> +    let severity = SEVERITY_LOG;
> +    if(contentWindowURIScheme == "https:" && aActivityObjectScheme != "https") {
> +	urlNode.style.color = "#FF0000";

Is it common to directly set colors in the console JS code? It seems like it would be cleaner to use a class or another attribute and then do one of the following in the CSS:

   .webConsoleMixedContentWarning { color: red }
or:
   [whateverAttribute=webConsoleMixedContentWarning] { color: red}

Also, this highlights the message in red, and red indicates errors. But, you have set the severity to SEVERITY_WARNING. I think it should be SEVERITY_ERROR or the color shouldn't be red.

Also, should we have different warnings for active vs. passive content? Perhaps the message for active content should be red/SEVERITY_ERROR and the warning for passive content should be yellow/SEVERITY_WARNING?

Comment 12

[Tanvi Vyas[:tanvi]]

Hey Brian,

> Also, should we have different warnings for active vs. passive content?
> Perhaps the message for active content should be red/SEVERITY_ERROR and the
> warning for passive content should be yellow/SEVERITY_WARNING?

We can't determine active/passive content until Brandon lands bug 62178.

For your other comments, I will talk to Paul/Mihai to see if I should set it as SEVERITY_ERROR or SEVERITY_WARNING.  And also figure out if there is a css file I can set the color in, instead of in the js directly.

Thanks for your feedback!

Comment 13

[Tanvi Vyas[:tanvi]]

Patch no longer works... need to update it after some changes that removed aActivityObject - http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/HUDService.jsm?rev=ebe7a997ac58#2370

Comment 14

[Mihai Sucan [:msucan]]

(In reply to Tanvi Vyas from comment #12)
> Hey Brian,
> 
> > Also, should we have different warnings for active vs. passive content?
> > Perhaps the message for active content should be red/SEVERITY_ERROR and the
> > warning for passive content should be yellow/SEVERITY_WARNING?
> 
> We can't determine active/passive content until Brandon lands bug 62178.
> 
> For your other comments, I will talk to Paul/Mihai to see if I should set it
> as SEVERITY_ERROR or SEVERITY_WARNING.  And also figure out if there is a
> css file I can set the color in, instead of in the js directly.

The CSS file is browser/themes/(gnomestripe|winstripe|pinstripe)/devtools/webconsole.css. gnomestripe for Linux, winstripe for Windows and pinstripe for Mac.

I would suggest SEVERITY_WARNING, but do note we should probably have SEVERITY_SECURITY (if Paul agrees). This would nicely allow us to add "Security" to the Logging drop down. Paul, what do you think?

(In reply to Tanvi Vyas from comment #13)
> Patch no longer works... need to update it after some changes that removed
> aActivityObject -
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/
> HUDService.jsm?rev=ebe7a997ac58#2370

Yeah, sorry about that. We've done a lot of work to make the Web Console UI async and many things moved out. This patch now needs an update / a rewrite.

The activity object moved into HUDService-content.js, but I do not suggest you make changes there, since these are UI-only changes. What you can do is change the code in HUD_logNetActivity() (search for this function in HUDService.jsm). In this function I believe you can change the severity of the message being created based on the request.url. You also have the current page URL in the HUD object (this.contentLocation).

I hope this helps. Please let me know if you have any questions.


(thanks for your work - this is great stuff!)

Comment 15

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch Refactored

I've updated the patch and attached it here.  I haven't addressed any of the new comments, just trying to make it work again.  

But it still isn't working.  Not sure what I should change these lines to:

let chromeWindow = HUDService.currentContext();
let contentWindow = chromeWindow.gBrowser.selectedBrowser.contentWindow;
let contentWindowURIScheme = contentWindow.location.protocol;

I've noticed that in other parts of the code, chromeWindow.gBrowser.selectedBrowser.contentWindow has been changed to chromeWindow.gBrowser.selectedTab.linkedPanel.  selectedTab doesn't seem to have a contentWindow from which I can extract the protocol.  Should I be using selectedTab.ownerDocument?

I'm getting the following error:
JavaScript error: resource:///modules/HUDService.jsm, line 2129: HUDService.getStr is not a function.  Line 2129 is a blank line.

Thanks!

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 632049 [details] [diff] [review]
Mixed Content Webconsole Patch Refactored

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

Thanks for your work Tanvi! Comments below...

::: browser/devtools/webconsole/HUDService.jsm
@@ +2124,5 @@
> +    let requestURIScheme = requestURIObject.scheme;
> +    
> +    let chromeWindow = HUDService.currentContext();
> +    let contentWindow = chromeWindow.gBrowser.selectedBrowser.contentWindow;
> +    let contentWindowURIScheme = contentWindow.location.protocol;

this.contentLocation is the page URL for the current web console instance. Use that instead of these three lines.

To determine the protocol you have to use nsIURI again (or you can use a regex for both cases instead of nsIURI).

@@ +2130,5 @@
> +    let severity = SEVERITY_LOG;
> +    if(contentWindowURIScheme == "https:" && requestURIScheme != "https") {
> +        urlNode.style.color = "#FF0000";
> +
> +        let mixedContentWarning = " ["+HUDService.getStr("webConsoleMixedContentWarning")+"]";

HUDService.getStr() has been replaced with l10n.getStr().

Comment 17

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch Refactored with bug

Seems to be a bug with this.contentLocation that returns the previous location instead of the current one.

Comment 18

[Tanvi Vyas[:tanvi]]

I added in a dump for requestURI -- dump("requestURI: " + requestURIObject.host + ".\n\n\n"); -- and below is what I observed.

***Open webconsole
***Visit cisforcookie.org
this.contentLocation:: about:home.
requestURI: visit.webhosting.yahoo.com.
contentWindowURIScheme: about.
requestURIScheme: http.


***Visit https://people.mozilla.com/~bsterne/tests/62178/test.html
this.contentLocation:: http://cisforcookie.org/.
requestURI: people.mozilla.com.
contentWindowURIScheme: http.
requestURIScheme: https.


++DOMWINDOW == 10 (0x10fc5b480) [serial = 12] [outer = 0x10fbd1000]
++DOCSHELL 0x10fc64c00 == 5 [id = 5]
++DOMWINDOW == 11 (0x10fc65880) [serial = 13] [outer = 0x0]
++DOMWINDOW == 12 (0x11e803880) [serial = 14] [outer = 0x10fc65800]
this.contentLocation:: http://cisforcookie.org/.
requestURI: people.mozilla.com.
contentWindowURIScheme: http.
requestURIScheme: https.


this.contentLocation:: http://cisforcookie.org/.
requestURI: people.mozilla.com.
contentWindowURIScheme: http.
requestURIScheme: http.

***Visit https://etherpad.mozilla.org
this.contentLocation:: https://people.mozilla.com/~bsterne/tests/62178/test.html.
requestURI: etherpad.mozilla.org.
contentWindowURIScheme: https.
requestURIScheme: https.


++DOMWINDOW == 13 (0x10e232480) [serial = 15] [outer = 0x10fbd1000]

Comment 19

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch Refactored with potential bug in this.contentLocation

Patch is compiles but does not work as intended (due to potential bug in this.contentLocation).  (In the meantime, to see a highlightened log entry, go to an https page followed by an http page.)

Fixed the styling so that the font color is in css.

Not sure how to make the Mixed Content text a clickable link.  I don't want the whole line to be a link, just the mixedContentWarning portion:

urlNode.setAttribute("value", request.url+mixedContentWarning);

Mihai, do you have any suggestions for that?

As for SEVERITY_WARNING, SEVERITY_ERROR, or SEVERITY_SECURITY, I'm going to leave int SEVERITY_WARNING for now until I have a discussion with Paul and Mihai on where we want security to fit in on the webconsole.

Thanks for your help Mihai!

Comment 20

[Mihai Sucan [:msucan]]

Tanvi: thanks for your patch.

I identified the problem: contentLocation is only updated when page load completes, hence you still see the previous location during loading, while you receive the network requests. I forgot this bit of implementation detail. My bad!

When the code was written I needed contentLocation to update the Web Console window title and for that case I chose to wait for load to end.

To fix this issue you have I will make a patch for you.

Comment 21

[Mihai Sucan [:msucan]]

Attachment: make HUDService-content send the new page location earlier

Tanvi: this patch should fix the problem you have. Please let me know if it works for you. Thanks!

I suggest you apply this patch before yours.

Comment 22

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch - Refactored

With Mihai's patch, my patch for mixed content highlighting is now working!  Here is the updated patch.

I still need to add a clickable link to more information, so that developers can learn the issues with mixed content.  I also need to determine what to do in the catch block.  Then I need to go back to working on the mochitests.

Thanks Mihai for your help!

Comment 23

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch - Refactored

Patch for making mixed content patches red.  I'll file a separate bug for adding a Learn More link.  Need to still figure out what to do in the catch and update that.

Comment 24

[Tanvi Vyas[:tanvi]]

Attachment: Mochitest

Here is the beginning of my mochitest.  The test is oddly failing.  When the console is opened, and the mixed content is loaded, the entry in webconsole is not red and does not say "Mixed Content".  But when I hit refresh it does.

When I load a mixed content page in the browser with the patch applied (example: https://people.mozilla.com/~bsterne/tests/62178/test.html) it works.  So I'm not sure what the problem is with the test case.

Mihai, Panos, Paul - any ideas?  Thanks for your help!

Comment 25

[Mihai Sucan [:msucan]]

Comment on attachment 636892 [details] [diff] [review]
Mixed Content Webconsole Patch - Refactored

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

This looks good to me.

Just one nit: please make sure patches do not have any trailing whitespace.

::: browser/devtools/webconsole/HUDService.jsm
@@ +2128,5 @@
> +    } catch(e) {
> +        /*
> +         * TODO: catch NS_ERROR_MALFORMED_URI?
> +         * if this error occurs, we skip the mixed content section and 
> +         * the webconsole does not warn about mixed content

I think this try-catch is sufficient. If the URI is malformed you just skip the "mixed content" check.

Do you want some specific behavior when the URI is malformed? If not, not much to do here.

@@ +2149,5 @@
> +
> +          /*TODO: take mixedContentWarning message and make it a link to a 
> +             mdn page that explains mixed content.  How do I do this?
> +            let mixedContentWarning = " [<a href='http://cisforcookie.org'>"+l10n.getStr
> +                                        ("webConsoleMixedContentWarning")+"<a>]";

You can do this as follows:

let mixedContentWarningNode = this.chromeDocument.createElement("label");
mixedContentWarningNode.setAttribute("value", mixedContentWarning);
linkNode.appendChild(mixedContentWarningNode);

To make that open a page do:

mixedContentWarningNode.addEventListener("click", function(aEvent) [
  this.chromeWindow.openUILinkIn(thePageYouWant, 'tab');
  aEvent.preventDefault();
  aEvent.stopPropagation();
}.bind(this));

(This is untested code - an approximation of how it should be. Please let me know if something doesn't work as desired.)

Comment 26

[Mihai Sucan [:msucan]]

Comment on attachment 636894 [details] [diff] [review]
Mochitest

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

Test looks good, but it needs to take into consideration the asyncness of the Web Console.

::: browser/devtools/webconsole/test/browser_webconsole_bug_737873_mixedcontent.js
@@ +10,5 @@
> +}
> +
> +function onLoad(aEvent) {
> +  browser.removeEventListener(aEvent.type, arguments.callee, true);
> +  //browser.removeEventListener("load", onLoad, true);

arguments.callee is deprecated.

@@ +11,5 @@
> +
> +function onLoad(aEvent) {
> +  browser.removeEventListener(aEvent.type, arguments.callee, true);
> +  //browser.removeEventListener("load", onLoad, true);
> +  openConsole();

While this *might* work, your best bet here is to provide an onOpen callback. The Web Console open is async.

openConsole(null, function() {
  ... load the test uri ...
});

@@ +33,5 @@
> +      found = true;
> +      break;
> +    }
> +  }
> +  ok(found, "Log Entry found is:" + found);

This should almost work, but all messages show async, so you can't be sure the message is already in the console output by the time of the page "load" event.

In other tests you may have seen waitForSuccess() is used a lot.

I suggest the following approach:

let hud = HUDService.getHudByWindow(content);

waitForSuccess({
  name: "mixed content warnings displayed",
  validatorFn: function() {
    // you have added the .webconsole-mixed-content class to the urlNode. let's use that to find the mixed content warnings. also we know from the test file that the web console should display only one warning.
    return hud.outputNode.querySelector(".webconsole-mixed-content");
  },
  successFn: function() {
    let node = hud.outputNode.querySelector(".webconsole-mixed-content");
    ... check anything you like for the node ...
    finishTest();
  },
  failureFn: finishTest,
});

Please let me know if this approach works for you. If you bump into issues, please let us know.

Thanks!

Comment 27

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch - UI styling incomplete

Updated my patch with some of Mihai's suggestions.  Need to update the UI a bit - there is an extra _ before [Mixed Content] and I'm not sure why.  Also, trying to experiment with making the color blue so that developers know that it is a clickable link.  Will work on the mochitests next and then get back to the UI later.

On another note, the class webconsole-msg-status doesn't seem to exist.  See like 2187:

2183     let statusNode = this.chromeDocument.createElementNS(XUL_NS, "label");
2184     statusNode.setAttribute("value", "");
2185     statusNode.classList.add("hud-clickable");
2186     statusNode.classList.add("webconsole-msg-body-piece");
2187     statusNode.classList.add("webconsole-msg-status");

Comment 28

[Mihai Sucan [:msucan]]

(In reply to Tanvi Vyas from comment #27)
> Created attachment 637338 [details] [diff] [review]
> Mixed Content Webconsole Patch - UI styling incomplete
> 
> Updated my patch with some of Mihai's suggestions.  Need to update the UI a
> bit - there is an extra _ before [Mixed Content] and I'm not sure why. 
> Also, trying to experiment with making the color blue so that developers
> know that it is a clickable link.  Will work on the mochitests next and then
> get back to the UI later.

The extra _ before the [Mixed Content] is easy to fix. You have:

let mixedContentWarning = " ["+l10n.getStr("webConsoleMixedContentWarning")+"]";

The extra space before the [ causes the extra _ to show. It's the underline of a blank space.


> On another note, the class webconsole-msg-status doesn't seem to exist.  See
> like 2187:
> 
> 2183     let statusNode = this.chromeDocument.createElementNS(XUL_NS,
> "label");
> 2184     statusNode.setAttribute("value", "");
> 2185     statusNode.classList.add("hud-clickable");
> 2186     statusNode.classList.add("webconsole-msg-body-piece");
> 2187     statusNode.classList.add("webconsole-msg-status");

Heh, yes, that might as well be the case. :) Maybe we have .webconsole-msg-status and no actual styling in webconsole.css. Don't worry about it.


UI looks good. I like this stuff. Thanks!

Comment 29

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch - UI styling almost complete

A couple of changes.  Need to write an mdn page about mixed content and link to it.  If possible, I'd like to make "Mixed Content" appear in blue, but if I can't get it in by ff16, I'll add it later as a feature enhancement.

Comment 30

[Tanvi Vyas[:tanvi]]

Attachment: Mochitest

Thanks Mihai for the feedback.  I think this looks better now.  I still need to figure out where the severity is stored and how to simulate a click and check the outcome.

Comment 31

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch - Ready for Review

Finished the patch and the mochitests.  Couple of minor questions in in-line comments.

Comment 32

[Tanvi Vyas[:tanvi]]

Attachment: Mochitest

Finished the patch and the mochitests.  Couple of minor questions in in-line comments.

Pushing to try shortly.

Comment 33

[Mihai Sucan [:msucan]]

Comment on attachment 638552 [details] [diff] [review]
Mixed Content Webconsole Patch - Ready for Review

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

Giving the patch r+, but please address the comments below. I like that patch - it does what it is intended to do.

Please also ask for review from a browser peer - you are changing webconsole.css which lives in the browser (for now). See the list of browser peers here:
https://wiki.mozilla.org/Modules/Firefox

Thank you!

::: browser/devtools/webconsole/HUDService.jsm
@@ +2126,5 @@
> +      contentWindowURI = ioServ.newURI(this.contentLocation, null, null);
> +      uriCreated = true;
> +    } catch(e) {
> +        /*
> +         * catch NS_ERROR_MALFORMED_URI?

Why do you have the question mark at the end here?

I would actually prefer you remove this comment since it's not needed.

@@ +2136,5 @@
> +
> +    // if the operations to create nsIURI objects for the contentWindow and request succeeeded
> +    if(uriCreated) {
> +
> +      //requestURIScheme is the scheme of the requested content - the entry in webconsole

nit: code style - please put a space after //.

Also general nit: we add comments only when we do things that might be confusing to people who read the code. For generic stuff, we do not comment too much.

@@ +2146,5 @@
> +      if(contentWindowURIScheme == "https" && requestURIScheme != "https") {
> +        urlNode.classList.add("webconsole-mixed-content");
> +        linkNode.appendChild(urlNode);
> +
> +        let mixedContentWarning = "["+l10n.getStr("webConsoleMixedContentWarning")+"]";

Nit: code style - please put spaces before and after the +.

@@ +2155,5 @@
> +        mixedContentWarningNode.setAttribute("title", mixedContentWarning);
> +
> +        //UI for mixed content warning.
> +        mixedContentWarningNode.classList.add("hud-clickable");
> +        mixedContentWarningNode.classList.add("webconsole-msg-body");  //should this be webconsole-msg-body-piece instead?

Either way. You any class that achieves the desired visual effect.

@@ +2156,5 @@
> +
> +        //UI for mixed content warning.
> +        mixedContentWarningNode.classList.add("hud-clickable");
> +        mixedContentWarningNode.classList.add("webconsole-msg-body");  //should this be webconsole-msg-body-piece instead?
> +        mixedContentWarningNode.classList.add("webconsole-mixed-content-link");

I'd actually copy the properties I need from .webconsole-msg-body over to .webconsole-mixed-content-link. I assume you only need margin-related stuff. Maybe margin:0? like in .webconsole-msg-body-piece. Really, do as you like here.

Paul will change all of this when he does the new Web Console theme.

@@ +2160,5 @@
> +        mixedContentWarningNode.classList.add("webconsole-mixed-content-link");
> +
> +        linkNode.appendChild(mixedContentWarningNode);
> +
> +        let learnMore="https://developer.mozilla.org/";

Before we can land this patch, please update the link to point to the actual wiki page you want.

Also please put this address in a constant defined at the top of the HUDService.jsm file.

@@ +2163,5 @@
> +
> +        let learnMore="https://developer.mozilla.org/";
> +
> +        mixedContentWarningNode.addEventListener("click", function(aEvent) {
> +          this.chromeWindow.openUILinkIn(learnMore, 'tab');

nit: code style - please use double quotes.

@@ +2172,5 @@
> +
> +        //If we define a SEVERITY_SECURITY in the future, switch this to SEVERITY_SECURITY
> +        severity = SEVERITY_WARNING;
> +
> +      } else { //not mixed content

nit: file code style - please put the else { on a new line.

}
else {
...
}

@@ +2177,5 @@
> +        linkNode.appendChild(urlNode);
> +      }
> +
> +    } else { //uriCreated failed
> +      linkNode.appendChild(urlNode);

Why not put linkNode.appendChild(urlNode) before the big if code block? You do this in all cases, anyway. These else blocks are not needed.

Comment 34

[Mihai Sucan [:msucan]]

Comment on attachment 638553 [details] [diff] [review]
Mochitest

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

Test looks good, but the part with the tab open needs a bit of changes. Comments below.

::: browser/devtools/webconsole/test/Makefile.in
@@ +109,5 @@
>  	browser_webconsole_window_zombie.js \
>  	browser_cached_messages.js \
>  	browser_bug664688_sandbox_update_after_navigation.js \
>  	browser_webconsole_menustatus.js \
> +  browser_webconsole_bug_737873_mixedcontent.js \

nit: code style - please align this new test file with the other tests above.

::: browser/devtools/webconsole/test/browser_webconsole_bug_737873_mixedcontent.html
@@ +1,2 @@
> +<!DOCTYPE HTML>
> +<html dir="ltr" xml:lang="en-US" lang="en-US"><head>

Please add the public domain license boilerplate to this test file.

http://www.mozilla.org/MPL/boilerplate-1.1/

::: browser/devtools/webconsole/test/browser_webconsole_bug_737873_mixedcontent.js
@@ +23,5 @@
> +}
> +
> +function testMixedContent() {
> +  content.location = TEST_HTTPS_URI;
> +  var aOutputNode = HUDService.getHudByWindow(content).outputNode;

You do not need to call getHudByWindow() here. testMixedContent() is the openConsole() callback which means you get the HUD object.

function testMixedContent(hud){
  content.location = TEST_HTTPS_URI;
  let outputNode = hud.outputNode;
  ...

@@ +25,5 @@
> +function testMixedContent() {
> +  content.location = TEST_HTTPS_URI;
> +  var aOutputNode = HUDService.getHudByWindow(content).outputNode;
> +
> +  waitForSuccess (

nit: code style - please remove the space after the function name.

@@ +29,5 @@
> +  waitForSuccess (
> +    {
> +      name: "mixed content warning displayed successfully",
> +      validatorFn: function() {
> +        return ( aOutputNode.querySelector(".webconsole-mixed-content") && aOutputNode.querySelector(".webconsole-mixed-content-link") );

Are both querySelector() calls needed here?

@@ +40,5 @@
> +        ok(testSeverity(node), "Severity type is SEVERITY_WARNING.");
> +
> +        //tests on the warningNode
> +        let warningNode = aOutputNode.querySelector(".webconsole-mixed-content-link");
> +        ok( (warningNode.value == "[Mixed Content]") , "Message text is accurate TEST1." );

Mochitests have a neat function you can use here:

is(warningNode.value, "[Mixed Content]", "Message text is accurate");

If the test fails, it tells you the expected value ("[Mixed Content]") and the value it actually got from warningNode.value. With ok() we get only a "test failed" without telling us which was the expected value and what it got from the warningNode.

@@ +43,5 @@
> +        let warningNode = aOutputNode.querySelector(".webconsole-mixed-content-link");
> +        ok( (warningNode.value == "[Mixed Content]") , "Message text is accurate TEST1." );
> +
> +        //This function is not necessary, should remove
> +        ok( function() {if( testLogEntry(warningNode, "[Mixed Content]") ) { return true; } else { return false } },

Agreed. This test is no longer needed. Please remove.

@@ +47,5 @@
> +        ok( function() {if( testLogEntry(warningNode, "[Mixed Content]") ) { return true; } else { return false } },
> +            "Message text is accurate TEST2.");
> +
> +        //will there be synchornization issues with this test, since ok() is called within the click event handler?
> +        testClickOpenNewTab(warningNode);

Yes, there's nothing that guarantees the test won't end before the new tab is open.

Try this trick:

let linkOpened = false;
let oldOpenUILinkIn = window.openUILinkIn;
window.openUILinkIn = function(aLink) {
  if (aLink == "http://...your link") {
    linkOpened = true;
  }
};

EventUtils.synthesizeMouse(warningNode, 2, 2, {});

ok(linkOpened, "clicking the warning node opens the desired page");

window.openUILinkIn = oldOpenUILinkIn;

finishTest();

I hope it works - didn't test.

Things to consider:

- network issues when running tests. We don't really need to open the actual page. Those parts of the browser are tested elsewhere.
- we don't even need to open the tab. openUILinkIn() is tested elsewhere.

Comment 35

[Mihai Sucan [:msucan]]

Updated link for license boilerplates:

http://www.mozilla.org/MPL/headers/

(just saw that the previous link is obsolete)

Comment 36

[Mihai Sucan [:msucan]]

Attachment: make HUDService-content send the new page location earlier (updated)

Rob: Tanvi is almost done with her work and this patch is needed to make sure the content window location is available as early as possible.

Asking for review so Tanvi won't wait for this patch to be able to land. Thanks!

Comment 37

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

Comment on attachment 638552 [details] [diff] [review]
Mixed Content Webconsole Patch - Ready for Review

>diff --git a/browser/devtools/webconsole/HUDService.jsm b/browser/devtools/webconsole/HUDService.jsm

>+    try{
>+      let ioServ = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);

You can use Services.io

>+    if(uriCreated) {

>+      if(contentWindowURIScheme == "https" && requestURIScheme != "https") {

Can you wrap these checks (and associated code) into an isMixedContentLoad (or somesuch) helper function? logNetActivity is getting rather unwieldy, I think that'd make it easier to read.

Similarly, you could have a makeMixedContentNode() helper that handled the mixed-content specific code (adding the label, etc.)

Comment 38

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Console Webconsole Patch

Thanks for the review!  I made all the changes that Mihai and Gavin suggested.  I am r? this to Gavin, since he has looked at the patch once before and I need a module owner/peer to review this for the webconsole.css changes.

Comment 39

[Tanvi Vyas[:tanvi]]

Attachment: Mochitest

I made most of the changes Mihai suggested for the mochitests.

(In reply to Mihai Sucan [:msucan] from comment #34)
#1
> @@ +29,5 @@
> > +  waitForSuccess (
> > +    {
> > +      name: "mixed content warning displayed successfully",
> > +      validatorFn: function() {
> > +        return ( aOutputNode.querySelector(".webconsole-mixed-content") && aOutputNode.querySelector(".webconsole-mixed-content-link") );
> 
> Are both querySelector() calls needed here?
Yes, both queryselector calls are necessary, since they are both needed in order for all the tests to succeed.

#2
> > +        //will there be synchornization issues with this test, since ok() is called within the click event handler?
> > +        testClickOpenNewTab(warningNode);
> 
> Yes, there's nothing that guarantees the test won't end before the new tab
> is open.
I tried following your proposal, but I'm not sure I understand what it's doing or why it works.  Perhaps we can discuss this more on IRC.  I have left the test case as is for now, and it's passing.  But you are right, there is no guarantee that the test won't complete before the new tab is opened.

Comment 40

[Tanvi Vyas[:tanvi]]

I've created a page that the webconsole will link to: https://developer.mozilla.org/en/Security/MixedContent.

Once we complete any remaining items for this patch, I'll work on updating the mdn page.

Thanks!

Comment 41

[Mihai Sucan [:msucan]]

Tanvi: thanks for your updates!

(In reply to Tanvi Vyas from comment #39)
> Created attachment 638970 [details] [diff] [review]
> Mochitest
> 
> I made most of the changes Mihai suggested for the mochitests.
> 
> (In reply to Mihai Sucan [:msucan] from comment #34)
> #1
> > @@ +29,5 @@
> > > +  waitForSuccess (
> > > +    {
> > > +      name: "mixed content warning displayed successfully",
> > > +      validatorFn: function() {
> > > +        return ( aOutputNode.querySelector(".webconsole-mixed-content") && aOutputNode.querySelector(".webconsole-mixed-content-link") );
> > 
> > Are both querySelector() calls needed here?
> Yes, both queryselector calls are necessary, since they are both needed in
> order for all the tests to succeed.

As far as I know both elements will show in the output at the same time, so querySelector() for only one class in the validatorFn() function would be sufficient.
 
> #2
> > > +        //will there be synchornization issues with this test, since ok() is called within the click event handler?
> > > +        testClickOpenNewTab(warningNode);
> > 
> > Yes, there's nothing that guarantees the test won't end before the new tab
> > is open.
> I tried following your proposal, but I'm not sure I understand what it's
> doing or why it works.  Perhaps we can discuss this more on IRC.  I have
> left the test case as is for now, and it's passing.  But you are right,
> there is no guarantee that the test won't complete before the new tab is
> opened.

What the the proposal does is it uses the knowledge of how we open the link (we call openUILinkIn()). We overwrite that function with our own function in the test and we check if it gets called properly, then we put back the original openUILinkIn() function. Also, we use the knowledge that synthesizeMouse() is sync (not async), so we do the click, we check the result, then we finish the test.

Sure, we can discuss this on IRC. Please ping me when you get a chance. Thanks!

Comment 42

[Tanvi Vyas[:tanvi]]

(In reply to Mihai Sucan [:msucan] from comment #41)
> > #2
> > > > +        //will there be synchornization issues with this test, since ok() is called within the click event handler?
> > > > +        testClickOpenNewTab(warningNode);
> > > 
> > > Yes, there's nothing that guarantees the test won't end before the new tab
> > > is open.
> > I tried following your proposal, but I'm not sure I understand what it's
> > doing or why it works.  Perhaps we can discuss this more on IRC.  I have
> > left the test case as is for now, and it's passing.  But you are right,
> > there is no guarantee that the test won't complete before the new tab is
> > opened.
> 
> What the the proposal does is it uses the knowledge of how we open the link
> (we call openUILinkIn()). We overwrite that function with our own function
> in the test and we check if it gets called properly, then we put back the
> original openUILinkIn() function. Also, we use the knowledge that
> synthesizeMouse() is sync (not async), so we do the click, we check the
> result, then we finish the test.
> 

Thanks Mihai!  This makes perfect sense now!  I'll update the patch and attach with an r? for final review.

Comment 43

[Tanvi Vyas[:tanvi]]

Attachment: Mochitest

Comment 44

[Tanvi Vyas[:tanvi]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6e8f3fcf2f93

Comment 45

[Tanvi Vyas[:tanvi]]

Attachment: Mochitest

Updated with the .html file.

Try push is now here: https://tbpl.mozilla.org/?tree=Try&rev=d5adc77a3e96

Comment 46

[Mihai Sucan [:msucan]]

Comment on attachment 639476 [details] [diff] [review]
Mochitest

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

Looks good to me! Thank you! r+ with a green try run.

Apologies for forgetting to review your previous test patch: somehow I completely forgot. Please ping me on IRC if I forget to address any review request - don't be shy. Thanks!

Comment 47

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

Comment on attachment 638968 [details] [diff] [review]
Mixed Console Webconsole Patch

>diff --git a/browser/devtools/webconsole/HUDService.jsm b/browser/devtools/webconsole/HUDService.jsm

>+const MIXED_CONTENT_LEARN_MORE = "https://developer.mozilla.org/en/Security/MixedContent";

Is there a bug that tracks proper l10n support for in-product MDN links? HUDService seems to use https://developer.mozilla.org/AppLinks/WebConsoleHelp, which looks promising, though I see that's not being used for CssHtmlTree.jsm.

>+  isMixedContentLoad: function HUD_isMixedContentLoad(request, contentLocation) {
>+    let requestURIObject = Services.io.newURI(request, null, null);
>+    let contentWindowURI = Services.io.newURI(contentLocation, null, null);

It's hard to tell from this patch (without being familiar with console implementation details) what the origins of these URL strings are. Do you need to worry about them being invalid URIs (i.e. newURI throwing)? Are they guaranteed to be absolute URIs?

>+  makeMixedContentNode: function HUD_makeMixedContentNode(linkNode) {

>+      aEvent.stopPropagation();
>+      }.bind(this)

nit: indentation looks odd

I assume you've tested that the styling looks OK on all three platforms.

r=me with those addressed.

Comment 48

[Mihai Sucan [:msucan]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #47)
> Comment on attachment 638968 [details] [diff] [review]
> Mixed Console Webconsole Patch
> 
> >diff --git a/browser/devtools/webconsole/HUDService.jsm b/browser/devtools/webconsole/HUDService.jsm
> 
> >+const MIXED_CONTENT_LEARN_MORE = "https://developer.mozilla.org/en/Security/MixedContent";
> 
> Is there a bug that tracks proper l10n support for in-product MDN links?
> HUDService seems to use
> https://developer.mozilla.org/AppLinks/WebConsoleHelp, which looks
> promising, though I see that's not being used for CssHtmlTree.jsm.

As far as I know, we have no such bug open. As discussed on IRC, Tanvi will file a follow-up bug.


> >+  isMixedContentLoad: function HUD_isMixedContentLoad(request, contentLocation) {
> >+    let requestURIObject = Services.io.newURI(request, null, null);
> >+    let contentWindowURI = Services.io.newURI(contentLocation, null, null);
> 
> It's hard to tell from this patch (without being familiar with console
> implementation details) what the origins of these URL strings are. Do you
> need to worry about them being invalid URIs (i.e. newURI throwing)? Are they
> guaranteed to be absolute URIs?

These *should* not cause newURI to throw. Both URIs come from either network logging code or from window.location objects and similar - where I always expect valid URIs. Still, as a safety, this could be in a try-catch.

Thank you for reviewing Tanvi's code!

Comment 49

[Tanvi Vyas[:tanvi]]

(In reply to:Gavin Sharp (use [e-mail] for email) from comment #47)
> 
> >+  isMixedContentLoad: function HUD_isMixedContentLoad(request, contentLocation) {
> >+    let requestURIObject = Services.io.newURI(request, null, null);
> >+    let contentWindowURI = Services.io.newURI(contentLocation, null, null);
> 
> It's hard to tell from this patch (without being familiar with console
> implementation details) what the origins of these URL strings are. Do you
> need to worry about them being invalid URIs (i.e. newURI throwing)? Are they
> guaranteed to be absolute URIs?
> 

request comes from networkInfo.httpActivity.log.entries[0].request.url.  And contentLocation comes from this.contentLocation, which is the location of the current tab.  I'm not sure if these are guaranteed to be absolute URIs.  In an earlier version of the patch, I had a try catch around this, so I can add that back in.  In the catch case, we do nothing and skip the mixed content code.

I'll attach an updated patch later today that addresses your comments.  Thanks!

Comment 50

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Webconsole Patch

Updated patch.  Thanks Gavin for your help!

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #47)
> Comment on attachment 638968 [details] [diff] [review]
> Mixed Console Webconsole Patch
> 
> >diff --git a/browser/devtools/webconsole/HUDService.jsm b/browser/devtools/webconsole/HUDService.jsm
> 
> >+const MIXED_CONTENT_LEARN_MORE = "https://developer.mozilla.org/en/Security/MixedContent";
> 
> Is there a bug that tracks proper l10n support for in-product MDN links?
> HUDService seems to use
> https://developer.mozilla.org/AppLinks/WebConsoleHelp, which looks
> promising, though I see that's not being used for CssHtmlTree.jsm.
> 

Filed bug 771641 for this issue.


> >+  isMixedContentLoad: function HUD_isMixedContentLoad(request, contentLocation) {
> >+    let requestURIObject = Services.io.newURI(request, null, null);
> >+    let contentWindowURI = Services.io.newURI(contentLocation, null, null);
> 
> It's hard to tell from this patch (without being familiar with console
> implementation details) what the origins of these URL strings are. Do you
> need to worry about them being invalid URIs (i.e. newURI throwing)? Are they
> guaranteed to be absolute URIs?
> 
Added a try catch.


> >+  makeMixedContentNode: function HUD_makeMixedContentNode(linkNode) {
> 
> >+      aEvent.stopPropagation();
> >+      }.bind(this)
> 
> nit: indentation looks odd
> 
Fixed.

> I assume you've tested that the styling looks OK on all three platforms.
> 
Testing in linux and windows now, since I haven't yet.

> r=me with those addressed.
Carrying over r+.

Comment 51

[Tanvi Vyas[:tanvi]]

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

Comment 52

[Tanvi Vyas[:tanvi]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #47)
> I assume you've tested that the styling looks OK on all three platforms.
> 

I will attach screenshots from all 3 platforms.

Comment 53

[Tanvi Vyas[:tanvi]]

Attachment: Mac Mixed Content Warning

Comment 54

[Tanvi Vyas[:tanvi]]

Attachment: Windows Mixed Content Warning

Comment 55

[Tanvi Vyas[:tanvi]]

Attachment: Ubuntu Mixed Contet Image

Rob - as soon as you review Mihai's patch, we can land this.  Thanks!

Comment 56

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 638665 [details] [diff] [review]
make HUDService-content send the new page location earlier (updated)

yup.

Comment 57

[Mihai Sucan [:msucan]]

Comment on attachment 638665 [details] [diff] [review]
make HUDService-content send the new page location earlier (updated)

Landed:
https://hg.mozilla.org/integration/fx-team/rev/395e4eaaf47b

Thank you Rob!

Comment 58

[Mihai Sucan [:msucan]]

Landed Tanvi's patches as well:
https://hg.mozilla.org/integration/fx-team/rev/b699ded05a12

(folded them into one, for easier management)

Thank you Tanvi for your work and patience!

Comment 59

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/395e4eaaf47b
https://hg.mozilla.org/mozilla-central/rev/b699ded05a12

Comment 60

[Rob Campbell [:rc] (:robcee)]

woo! \o/ :D