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.

* 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?

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 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?

> 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.

(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

(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.

(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.

(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 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.

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.

(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.

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

> 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.

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 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.

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.

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.

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.

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 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."

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 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.

Just noticed there are different webconsole.css for each platform. This patch adds the security pane styles to all of them.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2c01711a9e93

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.

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 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 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)

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).

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.

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 on attachment 739276 [details] [diff] [review] Patch 4 - Content Changes Changing from Kyle to Olli since Olli has reviewed the mixed content code.

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

Implement Ollie's suggestions from previous review. Tests pass.

Comment on attachment 739650 [details] [diff] [review] Patch 5 - Content Changes Carry over r+ from smaug's last review

Comment on attachment 739279 [details] [diff] [review] Patch 4 - Devtools changes Thanks for the updated patch! This looks ready to land.

Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/cd89b7d8dbf0 http://hg.mozilla.org/integration/mozilla-inbound/rev/af21a5a0956e

https://hg.mozilla.org/mozilla-central/rev/cd89b7d8dbf0 https://hg.mozilla.org/mozilla-central/rev/af21a5a0956e

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.

"%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.