Last Comment Bug 837351 - When mixed content is blocked, show the blocked URL in the Error Console and WebConsole
: When mixed content is blocked, show the blocked URL in the Error Console and ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla23
Assigned To: Garrett Robinson [:grobinson]
:
Mentors:
Depends on:
Blocks: MixedContentBlocker 863874 713980 863878 865344
  Show dependency treegraph
 
Reported: 2013-02-01 18:54 PST by Jesse Ruderman
Modified: 2013-07-18 21:09 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Log to error console and web console when mixed content is blocked v1 (6.96 KB, patch)
2013-02-06 13:19 PST, Tanvi Vyas[:tanvi]
mihai.sucan: feedback+
bugzilla: feedback+
Details | Diff | Review
Log to error console and web console when mixed content is blocked v2 (20.73 KB, patch)
2013-02-12 18:10 PST, Tanvi Vyas[:tanvi]
no flags Details | Diff | Review
Log to error console and web console when mixed content is blocked v3 (21.88 KB, patch)
2013-02-15 18:10 PST, Tanvi Vyas[:tanvi]
no flags Details | Diff | Review
Log to error console and web console when mixed content is blocked v4 (22.38 KB, patch)
2013-02-16 19:44 PST, Tanvi Vyas[:tanvi]
no flags Details | Diff | Review
Different Direction, Patch 1 (16.31 KB, patch)
2013-04-16 15:48 PDT, Garrett Robinson [:grobinson]
ian.melven: feedback+
Details | Diff | Review
Patch 2 (22.13 KB, patch)
2013-04-17 11:07 PDT, Garrett Robinson [:grobinson]
no flags Details | Diff | Review
Patch 3 (23.40 KB, patch)
2013-04-17 11:30 PDT, Garrett Robinson [:grobinson]
no flags Details | Diff | Review
Test 1 (3.64 KB, patch)
2013-04-17 16:38 PDT, Garrett Robinson [:grobinson]
no flags Details | Diff | Review
Test 2 (3.74 KB, patch)
2013-04-17 17:53 PDT, Garrett Robinson [:grobinson]
no flags Details | Diff | Review
Patch 4 - Content Changes (8.25 KB, patch)
2013-04-18 14:50 PDT, Garrett Robinson [:grobinson]
bugs: review+
Details | Diff | Review
Patch 4 - Devtools changes (19.39 KB, patch)
2013-04-18 14:54 PDT, Garrett Robinson [:grobinson]
mihai.sucan: review+
Details | Diff | Review
Patch 5 - Content Changes (8.12 KB, patch)
2013-04-19 09:57 PDT, Garrett Robinson [:grobinson]
garrett.f.robinson+mozilla: review+
Details | Diff | Review

Description Jesse Ruderman 2013-02-01 18:54:02 PST

    
Comment 1 Tanvi Vyas[:tanvi] 2013-02-06 13:11:02 PST
About to add a POC patch.  It updates the error console and the webconsole with the type of mixed content that was blocked and the url it came from.  However, there are some known bugs:

* I am using nsIScriptError, when this isn't a script error.  Hence this shows up in the JS panel of webconsole instead of the net panel.  Hopefully ddahl and msucan can point me at how to change this so that it shows up in the net panel (are there any existing interfaces I can use, or do I need to create a new nsIConsoleMessage interface?).

* The errors are shown twice instead of once.  This is because shouldLoad appears to be called twice.  This may be for a reason that I am unaware of, or it may be a bug in nsMixedContentBlocker.  I will look into this.
Comment 2 Tanvi Vyas[:tanvi] 2013-02-06 13:19:26 PST
Created attachment 710937 [details] [diff] [review]
Log to error console and web console when mixed content is blocked v1

* Actually, the error is is showing up twice in the error console but only once in the webconsole (which is good).  Mihai, do you know why it is duplicated in one but not the other?

* The strings need to be defined and moved out of this patch so that they can be localized.  Where should I define them?
Comment 3 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-02-06 14:07:43 PST
IMO, the error should not be sent to the error console if it is sent to the web console. The error console should be reserved for things we can't send to a web console because we don't know which tab is affected. Sending to both places causes too much noise and makes things confusing--e.g. were there two instances of the problem, or just one?
Comment 4 David Dahl :ddahl 2013-02-06 14:17:24 PST
Comment on attachment 710937 [details] [diff] [review]
Log to error console and web console when mixed content is blocked v1

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

Looks good. I think the approach is good - I imagine Mihai might have some better ideas than I had earlier. Perhaps you can write a 'nsIMixedContentError' object that descends from nsIScriptError that has just the properties you need and retains the initWithWindowId() method? Maybe that is overkill? Do you have a mixed content test that can be copied with a consoleListener thrown in to check for these errors?
Comment 5 Jesse Ruderman 2013-02-06 15:07:20 PST
> IMO, the error should not be sent to the error console if it is sent to the web 
> console.

That would be a departure from our current behavior.  I think we should continue reporting serious brokenness to the Error Console until we remove the Error Console.
Comment 6 David Dahl :ddahl 2013-02-07 10:52:21 PST
(In reply to Tanvi Vyas [:tanvi] from comment #2)
> Created attachment 710937 [details] [diff] [review]

> * The strings need to be defined and moved out of this patch so that they
> can be localized.  Where should I define them?
A quick scan in mxr reveals string properties being loaded like so:

nsContentUtils::GetLocalizedString(nsContentUtils::eFORMS_PROPERTIES, "Submit", defaultValue);

https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#3855
Comment 7 Mihai Sucan [:msucan] 2013-02-07 13:00:47 PST
(In reply to Tanvi Vyas [:tanvi] from comment #1)
> About to add a POC patch.  It updates the error console and the webconsole
> with the type of mixed content that was blocked and the url it came from. 
> However, there are some known bugs:
> 
> * I am using nsIScriptError, when this isn't a script error.  Hence this
> shows up in the JS panel of webconsole instead of the net panel.  Hopefully
> ddahl and msucan can point me at how to change this so that it shows up in
> the net panel (are there any existing interfaces I can use, or do I need to
> create a new nsIConsoleMessage interface?).

Here's an overview of what you need to do:

To have this message shown in the net panel you need to change dbg-webconsole-actors.js (find it with |hg locate|). This is the web console server-side stuff (no UI bits here). Look at WCA_onPageError() which handles nsIScriptErrors. Here you want to filter your messages - based on category and URL. You want to change the error message category from DOM Core to something specific to this new kind of messages - eg. "Mixed content blocker". For those messages that come from this new category, you need to find the NetworkEventActor that belongs to the same URL. These actors live in an ActorPool, see |this._actorPool| in WebConsoleActor, also see WCA_onNetworkEvent(). Once you found the NEA you want, you can the mixed content blocker error information. Change the NetworkEventActor implementation to properly record and transmit this new info to the Web Console client.

On the client-side you need to edit webconsole.js. See WCF_handleNetworkEvent() and WCF_logNetMessage(). Given proper transmission of new information, the new mixed content blocker error should end up in the |networkInfo| object for the given network event actor that exists and is associated with each network message in the web console output. This |networkInfo| object is sent to the NetworkPanel - see WCF_openNetworkPanel().

In NetworkPanel.jsm the |networkInfo| object is referred to as the |httpActivity| object (legacy code). The place that is of interest to you is the NP_update() function. Once you have the mixed content blocker information in httpActivity you can go ahead and update the netpanel UI as you see fit - use this.document to refer to the document / page where the UI is loaded. Load NetworkPanel.xhtml to see which document is displayed and how you can change it to suit your needs. At this point, you just need to do some DOM updates to display what you want.

The hard part is in the web console actors: iterating over _actorPool is not something I'd advise. You need to change onNetworkEvent such that any new network event actor is added to a separate pool that you can iterate. See the WebConsoleActor constructor for how you can add a new pool.

Another hard part is matching your message to the network event - I suggested above that you match based on URL. That's most likely not ideal. I'm not sure what are your requirements.

Maybe you don't need to do this? Just displaying the error in the web console output is sufficient?

> * The errors are shown twice instead of once.  This is because shouldLoad
> appears to be called twice.  This may be for a reason that I am unaware of,
> or it may be a bug in nsMixedContentBlocker.  I will look into this.

I'm not familiar with that code so I don't know what to suggest.
Comment 8 Mihai Sucan [:msucan] 2013-02-07 13:07:33 PST
(In reply to Tanvi Vyas [:tanvi] from comment #2)
> Created attachment 710937 [details] [diff] [review]
> Log to error console and web console when mixed content is blocked v1
> 
> * Actually, the error is is showing up twice in the error console but only
> once in the webconsole (which is good).  Mihai, do you know why it is
> duplicated in one but not the other?

This is probably because the web console coalesces repeated messages.

(In reply to Brian Smith (:bsmith) from comment #3)
> IMO, the error should not be sent to the error console if it is sent to the
> web console. The error console should be reserved for things we can't send
> to a web console because we don't know which tab is affected.

I consider the error console as a global console: you see everything. We'll actually add a Global Console that will display all messages from all over the place, see bug 587757. Nonetheless, I see your point and we should be able to filter messages that belong to tabs, and only see global-related messages.

I agree with comment #5.
Comment 9 Mihai Sucan [:msucan] 2013-02-07 13:21:53 PST
(In reply to David Dahl :ddahl from comment #4)
> Comment on attachment 710937 [details] [diff] [review]
> Log to error console and web console when mixed content is blocked v1
> 
> Review of attachment 710937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I think the approach is good - I imagine Mihai might have some
> better ideas than I had earlier. Perhaps you can write a
> 'nsIMixedContentError' object that descends from nsIScriptError that has
> just the properties you need and retains the initWithWindowId() method?
> Maybe that is overkill? Do you have a mixed content test that can be copied
> with a consoleListener thrown in to check for these errors?

I like the idea.

nsIMixedContentError sounds good to me, but it should probably inherit from nsIConsoleMessage. initWithWindowId() should become shared code.

The nsIConsoleListener used by the web console can be updated to use this new message.
Comment 10 Mihai Sucan [:msucan] 2013-02-07 13:24:26 PST
Comment on attachment 710937 [details] [diff] [review]
Log to error console and web console when mixed content is blocked v1

Looks good to me. It really depends on what you want to do, how far you want to take things here. You can do the netpanel change and you can do the nsIMixedContentError thing suggested by ddahl.

Please let me know if you have any questions.
Comment 11 Tanvi Vyas[:tanvi] 2013-02-07 17:22:18 PST
Hi Mihai.  Thank you so much for your help!

(In reply to Mihai Sucan [:msucan] from comment #7)
> The hard part is in the web console actors: iterating over _actorPool is not
> something I'd advise. You need to change onNetworkEvent such that any new
> network event actor is added to a separate pool that you can iterate. See
> the WebConsoleActor constructor for how you can add a new pool.
> 

This bugs is to add network requests that were blocked to the Webconsole in the Net panel.  So I'm not sure comment 7 is the right approach,since I wont' actually get a network event.

Hence, I think I should try the nsIMixedContentError suggestion instead.  I will start working on that.

(In reply to Mihai Sucan [:msucan] from comment #8)
> > * Actually, the error is is showing up twice in the error console but only
> > once in the webconsole (which is good).  Mihai, do you know why it is
> > duplicated in one but not the other?
> 
> This is probably because the web console coalesces repeated messages.
> 

Turns out we are getting two messages because of a bug in the content policies where shouldLoad() is called twice, and hence the log function is called twice.  I filed bug 839235 for this.
Comment 12 Mihai Sucan [:msucan] 2013-02-08 11:35:11 PST
(In reply to Tanvi Vyas [:tanvi] from comment #11)
> Hi Mihai.  Thank you so much for your help!
> 
> (In reply to Mihai Sucan [:msucan] from comment #7)
> > The hard part is in the web console actors: iterating over _actorPool is not
> > something I'd advise. You need to change onNetworkEvent such that any new
> > network event actor is added to a separate pool that you can iterate. See
> > the WebConsoleActor constructor for how you can add a new pool.
> > 
> 
> This bugs is to add network requests that were blocked to the Webconsole in
> the Net panel.  So I'm not sure comment 7 is the right approach,since I
> wont' actually get a network event.
> 
> Hence, I think I should try the nsIMixedContentError suggestion instead.  I
> will start working on that.

Sounds good then. Much simpler. You'll need to look into WebConsoleUtils.jsm, search for PageErrorListener, WCA_onPageError() in dbg-webconsole-actors.js, and WCF_reportPageError() in webconsole.js.
Comment 13 Tanvi Vyas[:tanvi] 2013-02-12 18:10:06 PST
Created attachment 713234 [details] [diff] [review]
Log to error console and web console when mixed content is blocked v2

WIP patch that adds an nsIMixedContentError interface.  But it doesn't quite build yet.  I seem to be missing some files where I need to include the new interface.

This patch includes:
content/base/src/nsMixedContentBlocker.cpp
content/base/src/nsMixedContentBlocker.h
js/xpconnect/idl/Makefile.in
js/xpconnect/idl/nsIMixedContentError.idl
js/xpconnect/src/XPCModule.h
js/xpconnect/src/nsMixedContentError.cpp
js/xpconnect/src/xpcprivate.h

Mihai, do you know what I am a missing?  Or am I doing something wrong in xpcprivate.h?  I am getting the following errors:
http://www.pastebin.mozilla.org/2134947
Comment 14 Jesse Ruderman 2013-02-13 16:08:53 PST
> I think we should continue reporting serious brokenness to the Error Console until 
> we remove the Error Console.

That would be bug 593540, for anyone who is still curious.
Comment 15 [:mmc] Monica Chew (no longer reading bugmail) 2013-02-13 21:41:24 PST
From the compile output it looks like you are missing some magic incantation of  NS_DECL_ISUPPORTS
(all the way down the chain)
NS_DECL_NSIMIXEDCONTENTERROR

which should appear in nsMixedContentError.h. These appear (by experimentation, not by documentation) to provide the boilerplate XPCOM functions for AddRef and friends, which are pure virtual right now. There is no declaration for nsMixedContentError, generated or otherwise, so that's what's causing "error: unknown type name 'nsMixedContentError'" and friends.
Comment 16 Mihai Sucan [:msucan] 2013-02-15 10:02:51 PST
Comment on attachment 713234 [details] [diff] [review]
Log to error console and web console when mixed content is blocked v2

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

I'm not strongly skilled in C++ and Gecko codebase. I believe you should ask for review/feedback here from a module owner.

What do you build with? gcc or clang? As far as I know, clang has much more helpful error messages. Here is what I get with clang 3.1:

http://pastebin.mozilla.org/2142280

Looking into the errors I think you should try to tackle them one by one. For example, you need to implement the virtual methods in your class and get the NS_GENERIC_FACTORY_CONSTRUCTOR right. See how nsIScriptError is implemented.

I hope this helps.
Comment 17 Tanvi Vyas[:tanvi] 2013-02-15 18:10:38 PST
Created attachment 714705 [details] [diff] [review]
Log to error console and web console when mixed content is blocked v3

Thanks Monica and Mihai for your tips.  Here is an updated patch, but it still doesn't quite compile - http://pastebin.mozilla.org/2143457

Will continue trying to figure out what is missing (or extra) in this patch.
Comment 18 Mihai Sucan [:msucan] 2013-02-16 02:47:33 PST
Tanvi: take a look at https://wiki.mozilla.org/Modules/All#XPConnect << select someone from here to ask for feedback. Your patch will need to be reviewed by an XPConnect peer anyway.
Comment 19 Tanvi Vyas[:tanvi] 2013-02-16 19:44:11 PST
Created attachment 714861 [details] [diff] [review]
Log to error console and web console when mixed content is blocked v4

I forgot to add nsMixedContentError.cpp to the Makefile (had only added the idl).  Thanks bsmith for the tip!  This is a WIP patch that shows the errors in the error console but not the webconsole yet.
Comment 20 Garrett Robinson [:grobinson] 2013-04-16 15:48:48 PDT
Created attachment 738236 [details] [diff] [review]
Different Direction, Patch 1

Writing a new C++ class for Security Errors is nontrivial, and it seems like an overhaul of Gecko logging might be more desired rather than continuing to repurpose (or duplicate the functionality of) nsIScriptError.

In the meantime, this bug needs to be resolved becuase of frequent user complaints that they can't figure out what is being blocked by the mixed content blocker in Nightly.

This patch uses commonly used nsContentUtils::ReportToConsole mechanism and tries to clean up/separate security-specific elements of the code. It adds a specific "Security" category to the web console error messages.
Comment 21 Ian Melven :imelven 2013-04-16 16:45:15 PDT
Comment on attachment 738236 [details] [diff] [review]
Different Direction, Patch 1

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

Driveby feedback !

I guess repeated node is an existing web console thing but it wasn't immediately clear what repeatNode meant.

::: browser/devtools/webconsole/webconsole.js
@@ +121,5 @@
>    [ "exception",  "jswarn",     null,   null,          ],  // JS
>    [ "error",      "warn",       "info", "log",         ],  // Web Developer
>    [ null,         null,         null,   null,          ],  // Input
>    [ null,         null,         null,   null,          ],  // Output
> +  [ "error",      "warn",       "info", "log",          ],  // Security

nit: should align ] with above

@@ +1949,5 @@
>               aNode.classList.contains("webconsole-msg-network")) {
>        delete this._networkRequests[aNode._connectionId];
>        this._releaseObject(aNode._connectionId);
>      }
>      else if (aNode.classList.contains("webconsole-msg-inspector")) {

do you need a delete here for when classList.contains("webconsole-msg-security") ?

@@ +4233,5 @@
>      switch (aScriptError.category) {
>        case "CSS Parser":
>        case "CSS Loader":
>          return CATEGORY_CSS;
> +      

nit: whitespace

::: dom/locales/en-US/chrome/security/security.properties
@@ +1,2 @@
> +BlockMixedDisplayContent = Mixed Display Content at "%1$S" is blocked
> +BlockMixedActiveContent = Mixed Active Content at "%1$S" is blocked

I think we could be more explicative here, or perhaps " "%1$S" was blocked from loading due to being mixed [active|display] content."
Comment 22 Garrett Robinson [:grobinson] 2013-04-16 23:29:36 PDT
My try run uncovered a related webconsole browser mochitest failure, which I will fix in the morning along with imelven's feedback. Mihai, you might want to wait to review until I've made these obvious fixes. Thanks, and see you in the morning!
Comment 23 Garrett Robinson [:grobinson] 2013-04-17 11:07:09 PDT
Created attachment 738629 [details] [diff] [review]
Patch 2
Comment 24 Garrett Robinson [:grobinson] 2013-04-17 11:09:20 PDT
Comment on attachment 738629 [details] [diff] [review]
Patch 2

Hit return early by accident. This patch incorporates imelven's feedback and discussion from this morning's meeting. We log blocked mixed content resources to a new, dedicated security pane in the Web Console panel. This patch also fixes the broken test from yesterday's try run.
Comment 25 Garrett Robinson [:grobinson] 2013-04-17 11:30:37 PDT
Created attachment 738646 [details] [diff] [review]
Patch 3

Just noticed there are different webconsole.css for each platform. This patch adds the security pane styles to all of them.
Comment 26 Garrett Robinson [:grobinson] 2013-04-17 16:29:17 PDT
Try run: https://tbpl.mozilla.org/?tree=Try&rev=2c01711a9e93
Comment 27 Garrett Robinson [:grobinson] 2013-04-17 16:38:33 PDT
Created attachment 738807 [details] [diff] [review]
Test 1

Mochitest to test if 1) the security pane exists in the Web Console, and 2) warnings about blocked mixed content are logged to it, using the same test as test-bug-737873-mixedcontent.html.

This test doesn't change the pref values like browser_webconsole_bug_737873_mixedcontent.js because active mixed content blocking is now on by default.
Comment 28 Garrett Robinson [:grobinson] 2013-04-17 17:53:26 PDT
Created attachment 738833 [details] [diff] [review]
Test 2

Tanvi advised me to explicitly set the pref, and not to rely on it being on by default. This new test achieves that using SpecialPowers.pushPrefEnv.
Comment 29 Mihai Sucan [:msucan] 2013-04-18 05:52:25 PDT
Comment on attachment 738646 [details] [diff] [review]
Patch 3

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

Thank you for these patches!

Unfortunately, both patches fail to apply for me - I get multiple rejected hunks in different files. Please rebase on top of the latest fx-team repo.

I looked through the web console changes and they look fine. Do you have a green try push for this patch and for the test?

Please note that you will need to ask for review from someone else for the changes in content/base - I can only review the devtools-related changes.
Comment 30 Mihai Sucan [:msucan] 2013-04-18 05:52:43 PDT
Comment on attachment 738833 [details] [diff] [review]
Test 2

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

Test looks good as well, just some minor comments.

::: browser/devtools/webconsole/test/Makefile.in
@@ +120,5 @@
>  	browser_console_variables_view.js \
>  	browser_console_variables_view_while_debugging.js \
>  	browser_console.js \
>  	head.js \
> +	browser_webconsole_bug_837351_securityerrors.js \

Please put this before head.js.

::: browser/devtools/webconsole/test/browser_webconsole_bug_837351_securityerrors.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const TEST_URI = "https://example.com/browser/browser/devtools/webconsole/test/test-bug-837351-security-errors.html";
> +
> +String.prototype.contains = function(it) { return this.indexOf(it) != -1; };

This kind of changes to the String prototype are not recommended because they don't affect only this test environment. If I am not mistaken this changes things for other tests as well.

@@ +14,5 @@
> +      let button = hud.ui.rootElement.querySelector(".webconsole-filter-button[category=\"security\"]");
> +      ok(button, "Found security button in the web console");
> +      var outputNode = hud.outputNode;
> +
> +      waitForSuccess({

Instead of waitForSuccess() please use waitForMessages(). For how to use it see:

http://mxr.mozilla.org/mozilla-central/ident?i=waitForMessages&tree=mozilla-central&filter=webconsole

(you can find where it is defined and where other tests use this new function)
Comment 31 Garrett Robinson [:grobinson] 2013-04-18 14:50:34 PDT
Created attachment 739276 [details] [diff] [review]
Patch 4 - Content Changes

I've split this patch into two parts so they can each be reviewed by the appropriate individual.

This patch adds a function to log blocked mixed content to the Error/Web console using the standard nsContentUtils::ReportToConsole mechanism. It adds a localizable properties file specifically for security-related messages (unlike previous similar bugs for reporting CSP errors, which reused dom.properties).
Comment 32 Garrett Robinson [:grobinson] 2013-04-18 14:54:35 PDT
Created attachment 739279 [details] [diff] [review]
Patch 4 - Devtools changes

This patch adds a category to the web console code for recognizing/filtering security errors, and it adds a filter button the Web Console specifically for Security messages.

The mochitest is included here because it lives with the rest of the webconsole tests. Note that it will not pass unless "Patch 4 - Content Changes" is also applied, since the test is based on checking that we properly log to the web console when mixed content is blocked.
Comment 33 Garrett Robinson [:grobinson] 2013-04-18 14:56:49 PDT
Try run: https://tbpl.mozilla.org/?tree=Try&rev=a5831ea522fd

The last try run was green, so I expect this one will be as well because the changes from Mihai's review were minimal.
Comment 34 Tanvi Vyas[:tanvi] 2013-04-18 15:08:57 PDT
Comment on attachment 739276 [details] [diff] [review]
Patch 4 - Content Changes

Changing from Kyle to Olli since Olli has reviewed the mixed content code.
Comment 35 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2013-04-19 05:20:02 PDT
Comment on attachment 739276 [details] [diff] [review]
Patch 4 - Content Changes


>+void
>+LogBlockingMixedContent(MixedContentTypes classification,
>+                        nsIURI* aContentLocation,
>+                        nsIDocument* aRootDoc)
>+{
>+  nsAutoCString errorMsgName;
>+  if (classification == eMixedDisplay) {
>+    errorMsgName = "BlockMixedDisplayContent";
>+  } else {
>+    errorMsgName = "BlockMixedActiveContent";
>+  }
>+
>+  nsAutoCString locationSpec;
>+  aContentLocation->GetSpec(locationSpec);
>+  nsAutoString locationSpecUTF16 = NS_ConvertUTF8toUTF16(locationSpec);
NS_ConvertUTF8toUTF16 locationSpecUTF16(locationSpec); should work

>+  const PRUnichar *strings[] = { locationSpecUTF16.get() };
const PRUnichar *

>+  nsContentUtils::ReportToConsole(nsIScriptError::errorFlag,
>+                                  "Mixed Content Blocker",
>+                                  aRootDoc,
>+                                  nsContentUtils::eSECURITY_PROPERTIES,
>+                                  errorMsgName.get(),
I'd write this
classification == eMixedDisplay ? "BlockMixedDisplayContent" : "BlockMixedActiveContent"
and drop errorMsgName
Comment 36 Garrett Robinson [:grobinson] 2013-04-19 09:57:00 PDT
Created attachment 739650 [details] [diff] [review]
Patch 5 - Content Changes

Implement Ollie's suggestions from previous review. Tests pass.
Comment 37 Garrett Robinson [:grobinson] 2013-04-19 11:53:55 PDT
Comment on attachment 739650 [details] [diff] [review]
Patch 5 - Content Changes

Carry over r+ from smaug's last review
Comment 38 Mihai Sucan [:msucan] 2013-04-22 09:26:07 PDT
Comment on attachment 739279 [details] [diff] [review]
Patch 4 - Devtools changes

Thanks for the updated patch! This looks ready to land.
Comment 41 Francesco Lodolo [:flod] - OFFLINE Jun 26-Jul 3 2013-04-23 23:54:43 PDT
What exactly is "%1$S" in those two strings? I think you need to add a localization comment to security.properties, if you prefer I can open a new bug depending on this one.
Comment 42 Garrett Robinson [:grobinson] 2013-04-24 09:17:51 PDT
"%1$S" is the URI of the resource that was blocked. If you open a new bug I would be happy to add a localization comment.

Note You need to log in before you can comment on or make changes to this bug.