846918 - Report invalid strict-transport-security headers to the web console

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Comment 1

[Jason Duell]

Yeah, see bug 688345 too.  Our reporting for this stuff sucks right now.

Comment 2

[Ivan Alagenchev :ialagenchev]

Attachment: Adding a work in progress for grobinson to review - test files.

Comment 3

[Ivan Alagenchev :ialagenchev]

Attachment: Adding a work in progress for grobinson to review.

Comment 4

[Ivan Alagenchev :ialagenchev]

Attachment: Adding a work in progress for grobinson to review -- Tests.

Comment 5

[Ivan Alagenchev :ialagenchev]

Attachment: Adding a patch for Garrett to review before I submit an official patch

Comment 6

[Garrett Robinson [:grobinson]]

Comment on attachment 762329 [details] [diff] [review]
Adding a patch for Garrett to review before I submit an official patch

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

Nice work, Ivan! I was pleased to see your new test works, and that you successfully replaced the functionality from 821877 with your more generic code so that test passes as well.

There are numerous style nits - please make sure you code conforms to the style guide. IIRC you are a Vim user. I recommend adding the following to your .vimrc:

" Mozilla Coding Style
autocmd BufNewFile,BufRead /path/to/mozilla-central/* set ts=2 sw=2 et tw=78

" Highlight trailing whitespace (call after loading colorscheme)
highlight ExtraWhitespace ctermbg=red guibg=red
match ExtraWhitespace /\s\+$/

Otherwise, my two big notes are:

1. You have a repeated chunk of code that you use whenever you enqueue a message. Abstract this into a function - it should take one line to enqueue an error message.
2. You might *not* want to reuse nsContentUtils::ReportToConsole, especially since you're already passing around nsIScriptErrors and it seems wasteful to extract the information from the one you've stored in the queue, only to pass it in parameters to ReportToConsole which just builds another one.

Overall, good work! It would be excellent if you have time to put some notes about what you've learned about the document lifecycle, etc. up on the wiki, even if isn't complete or particularly polished.

::: browser/devtools/webconsole/test/browser_webconsole_bug_846918_hsts_invalid-headers.js
@@ +1,3 @@
> +/* Tests that errors about invalid HSTS security headers are logged
> + *  to the web console */
> + 

Trailing whitespace

@@ +5,5 @@
> +  * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const TEST_URI = "https://example.com/browser/browser/devtools/webconsole/test/test-bug-846918-hsts-invalid-headers.html";
> +const HSTS_INVALID_HEADER_MSG = "This site specified an invalid Strict-Transport-Security header. Please use Strict-Transport-Security headers with HSTS spec compliant syntax instead.";
> + 

Trailing whitespace

@@ +11,5 @@
> + {
> +   addTab(TEST_URI);
> +   browser.addEventListener("load", function onLoad(aEvent) {
> +     browser.removeEventListener(aEvent.type, onLoad, true);
> +    openConsole(null, function testHSTSErrorLogged (hud) {

Use consistent indentation style

::: content/base/src/nsDocument.cpp
@@ +2597,3 @@
>    // If the old header is present, warn that it will be deprecated.
>    if (!cspOldHeaderValue.IsEmpty() || !cspOldROHeaderValue.IsEmpty()) {
> +    

Whitespace... please make sure all code has no trailing whitespace, is properly indented, and otherwise confirms to the Mozilla Style Guide: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

@@ +2632,5 @@
> +        
> +        if(!NS_FAILED(rv)) {
> +          internalChannel->AddSecurityMessage(error);
> +        }
> +      }

The two above blocks of code are almost identical. You should DRY this out by making a function to encapsulate the shared logic.

@@ +2687,5 @@
> +            internalChannel->AddSecurityMessage(error);
> +          }
> +        }
> +      }
> +

Same here. Any time you are copy and pasting code you need to stop and reevaluate your design.

@@ +4289,5 @@
>    // Now that we know what our window is, we can flush the CSP errors to the
>    // Web Console.
> +  if(window) {
> +    nsCOMPtr<nsIChannel> channel = GetChannel();
> +    nsCOMPtr<nsIHttpChannelInternal> httpChannel = do_QueryInterface(channel);

I would advocate trying to maintain consistent variable naming whenever possible. In InitCSP, you called this object internalChannel.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +19,5 @@
>  #include "nsIApplicationCacheChannel.h"
>  #include "nsILoadContext.h"
>  #include "nsEscape.h"
>  #include "nsStreamListenerWrapper.h"
> +#include "nsContentUtils.h"

Other reviewers might give you a hard time about importing this here because it's so massive. Leave it for now, but you might want to import it (and define the functions that need it) in the header instead.

@@ +1321,5 @@
> +    char *category;
> +    error->GetCategory(&category);
> +    
> +    nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,category,
> +        aDocument, nsContentUtils::eSECURITY_PROPERTIES,errorMessage);

This seems a bit wasteful, since you've already created nsIScriptError objects and put them in the queue. You're pulling parameters out of nsIScriptError's and recreating them inside nsContentUtils::ReportToConsole. In an ideal world, here you would just set the innerWindowID on the nsIScriptError (the only piece of information you didn't know, and needed), and then send them to the console service.

It seems there are two tasks nsContentUtils::ReportToConsole is performing for you here.

1. Get the current inner window ID and set it on the nsIScriptError.
2. Look up the correct localized string to use.

Both tasks are very easy (see the nsContentUtils::ReportToConsole code). I recommend you look in there and see if you can pull out what you need to avoid recreating the data structure.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1168,5 @@
> +                if (NS_FAILED(rv)) {
> +                    return NS_OK;
> +                }
> +
> +        AddSecurityMessage(error);

DRY this out.

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +54,5 @@
>      /**
>       * Get the major/minor version numbers for the response
>       */
>      void getResponseVersion(out unsigned long major, out unsigned long minor);
> +    

Comment these, explaining their purpose and rationale for locating them here. idl's are traditionally very well-commented.

Comment 7

[Ivan Alagenchev :ialagenchev]

After several discussions on irc with grobinson and on #devtools, we've decided to add our own struct to hold the message information, so that we won't have to incur the overhead of creating and destructing that nsIScriptError objects. The new idl will be put in xpcom/base, so that it's easier for netwerk and nsDocument to access it.

Comment 8

[Ivan Alagenchev :ialagenchev]

Attachment: newly proposed patch

Comment 9

[Ivan Alagenchev :ialagenchev]

Comment on attachment 768095 [details] [diff] [review]
newly proposed patch

In this patch, we moved away from using nsIScriptErrors to store the security messages.We now use the new nsISecurityConsoleMessage for that purpose. I also removed the reference to nsIDocument inside the http channel and now the document is responsible for dispatching the messages to its console.

Comment 10

[Garrett Robinson [:grobinson]]

Comment on attachment 768095 [details] [diff] [review]
newly proposed patch

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

Looks good to me. Submit this for review modulo the fixes I suggest below. If it doesn't say "fix", consider it but don't let it delay you from submitting this for review. Have you pushed this to try?

::: browser/devtools/webconsole/test/browser_webconsole_bug_846918_hsts_invalid-headers.js
@@ +15,5 @@
> +    openConsole(null, function testHSTSErrorLogged (hud) {
> +      waitForMessages({
> +        webconsole: hud,
> +        messages: [
> +      {

Fix: Indentation

::: content/base/src/nsDocument.cpp
@@ +2429,5 @@
> +    char *category = ToNewUTF8String(categoryString);
> +
> +    nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
> +        category, this, nsContentUtils::eSECURITY_PROPERTIES,
> +        messageTagString);

A thought: this could be more generic, and hence more reusable, if the type of message (error/warning) and the source of the message were also stored in the nsISecurityConsoleMessage... and if you did that, I would probably change the name to nsIWebConsoleMessage. You do not have to do this now but it is good to think about how code can be used to make potential future bugs easier to fix.

@@ +4216,5 @@
>    nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mScriptGlobalObject);
>    mWindow = window;
>  
>    // Now that we know what our window is, we can flush the CSP errors to the
>    // Web Console.

Fix: update this comment to reflect that we are flushing all "occurred before we had the web console" errors at this point.

@@ +4218,5 @@
>  
>    // Now that we know what our window is, we can flush the CSP errors to the
>    // Web Console.
>    FlushCSPWebConsoleErrorQueue();
> +  if(window) {

Do you need this if statement? I know a prior comment says "pointer to the window (or lack thereof)", but if you look at the beginning of the function, there is an assertion if you can't QI to nsPIDOMWindow. I tried asking on #developers but nobody responded :(

::: dom/locales/en-US/chrome/security/security.properties
@@ +9,5 @@
>  OldCSPHeaderDeprecated=The X-Content-Security-Policy and X-Content-Security-Report-Only headers will be deprecated in the future. Please use the Content-Security-Policy and Content-Security-Report-Only headers with CSP spec compliant syntax instead.
>  # LOCALIZATION NOTE: Do not translate "X-Content-Security-Policy/Report-Only" or "Content-Security-Policy/Report-Only"
>  BothCSPHeadersPresent=This site specified both an X-Content-Security-Policy/Report-Only header and a Content-Security-Policy/Report-Only header. The X-Content-Security-Policy/Report-Only header(s) will be ignored.
> +# LOCALIZATION NOTE: Do not translate "Strict-Transport-Security" or "HSTS"
> +InvalidSTSHeaders=This site specified an invalid Strict-Transport-Security header. Please use Strict-Transport-Security headers with HSTS spec compliant syntax instead.

Fix: Remove the part about "spec compliant syntax" (is that leftover from 821877?). These errors are triggered if there is an error parsing the header, and do not specifically relate to spec compliance.

::: xpcom/base/nsISecurityConsoleMessage.idl
@@ +12,5 @@
> +};
> +
> +%{ C++
> +#define NS_SECURITY_CONSOLE_MESSAGE_CONTRACTID "@mozilla.org/securityconsole/message;1"
> +%}

For the future reference, can you explain the rationale for creating an interface for messages in the bug somewhere?

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 768095 [details] [diff] [review]
newly proposed patch

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

Forgive the drive-by review - I just have a few style pointers.

::: browser/devtools/webconsole/test/Makefile.in
@@ +116,5 @@
>  	browser_output_longstring_expand.js \
>  	browser_netpanel_longstring_expand.js \
>  	browser_repeated_messages_accuracy.js \
>  	browser_webconsole_bug_821877_csp_errors.js \
> +	browser_webconsole_bug_846918_hsts_invalid-headers.js \

Is there value in having the bug number in the name of the file? It could be in a comment in the file itself instead.

@@ +236,5 @@
>  	test-bug-766001-js-errors.js \
>  	test-bug-821877-csperrors.html \
>  	test-bug-821877-csperrors.html^headers^ \
> +	test-bug-846918-hsts-invalid-headers.html \
> +	test-bug-846918-hsts-invalid-headers.html^headers^ \

Same here regarding the bug number.

::: content/base/src/nsDocument.cpp
@@ +2433,5 @@
> +        messageTagString);
> +  }
> +  aMessages.Clear();
> +  return NS_OK;
> +}

always have a blank line after a function's closing brace

@@ +4218,5 @@
>  
>    // Now that we know what our window is, we can flush the CSP errors to the
>    // Web Console.
>    FlushCSPWebConsoleErrorQueue();
> +  if(window) {

always have spaces after predicates here and elsewhere (i.e. "if (window) ...")

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1304,5 @@
> +{
> +  aMessages = mSecurityConsoleMessages;
> +  return NS_OK;
> +}
> +NS_IMETHODIMP

add a blank line between here and the previous closing brace

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1151,5 @@
>      // All other failures are fatal.
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      rv = stss->ProcessStsHeader(mURI, stsHeader.get(), flags, NULL, NULL);
> +

I would remove this blank line to be more clear that the NS_FAILED test is directly related to the call to ProcessStsHeader

::: netwerk/protocol/http/nsHttpChannel.h
@@ +55,5 @@
>                      , public nsIAsyncVerifyRedirectCallback
>                      , public nsITimedChannel
>  {
>  public:
> +

You probably don't need to add this line if you're not touching this file elsewhere.

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +36,5 @@
>   * Dumping ground for http.  This interface will never be frozen.  If you are
>   * using any feature exposed by this interface, be aware that this interface
>   * will change and you will be broken.  You have been warned.
>   */
> +[scriptable, uuid(5B4B2632-CEE4-11E2-8E84-C7506188709B)]

lower-case hex is more common, from what I've seen

::: xpcom/base/nsSecurityConsoleMessage.cpp
@@ +6,5 @@
> +NS_IMPL_ISUPPORTS1(nsSecurityConsoleMessage, nsISecurityConsoleMessage)
> +
> +nsSecurityConsoleMessage::nsSecurityConsoleMessage()
> +{
> +  /* member initializers and constructor code */

You probably don't need this comment or the one in the destructor.

@@ +32,5 @@
> +{
> +  aMessageCategory = mMessageCategory;
> +  return NS_OK;
> +}
> +NS_IMETHODIMP nsSecurityConsoleMessage::SetMessageCategory(const nsAString & aMessageCategory)

Add a blank line between this and the previous closing brace.
Also, the code style for C++ generally follows this:

NS_IMETHODIMP
nsSecurityConsoleMessage::Blah...
{
...

(with the function return value on its own line)

Comment 12

[Garrett Robinson [:grobinson]]

Putting the bug number in the filename is a common convention in the codebase (and `ls | grep` is faster than `grep -r`, although that's fairly minor).

Comment 13

[Ivan Alagenchev :ialagenchev]

(In reply to Garrett Robinson [:grobinson] from comment #10)
> Comment on attachment 768095 [details] [diff] [review]
> newly proposed patch
> 
> Review of attachment 768095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. Submit this for review modulo the fixes I suggest below.
> If it doesn't say "fix", consider it but don't let it delay you from
> submitting this for review. Have you pushed this to try?
> 
> :::
> browser/devtools/webconsole/test/browser_webconsole_bug_846918_hsts_invalid-
> headers.js
> @@ +15,5 @@
> > +    openConsole(null, function testHSTSErrorLogged (hud) {
> > +      waitForMessages({
> > +        webconsole: hud,
> > +        messages: [
> > +      {
> 
> Fix: Indentation

The indentation appears to be fine according to vim. I have the vim settings you sent me and nothing happens when I try to autoformat. Which line are you referring to? The one that looks funny to me is the opening curly brace for the messages array? Is that the one you think should be addressed?

Comment 14

[Ivan Alagenchev :ialagenchev]

> 
> ::: dom/locales/en-US/chrome/security/security.properties
> @@ +9,5 @@
> >  OldCSPHeaderDeprecated=The X-Content-Security-Policy and X-Content-Security-Report-Only headers will be deprecated in the future. Please use the Content-Security-Policy and Content-Security-Report-Only headers with CSP spec compliant syntax instead.
> >  # LOCALIZATION NOTE: Do not translate "X-Content-Security-Policy/Report-Only" or "Content-Security-Policy/Report-Only"
> >  BothCSPHeadersPresent=This site specified both an X-Content-Security-Policy/Report-Only header and a Content-Security-Policy/Report-Only header. The X-Content-Security-Policy/Report-Only header(s) will be ignored.
> > +# LOCALIZATION NOTE: Do not translate "Strict-Transport-Security" or "HSTS"
> > +InvalidSTSHeaders=This site specified an invalid Strict-Transport-Security header. Please use Strict-Transport-Security headers with HSTS spec compliant syntax instead.
> 
> Fix: Remove the part about "spec compliant syntax" (is that leftover from
> 821877?). These errors are triggered if there is an error parsing the
> header, and do not specifically relate to spec compliance.
> 
It is leftover from the CSP stuff, but I also left it because I found this online: http://tools.ietf.org/html/rfc6797. Do you think I should leave it in, or take it out since that appears to be only an RFC?

Comment 15

[Ivan Alagenchev :ialagenchev]

> 
> ::: xpcom/base/nsISecurityConsoleMessage.idl
> @@ +12,5 @@
> > +};
> > +
> > +%{ C++
> > +#define NS_SECURITY_CONSOLE_MESSAGE_CONTRACTID "@mozilla.org/securityconsole/message;1"
> > +%}
> 
> For the future reference, can you explain the rationale for creating an
> interface for messages in the bug somewhere?

Absolutely!

Initially, we were using nsIScriptError in our nsHttpChannel queue of messages. However, that was requiring us to create objects of nsIScriptError when the header validation fails, store the messages in the queue and then call nsContentUtils::ReportToConsole. The call to ReportToConsole takes in each individual parameter separately, so that required us to deconstruct the nsIScriptError object only to construct it again inside the ReportToConsole method. We wanted to avoid the unnecessary overhead. Additionally we also considered the option of doing everything that ReportToConsole does during the object creation, but that would have required us to pull some headers we didn't want to pull in and dealing with the StringBundles was less than ideal in some of the locations where messages can get generated. 
All of the above considerations led us to believe that creating a new thin object for storing the errors was the best approach going forward. Since this object would have to be accessible from both content and netwerk, we had to add a new xpcom interface. However, since we don't foresee it being used from anywhere else, but internal locations, it is marked as [noscript]

Comment 16

[Ivan Alagenchev :ialagenchev]

> ::: content/base/src/nsDocument.cpp
> @@ +2429,5 @@
> > +    char *category = ToNewUTF8String(categoryString);
> > +
> > +    nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
> > +        category, this, nsContentUtils::eSECURITY_PROPERTIES,
> > +        messageTagString);
> 
> A thought: this could be more generic, and hence more reusable, if the type
> of message (error/warning) and the source of the message were also stored in
> the nsISecurityConsoleMessage... and if you did that, I would probably
> change the name to nsIWebConsoleMessage. You do not have to do this now but
> it is good to think about how code can be used to make potential future bugs
> easier to fix.
I looked into this. The error flag is easy to add, but decided to hold off on this one for now, since the type (nsContentUtils::eSECURITY_PROPERTIES) is an enum PropertiesFile that is declared in nsContentUtils.h - a header that I don't want to pull into the idl. My thinking is that this isn't needed for now, so when it's needed the person that needs it can come up with ways to design around this. 

> @@ +4218,5 @@
> >  
> >    // Now that we know what our window is, we can flush the CSP errors to the
> >    // Web Console.
> >    FlushCSPWebConsoleErrorQueue();
> > +  if(window) {
> 
> Do you need this if statement? I know a prior comment says "pointer to the
> window (or lack thereof)", but if you look at the beginning of the function,
> there is an assertion if you can't QI to nsPIDOMWindow. I tried asking on
> #developers but nobody responded :(
> 
The gist of it - I removed it, tests passed. I'll keep it out. :-)

Comment 17

[Garrett Robinson [:grobinson]]

(In reply to Ivan Alagenchev :ialagenchev from comment #13)

> The indentation appears to be fine according to vim. I have the vim settings
> you sent me and nothing happens when I try to autoformat. Which line are you
> referring to? The one that looks funny to me is the opening curly brace for
> the messages array? Is that the one you think should be addressed?

Vim's built-in javascript indentation is mediocre. I recommend using this Vim plugin: https://github.com/pangloss/vim-javascript.

You're building an array of JS objects. I would expect the objects that are inside the array to be indented one level beyond the line defining the array.

Comment 18

[Garrett Robinson [:grobinson]]

(In reply to Ivan Alagenchev :ialagenchev from comment #14)
> > 
> > ::: dom/locales/en-US/chrome/security/security.properties
> > @@ +9,5 @@
> > >  OldCSPHeaderDeprecated=The X-Content-Security-Policy and X-Content-Security-Report-Only headers will be deprecated in the future. Please use the Content-Security-Policy and Content-Security-Report-Only headers with CSP spec compliant syntax instead.
> > >  # LOCALIZATION NOTE: Do not translate "X-Content-Security-Policy/Report-Only" or "Content-Security-Policy/Report-Only"
> > >  BothCSPHeadersPresent=This site specified both an X-Content-Security-Policy/Report-Only header and a Content-Security-Policy/Report-Only header. The X-Content-Security-Policy/Report-Only header(s) will be ignored.
> > > +# LOCALIZATION NOTE: Do not translate "Strict-Transport-Security" or "HSTS"
> > > +InvalidSTSHeaders=This site specified an invalid Strict-Transport-Security header. Please use Strict-Transport-Security headers with HSTS spec compliant syntax instead.
> > 
> > Fix: Remove the part about "spec compliant syntax" (is that leftover from
> > 821877?). These errors are triggered if there is an error parsing the
> > header, and do not specifically relate to spec compliance.
> > 
> It is leftover from the CSP stuff, but I also left it because I found this
> online: http://tools.ietf.org/html/rfc6797. Do you think I should leave it
> in, or take it out since that appears to be only an RFC?

Yes, there is an RFC (spec) for STS. However, your bug is reporting header parsing error. Saying the header is not spec compliant implies that it parsed correctly (for a previous, development version of the spec) but that it no longer adheres to an updated, spec-compliant syntax. This is wrong and will confuse users.

For reference, that language was used in Bug 821877 because we currently support two versions of CSP in Firefox - the pre-1.0 version and the 1.0 version. We refer to CSP 1.0 as "spec compliant", and 821877 was created to inform developers that know that CSP 1.0 is standardized, they should prefer it over earlier, browser-specific versions of the header.

Comment 19

[[e-mail]]

Comment on attachment 768095 [details] [diff] [review]
newly proposed patch

Just some style points:

>+nsDocument::SendToConsole(nsTArray<nsCOMPtr<nsISecurityConsoleMessage>> &aMessages)
...
>+  aMessages.Clear();
SetScriptGlobalObject already clears its copy of the messages, so this is unnecessary. On the other hand, nsHttpChannel does not, was it supposed to?

>+  if(window) {
>+    nsCOMPtr<nsIChannel> channel = GetChannel();
>+    if(channel) {
>+      nsCOMPtr<nsIHttpChannelInternal> internalChannel = do_QueryInterface(channel);
>+
>+      if(internalChannel) {
do_QueryInterface already null-tests channel, so you don't need to.
Mozilla style is to have a space after control keywords i.e. if (window)

>+//Web Security Console messages support
>+NS_IMETHODIMP
>+HttpBaseChannel::GetSecurityMessages(nsTArray<nsCOMPtr<nsISecurityConsoleMessage> > &aMessages)
>+{
>+  aMessages = mSecurityConsoleMessages;
You may want to consider swapping the messages if you'll only need them once.

>+    nsTArray<nsCOMPtr<nsISecurityConsoleMessage> > mSecurityConsoleMessages;
You may find nsCOMArray<nsISecurityConsoleMessage> easier to use.

>+        nsAutoString category;
>+        category.AssignLiteral("HSTS");
>+        message->SetMessageCategory(category);
You could use NS_LITERAL_STRING("HSTS") here.

>+    [noscript] void addSecurityMessage(in  securityMessageCOMPtr aMessage);
You could use in nsISecurityConsoleMessage aMessage here.

Comment 20

[[e-mail]]

(In reply to comment #19)
> >+    [noscript] void addSecurityMessage(in  securityMessageCOMPtr aMessage);
> You could use in nsISecurityConsoleMessage aMessage here.
(It would change the type of the parameter so a C++ tweak would also be needed.)

Comment 21

[Ivan Alagenchev :ialagenchev]

(In reply to [e-mail] from comment #19)
> Comment on attachment 768095 [details] [diff] [review]
> newly proposed patch
> 
> Just some style points:
> 
> >+nsDocument::SendToConsole(nsTArray<nsCOMPtr<nsISecurityConsoleMessage>> &aMessages)
> ...
> >+  aMessages.Clear();
> SetScriptGlobalObject already clears its copy of the messages, so this is
> unnecessary. On the other hand, nsHttpChannel does not, was it supposed to?
> 
SetScriptGlobalObject calls FlushCSPWebConsoleErrorQueue, which clears only the CSP queue. The aMessages.Clear from above clears the http channel queue. Let me know know if I misunderstood you. 

Thanks for the NS_LITERAL_STRING and SwapMessages. That is better indeed.

Comment 22

[[e-mail]]

(In reply to Ivan Alagenchev from comment #21)
> (In reply to comment #19)
> > (From update of attachment 768095 [details] [diff] [review])
> > >+nsDocument::SendToConsole(nsTArray<nsCOMPtr<nsISecurityConsoleMessage>> &aMessages)
> > ...
> > >+  aMessages.Clear();
> > SetScriptGlobalObject already clears its copy of the messages, so this is
> > unnecessary. On the other hand, nsHttpChannel does not, was it supposed to?
> > 
> SetScriptGlobalObject calls FlushCSPWebConsoleErrorQueue, which clears only
> the CSP queue. The aMessages.Clear from above clears the http channel queue.
> Let me know know if I misunderstood you. 
> 
No, it clears the copy of the queue that SetScriptGlobalObject retrieved from its call to GetSecurityMessages.

Comment 23

[Ivan Alagenchev :ialagenchev]

Now I see what you are referring to. Sure, I will take the Clear() line out.

Comment 24

[Ivan Alagenchev :ialagenchev]

Attachment: XPCOM patch

Comment 25

[Ivan Alagenchev :ialagenchev]

Attachment: NETWERK patch

Comment 26

[Ivan Alagenchev :ialagenchev]

Attachment: DOM module patch

Comment 27

[Ivan Alagenchev :ialagenchev]

Attachment: DEVTOOLS module patch

Comment 28

[Ivan Alagenchev :ialagenchev]

Here are the results of my try submission: https://tbpl.mozilla.org/?tree=Try&rev=ab335ca99d79

Comment 29

[Mihai Sucan [:msucan]]

Comment on attachment 774377 [details] [diff] [review]
DEVTOOLS module patch

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

Looks good!

::: browser/devtools/webconsole/test/browser_webconsole_bug_846918_hsts_invalid-headers.js
@@ +1,2 @@
> +/* Tests that errors about invalid HSTS security headers are logged
> + *  to the web console */

Please put this test description after the PD license comment.

::: browser/devtools/webconsole/webconsole.js
@@ +4420,5 @@
>          return CATEGORY_CSS;
>  
>        case "Mixed Content Blocker":
>        case "CSP":
> +      case "HSTS":

Not binding for this review, but could we use more descriptive category names?

Comment 30

[Mounir Lamouri (:mounir)]

Comment on attachment 774376 [details] [diff] [review]
DOM module patch

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

My general feeling is that adding that kind of code in the content code might slow down things and I would feel more comfortable with having it living in the console code so I will pass the review to Justin who might have a way more neutral vision on this.

What I would do for example is a way to pass messages directly from the HTTP channel to the Web Console and when the page is loaded, the console would display the error messages passed during the loading. Passing the message could be done async so that it wouldn't slow down too much the general loading time.

Comment 31

[Garrett Robinson [:grobinson]]

(In reply to Mounir Lamouri (:mounir) from comment #30)
> Comment on attachment 774376 [details] [diff] [review]
> DOM module patch
> 
> Review of attachment 774376 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My general feeling is that adding that kind of code in the content code
> might slow down things and I would feel more comfortable with having it
> living in the console code so I will pass the review to Justin who might
> have a way more neutral vision on this.
> 

Ideally we would report the errors (to the nsIConsoleService) when they happen, but in this case the error is reported by header parsing code in the channel, which is run before the docshell is built. Our approach here is based on the approach used by Bug 821877, which is to maintain a queue of "pre-docshell" errors somewhere logical until we now it is ready to be logged to, at which point we flush the queue to the ConsoleService and the messages are correctly routed to the Web Console.

> What I would do for example is a way to pass messages directly from the HTTP
> channel to the Web Console and when the page is loaded, the console would
> display the error messages passed during the loading. Passing the message
> could be done async so that it wouldn't slow down too much the general
> loading time.

:ialagenchev spoke to :msucan on IRC and this appears to be sort-of possible. The Web Console code could use its Network Monitor to inspect channels, and dump any messages from their pre-docshell error queues. I think this would be a hack and is not worth it given the extremely minor peformance hit caused by the current approach.

I am interested in hearing jlebar's opinion, and would also be interested in a more detailed explanation of how the Web Console can inspect error queues on channels.

Comment 32

[Justin Lebar (not reading bugmail)]

What I'm most concerned about is the case where we enqueue either
arbitrarily-many or arbitrarily-large messages.  In either case, a channel can
suddenly take up a lot more memory than it used to.

Similarly, are we guaranteed that we only enqueue CSP messages on channels
attached to nsGlobalWindows?  If not, we'll leak these messages for the
lifetime of their channels.

None of this is a problem with the patches as currently written, but we've seen
cases where the console service uses hundreds of mb of RAM because we enqueue
many small message or a few large messages.

I'm not worried about the speed implications here, if we can be assured that we
meet the criteria above.

Comment 33

[Ivan Alagenchev :ialagenchev]

:jlebar, thank you for your comment. I agree with you, I don't think we can run into these issues for this particular patch. Unfortunately, I also don't have enough background information, or knowledge of what is to come going forward to address your other concerns. 
I am not clear on your comment about the CSP messages, since we enqueue them in nsDocument ATM. I think :grobinson can help me on that one. 
Would something like adding a limit to the max size that the queues can grow to address your concerns with this patch?

Comment 34

[Mihai Sucan [:msucan]]

(In reply to Garrett Robinson [:grobinson] from comment #31)
> > What I would do for example is a way to pass messages directly from the HTTP
> > channel to the Web Console and when the page is loaded, the console would
> > display the error messages passed during the loading. Passing the message
> > could be done async so that it wouldn't slow down too much the general
> > loading time.
> 
> :ialagenchev spoke to :msucan on IRC and this appears to be sort-of
> possible. The Web Console code could use its Network Monitor to inspect
> channels, and dump any messages from their pre-docshell error queues. I
> think this would be a hack and is not worth it given the extremely minor
> peformance hit caused by the current approach.
> 
> I am interested in hearing jlebar's opinion, and would also be interested in
> a more detailed explanation of how the Web Console can inspect error queues
> on channels.

The suggestion I gave Ivan was based on how the web console network monitor can listen to all the window-specific requests. We always have an http channel and we know to which window it belongs to. We can call any new method on the channel to retrieve the queue of messages and log them to the web console.

See WebConsoleUtils.jsm, search for NetworkMonitor. We use the nsIHttpActivityDistributor for monitoring. We get the associatedWindow from the nsILoadContext (channel.notificationCallbacks).

I agree it is a hack, I prefer having Gecko code that logs the messages directly.

Comment 35

[Justin Lebar (not reading bugmail)]

> Would something like adding a limit to the max size that the queues can grow to address your 
> concerns with this patch?

Yes, that would make me feel a lot better.

>> Similarly, are we guaranteed that we only enqueue CSP messages on channels
>> attached to nsGlobalWindows?  If not, we'll leak these messages for the
>> lifetime of their channels.
>
> I am not clear on your comment about the CSP messages, since we enqueue them in nsDocument

Sorry, I meant STS, not CSP.

Comment 36

[Ivan Alagenchev :ialagenchev]

I spoke with :briansmith on irc and he believes that it's possible for http channels to not be attached to nsGlobalWindows. I am going to look at the httpChannel code to see if I can account for that.

Comment 37

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

Comment on attachment 774373 [details] [diff] [review]
NETWERK patch

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1301,5 @@
>  
>    return NS_OK;
>  }
>  
> +//Web Security Console messages support

remove this comment.

@@ +1305,5 @@
> +//Web Security Console messages support
> +NS_IMETHODIMP
> +HttpBaseChannel::GetSecurityMessages(nsCOMArray<nsISecurityConsoleMessage> &aMessages)
> +{
> +  aMessages.SwapElements(mSecurityConsoleMessages);

I think this should be:

aMessages.Clear();
aMessages.SwapElements(mSecurityConsoleMessages);

That is, we don't want the channel to retain any elements that were in aMesssages when the method was called.

@@ +1310,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +HttpBaseChannel::AddSecurityMessage(nsISecurityConsoleMessage *aMessage)

Nit: perhaps you should have this method be something like:

void
HttpBaseChannel::AddSecurityMessage(const nsAString & message,
                                    const nsAString & category)
{
  nsCOMPtr<nsISecurityConsoleMessage> message = 
    do_CreateInstance(NS_SECURITY_CONSOLE_MESSAGE_CONTRACTID);
  if (message) {
    message->SetMessageTag(NS_LITERAL_STRING("InvalidSTSHeaders"));
    message->SetMessageCategory(NS_LITERAL_STRING("HSTS"));
    mSecurityConsoleMessages.AppendElement(aMessage);
  }
}

This way, future callers of AddSecurityMessage will be easy to add.

@@ +1315,5 @@
> +{
> +  mSecurityConsoleMessages.AppendElement(aMessage);
> +  return NS_OK;
> +}
> +//end Web Security Console support

remove this comment.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1194,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      rv = stss->ProcessStsHeader(mURI, stsHeader.get(), flags, NULL, NULL);
>      if (NS_FAILED(rv)) {
> +        nsCOMPtr<nsISecurityConsoleMessage> message(do_CreateInstance(NS_SECURITY_CONSOLE_MESSAGE_CONTRACTID));

wrap this at 80 characters.

do_CreateInstance isn't infallible, is it? You need to check that do_CreateInstance didn't return null.

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +55,5 @@
>       * Get the major/minor version numbers for the response
>       */
>      void getResponseVersion(out unsigned long major, out unsigned long minor);
>  
> +    [noscript] void addSecurityMessage(in  nsISecurityConsoleMessage aMessage);

Do no add addSecurityMessage to the interface since you're not call it from outside of the implementation.

@@ +56,5 @@
>       */
>      void getResponseVersion(out unsigned long major, out unsigned long minor);
>  
> +    [noscript] void addSecurityMessage(in  nsISecurityConsoleMessage aMessage);
> +    [noscript] void getSecurityMessages(in securityMessagesArray aMessages);

Perhaps this would be clearer:

   [noscript] securityMessagesArray takeSecurityMessages();

I suggest you name this method "takeSecurityMessages" because it isn't a normal (side-effect-free) getter. Instead, it is unusual in that it has the side-effect of clearing the security messages. You should document what this method does here in the IDL.

Comment 38

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

(In reply to Justin Lebar [:jlebar] from comment #32)
> Similarly, are we guaranteed that we only enqueue CSP messages on channels
> attached to nsGlobalWindows?  If not, we'll leak these messages for the
> lifetime of their channels.

> None of this is a problem with the patches as currently written, but we've
> seen cases where the console service uses hundreds of mb of RAM because we
> enqueue many small message or a few large messages.

I agree, but I think you do not need to be so worried. These messages are only going to be enqueued when the server does something bad (in this case, sending an invalid HSTS header). That should be very, very rare. The overhead of the messages themselves is low and likely to stay low. If the overhead of the messages were high, then this would be even worse for channels attached to nsGlobalWindows than for channels not attached to nsGlobalWindows, because the number of channels attached to nsGlobalWindows is much higher. Finally, doing what you suggest would make this untestable in xpcshell, AFAICT.

So, I suggest that we just resolve this by adding a comment to nsHttpChannel::AddSecurityMessage saying to be careful to only use it for adding messages in rare, erroneous cases so that we don't end up bloating channels unnecessarily.

Comment 39

[Justin Lebar (not reading bugmail)]

That's fine with me, so long as you promise to fix any memory issues caused here as a high priority, and so long as the error messages are static strings (as opposed to arbitrarily-long strings which include data from the server).

I'm determined not to repeat the mistakes of the console service, which continue to haunt us.

Comment 40

[Ivan Alagenchev :ialagenchev]

(In reply to Justin Lebar [:jlebar] from comment #39)
> That's fine with me, so long as you promise to fix any memory issues caused
> here as a high priority, and so long as the error messages are static
> strings (as opposed to arbitrarily-long strings which include data from the
> server).
> 
> I'm determined not to repeat the mistakes of the console service, which
> continue to haunt us.

Justin, thank you for your input! Here are my thoughts:

I don't see a way in the http channel to determine if the channel is a document channel without coupling the netwerk code with the DOM code, which is something that is frowned upon by the netwerk folks. I think that's the only way I can address your concern about only enqueing messages that have documents with them. I don't exactly understand what you mean by "leaking" messages, but I think you are concerned with accumulating messages that can never be flushed, because we don't have windows where to flush to. Do I understand you correctly? If someone knows a better approach then getting a handle of the associated window in the http channel and testing it for null, then I'm all ears. 

Currently, all messages are static strings and don't contain any data coming from the server side. I don't foresee things changing, but it's code - things generally do change. The only safe-guard I can put in place is the comment that Brian mentioned and to reference this bug # in the comment. 

I can still try to put an upper bound on the number of messages that can be accumulated. What's a good number in your opinion? 

If anyone has other ideas, suggestions or comments how to address your concerns and move forward, then please add them to the discussion. I want to make sure we move forward in the best possible way. 

Thank you everyone for taking the time to discuss this.

Comment 41

[Justin Lebar (not reading bugmail)]

> Do I understand you correctly?

Yes.  If it's just a handful of messages and they're not of arbitrary size, then it's fine, we don't need to worry about it.  If OTOH a channel can accumulate messages, or if the messages are not bounded in size, then we do.

It sounds like we don't need to worry.  I'm not trying to stand in your way!

Comment 42

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

(In reply to Justin Lebar [:jlebar] from comment #39)
> That's fine with me, so long as you promise to fix any memory issues caused
> here as a high priority, and so long as the error messages are static
> strings (as opposed to arbitrarily-long strings which include data from the
> server).
> 
> I'm determined not to repeat the mistakes of the console service, which
> continue to haunt us.

I promise.

(In reply to Ivan Alagenchev :ialagenchev from comment #40)

In comment 39 Justin already said he was OK with just adding the comment. Let's not make this more complicated than it needs to be.

Comment 43

[Ivan Alagenchev :ialagenchev]

(In reply to Mihai Sucan [:msucan] from comment #29)
> Comment on attachment 774377 [details] [diff] [review]
> DEVTOOLS module patch
> 
> Review of attachment 774377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> :::
> browser/devtools/webconsole/test/browser_webconsole_bug_846918_hsts_invalid-
> headers.js
> @@ +1,2 @@
> > +/* Tests that errors about invalid HSTS security headers are logged
> > + *  to the web console */
> 
> Please put this test description after the PD license comment.

done 

> ::: browser/devtools/webconsole/webconsole.js
> @@ +4420,5 @@
> >          return CATEGORY_CSS;
> >  
> >        case "Mixed Content Blocker":
> >        case "CSP":
> > +      case "HSTS":
> 
> Not binding for this review, but could we use more descriptive category
> names?

I changed the category to "Invalid HSTS Headers" and we've already gone forward with better descriptions for other message categories. Thank you for your suggestion.

Comment 44

[Ivan Alagenchev :ialagenchev]

(In reply to Brian Smith (:briansmith), was [e-mail] (:bsmith) from comment #42)
> (In reply to Justin Lebar [:jlebar] from comment #39)
> > That's fine with me, so long as you promise to fix any memory issues caused
> > here as a high priority, and so long as the error messages are static
> > strings (as opposed to arbitrarily-long strings which include data from the
> > server).
> > 
> > I'm determined not to repeat the mistakes of the console service, which
> > continue to haunt us.
> 
> I promise.
> 
> (In reply to Ivan Alagenchev :ialagenchev from comment #40)
> 
> In comment 39 Justin already said he was OK with just adding the comment.
> Let's not make this more complicated than it needs to be.

Works for me!

Comment 45

[Ivan Alagenchev :ialagenchev]

Attachment: NETWERK_846918.patch

New patch with suggested changes incorporated
Here is a new try server submission: https://tbpl.mozilla.org/?tree=Try&rev=e32764f202a7

Comment 46

[Ivan Alagenchev :ialagenchev]

Attachment: DOM_846918.patch

I made a very minor change to the DOM patch because of the changes suggested by Brian. I'm uploading the new version, since the old patch hasn't been r+-ed yet.

Comment 47

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

Comment on attachment 778250 [details] [diff] [review]
NETWERK_846918.patch

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1318,5 @@
> + * Add only a limited number of messages to the queue to keep
> + * the channel size down and do so only in rare erroneous situations.
> + * More information can be found here:
> + * https://bugzilla.mozilla.org/show_bug.cgi?id=846918
> + */

Use C++-style comments (//)
You can site the bug as simply "bug 846918" instead of with the URL.

@@ +1321,5 @@
> + * https://bugzilla.mozilla.org/show_bug.cgi?id=846918
> + */
> +NS_IMETHODIMP
> +HttpBaseChannel::AddSecurityMessage(const nsAString &aMessageTag,
> +    const nsAString &aMessageCategory)

align the second parameter declaration with the first parameter's declaration.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +44,5 @@
>  #include "sslt.h"
>  #include "nsContentUtils.h"
>  #include "nsIPermissionManager.h"
>  #include "nsIPrincipal.h"
> +#include "nsISecurityConsoleMessage.h"

You don't need this include because the header includes it.

@@ +49,4 @@
>  #include "nsIScriptSecurityManager.h"
>  #include "nsISSLStatus.h"
>  #include "nsISSLStatusProvider.h"
> +#include "nsIDOMWindow.h"

Remove this include.

@@ +1196,5 @@
>  
>      rv = stss->ProcessStsHeader(mURI, stsHeader.get(), flags, NULL, NULL);
>      if (NS_FAILED(rv)) {
> +        AddSecurityMessage(NS_LITERAL_STRING("InvalidSTSHeaders"),
> +                NS_LITERAL_STRING("Invalid HSTS Headers"));

in e10s mode, this code will run in the parent process. But, I think that you need access to the messages in the child process in order for the console to pick them up.

AFAICT, this means that you should modify the IPC code (HttpChannelParent/Child) so that one of the messages that gets sent from parent -> child includes all the messages from parsing the HTTP headers. You can do this in a follow-up patch.

(Also, maybe I'm wrong: verify that what I say above is correct.)

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +59,5 @@
> +    /*
> +     * Retrieves all security messages from the security message queue
> +     * and empties the queue after retrieval
> +     */
> +    [noscript] void takeAllSecurityMessages(in securityMessagesArray aMessages);

Shouldn't the aMessages parameter be marked inout or something? It is confusing for it to be marked "in".

Comment 48

[Ivan Alagenchev :ialagenchev]

> @@ +1196,5 @@
> >  
> >      rv = stss->ProcessStsHeader(mURI, stsHeader.get(), flags, NULL, NULL);
> >      if (NS_FAILED(rv)) {
> > +        AddSecurityMessage(NS_LITERAL_STRING("InvalidSTSHeaders"),
> > +                NS_LITERAL_STRING("Invalid HSTS Headers"));
> 
> in e10s mode, this code will run in the parent process. But, I think that
> you need access to the messages in the child process in order for the
> console to pick them up.
> 
> AFAICT, this means that you should modify the IPC code
> (HttpChannelParent/Child) so that one of the messages that gets sent from
> parent -> child includes all the messages from parsing the HTTP headers. You
> can do this in a follow-up patch.
> 
> (Also, maybe I'm wrong: verify that what I say above is correct.)
>
Thanks I will investigate this. 

> ::: netwerk/protocol/http/nsIHttpChannelInternal.idl
> @@ +59,5 @@
> > +    /*
> > +     * Retrieves all security messages from the security message queue
> > +     * and empties the queue after retrieval
> > +     */
> > +    [noscript] void takeAllSecurityMessages(in securityMessagesArray aMessages);
> 
> Shouldn't the aMessages parameter be marked inout or something? It is
> confusing for it to be marked "in".

I was confused after reading the xpidl description on in/out/inout when I read MDN when I was implementing this and asked for clarification on #developers. Here is what I got: 

NeilAway: ok, so yeah, xpidl isn't very good at out arrays
ialagenchev: Can you give me a brief description about when I would use in, out and inout? The explanation on MDN just confused me more
NeilAway: I did at one point write some helper methods to make it easier but I'm not sure I ever attached it to a bug let alone requested review
NeilAway: well, don't worry about inout, we avoid it, and as a result, if you do want to use it, there are no helpers for it, which makes it easy to get wrong
NeilAway: in works reasonably well in xpidl, even for arrays (you pass a pointer to the first element and the length)
NeilAway: out arrays aren't much fun, although the story for JS callers handling a return array is much better than it used to be

So I went with in...

Comment 49

[Justin Lebar (not reading bugmail)]

Comment on attachment 778252 [details] [diff] [review]
DOM_846918.patch

> nsresult
>+nsDocument::SendToConsole(nsCOMArray<nsISecurityConsoleMessage> &aMessages)
>+{
>+  for (uint32_t i = 0; i < aMessages.Length(); i++) {
>+    nsCOMPtr<nsISecurityConsoleMessage> securityMessage = aMessages[i];
>+
>+    nsString messageTag;
>+    securityMessage->GetMessageTag(messageTag);
>+    char *messageTagString = ToNewUTF8String(messageTag);
>+
>+    nsString categoryString;
>+    securityMessage->GetMessageCategory(categoryString);
>+    char *category = ToNewUTF8String(categoryString);

Doesn't this leak the UTF8 strings?

I think you want instead to do

  ReportToConsole(NS_ConvertUTF16toUTF8(categoryString).get());

>+    nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
>+        category, this, nsContentUtils::eSECURITY_PROPERTIES,
>+        messageTagString);
>+  }
>+  return NS_OK;
>+}

>   // Now that we know what our window is, we can flush the CSP errors to the
>-  // Web Console.
>+  // Web Console. We are flushing all messages that occured and were stored
>+  // in the queue prior to this point

Nit: End sentences with a period.

>diff --git a/dom/locales/en-US/chrome/security/security.properties b/dom/locales/en-US/chrome/security/security.properties
>index 66bf86a..4313fb7 100644
>--- a/dom/locales/en-US/chrome/security/security.properties
>+++ b/dom/locales/en-US/chrome/security/security.properties

I can't review this bit.

Comment 50

[Justin Lebar (not reading bugmail)]

Comment on attachment 774372 [details] [diff] [review]
XPCOM patch

>diff --git a/xpcom/base/nsISecurityConsoleMessage.idl b/xpcom/base/nsISecurityConsoleMessage.idl
>new file mode 100644
>index 0000000..15c94c6
>--- /dev/null
>+++ b/xpcom/base/nsISecurityConsoleMessage.idl

>@@ -0,0 +1,16 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+* License, v. 2.0. If a copy of the MPL was not distributed with this
>+* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: The *'s should all be aligned.

>+#include "nsISupports.idl"
>+
>+[uuid(FE9FC9B6-DDE2-11E2-A8F1-0A326188709B)]
>+interface nsISecurityConsoleMessage : nsISupports

Please add a brief comment explaining what this interface is for.

>+{
>+  attribute AString messageTag;
>+  attribute AString messageCategory;

Nit: I might just call these "tag" and "category"; the interface already has
"message" in the name.  But that's up to you.

>diff --git a/xpcom/base/nsSecurityConsoleMessage.h b/xpcom/base/nsSecurityConsoleMessage.h
>new file mode 100644
>index 0000000..94baf7c
>--- /dev/null
>+++ b/xpcom/base/nsSecurityConsoleMessage.h

>@@ -0,0 +1,28 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ *  * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ *   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Please fix this.

>diff --git a/xpcom/base/nsSecurityConsoleMessage.cpp b/xpcom/base/nsSecurityConsoleMessage.cpp
>new file mode 100644
>index 0000000..26dc89a
>--- /dev/null
>+++ b/xpcom/base/nsSecurityConsoleMessage.cpp

>@@ -0,0 +1,44 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+#include "nsSecurityConsoleMessage.h"

Nit: Newline after the license header.

>+NS_IMPL_ISUPPORTS1(nsSecurityConsoleMessage, nsISecurityConsoleMessage)
>+
>+nsSecurityConsoleMessage::nsSecurityConsoleMessage()
>+{
>+}
>+
>+nsSecurityConsoleMessage::~nsSecurityConsoleMessage()
>+{
>+}
>+
>+/* attribute AString messageTag; */

We don't need these comments, unless you really like them.  Old code has them,
but old code is old.

>+NS_IMETHODIMP
>+nsSecurityConsoleMessage::GetMessageTag(nsAString & aMessageTag)

Old style is to do |Foo & aFoo|, but new style is to do |Foo& aFoo|.  We should
follow new style in new code (sorry it's so confusing).

r=me with these nits addressed.

Comment 51

[Ivan Alagenchev :ialagenchev]

> >+{
> >+  attribute AString messageTag;
> >+  attribute AString messageCategory;
> 
> Nit: I might just call these "tag" and "category"; the interface already has
> "message" in the name.  But that's up to you.
>
I went with your suggestion. 
> >+
> >+nsSecurityConsoleMessage::~nsSecurityConsoleMessage()
> >+{
> >+}
> >+
> >+/* attribute AString messageTag; */
> 
> We don't need these comments, unless you really like them.  Old code has
> them,
> but old code is old.
Agreed. I had them there because of the example code I was using. I don't see the need for them, so I removed them. 
> >+NS_IMETHODIMP
> >+nsSecurityConsoleMessage::GetMessageTag(nsAString & aMessageTag)
> 
> Old style is to do |Foo & aFoo|, but new style is to do |Foo& aFoo|.  We
> should
> follow new style in new code (sorry it's so confusing).
Thanks for letting me know. Code has been changed accordingly. 

> r=me with these nits addressed.
Much appreciated.

Comment 52

[Ivan Alagenchev :ialagenchev]

Attachment: DOM_846918.patch

Patch incorporating changes suggested from previous review and splitting out the localization text as a separate patch

Comment 53

[Ivan Alagenchev :ialagenchev]

Attachment: UX_846918.patch

Changes adding a localized error string to security.properties

Comment 54

[Axel Hecht [:Pike]]

Comment on attachment 780003 [details] [diff] [review]
UX_846918.patch

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

There's nothing for me to review in this patch, really, clearing the flag.

Comment 55

[Justin Dolske [:Dolske]]

Comment on attachment 780003 [details] [diff] [review]
UX_846918.patch

I don't think this string needs explicit review, but I'm ok with it. It's short and clear.

I'd generally suggest that strings like this say _what_ site generated the message, but since our console is now more tab-specific and the other strings here also don't, I'm not concerned. See also bug 607067 :-)

Comment 56

[Ivan Alagenchev :ialagenchev]

Attachment: DOM_846918.patch

addresses Smaug's suggestions from irc chat

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 780065 [details] [diff] [review]
DOM_846918.patch

I think you don't need to #include nsISecurityConsoleMessage.h in nsDocument.h
class nsISecurityConsoleMessage; should be enough.

>+nsDocument::SendToConsole(nsCOMArray<nsISecurityConsoleMessage>& aMessages)
>+{
>+  for (uint32_t i = 0; i < aMessages.Length(); ++i) {
>+    nsCOMPtr<nsISecurityConsoleMessage> securityMessage = aMessages[i];
nsCOMPtr<nsISecurityConsoleMessage> securityMessage doesn't have to be strong

>@@ -4281,18 +4301,26 @@ nsDocument::SetScriptGlobalObject(nsIScriptGlobalObject *aScriptGlobalObject)
>   }
> 
>   // Remember the pointer to our window (or lack there of), to avoid
>   // having to QI every time it's asked for.
>   nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mScriptGlobalObject);
>   mWindow = window;
> 
>   // Now that we know what our window is, we can flush the CSP errors to the
>-  // Web Console.
>+  // Web Console. We are flushing all messages that occured and were stored
>+  // in the queue prior to this point.
>   FlushCSPWebConsoleErrorQueue();
>+  nsCOMPtr<nsIChannel> channel = GetChannel();
>+  nsCOMPtr<nsIHttpChannelInternal> internalChannel = do_QueryInterface(channel);
nsCOMPtr<nsIHttpChannelInternal> internalChannel = do_QueryInterface(GetChannel())

>+  if (internalChannel) {
>+    nsCOMArray<nsISecurityConsoleMessage> messages;
>+    internalChannel->TakeAllSecurityMessages(messages);
>+    SendToConsole(messages);
>+  }
Assuming we don't want to do anything special with data documents, this should be ok.
(XHR's responseXML and XMLDocument.load are examples of data documents. And looks like csp doesn't handle them either.)

I think I want to see a new patch with nits fixed, and verification that we don't want to handle data documents.
(Should we look for other stuff loading data? images? script files? css?)

Comment 58

[Ivan Alagenchev :ialagenchev]

> 
> I think I want to see a new patch with nits fixed, and verification that we
> don't want to handle data documents.
> (Should we look for other stuff loading data? images? script files? css?)

I would like to move forward with normal documents for now and plan to investigate the other cases as part of a separate bug. I already created one: https://bugzilla.mozilla.org/show_bug.cgi?id=897568

A new patch is on the way to address the nits

Comment 59

[Ivan Alagenchev :ialagenchev]

(In reply to Olli Pettay [:smaug] from comment #57)
> Comment on attachment 780065 [details] [diff] [review]
> DOM_846918.patch
> 
> I think you don't need to #include nsISecurityConsoleMessage.h in
> nsDocument.h
> class nsISecurityConsoleMessage; should be enough.
> 

I get a compilation error if I use class nsISecurityConsoleMessage instead of the #include: 

note: forward declaration of 'nsISecurityConsoleMessage'
5:37.49 class nsISecurityConsoleMessage;

Comment 60

[Justin Lebar (not reading bugmail)]

> I get a compilation error if I use class nsISecurityConsoleMessage instead of the 
> #include: 

Even after you added the #include to the cpp file?

Comment 61

[Justin Lebar (not reading bugmail)]

I delegate future reviews here to Olli.

Comment 62

[Ivan Alagenchev :ialagenchev]

Attachment: DOM_846918.patch

Yet another DOM patch with implemented suggestions from reviews.

Comment 63

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 780527 [details] [diff] [review]
DOM_846918.patch

Just retry to fix the #include handling per
https://bugzilla.mozilla.org/show_bug.cgi?id=846918#c60

Comment 64

[Ivan Alagenchev :ialagenchev]

(In reply to Olli Pettay [:smaug] from comment #63)
> Comment on attachment 780527 [details] [diff] [review]
> DOM_846918.patch
> 
> Just retry to fix the #include handling per
> https://bugzilla.mozilla.org/show_bug.cgi?id=846918#c60

The DOM patch I have uploaded used Justin's advice and it worked.

Comment 65

[Ivan Alagenchev :ialagenchev]

Comment on attachment 780003 [details] [diff] [review]
UX_846918.patch

r=dolske

Comment 66

[Ivan Alagenchev :ialagenchev]

Comment on attachment 780527 [details] [diff] [review]
DOM_846918.patch

try submission: https://tbpl.mozilla.org/?tree=Try&rev=9f9af1d609cf
r=smaug

Comment 67

[Ivan Alagenchev :ialagenchev]

Attachment: NETWORK_846918.patch

r=brian (bsmith)

Comment 68

[Ivan Alagenchev :ialagenchev]

Attachment: XPCOM_846918.patch

r=justin.lebar

Comment 69

[Ivan Alagenchev :ialagenchev]

Attachment: DEVTOOLS_846918.patch

Comment 70

[Camilo Viecco (:cviecco)]

chekin central:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b1e97a8eea0f

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 781263 [details] [diff] [review]
DEVTOOLS_846918.patch

Also, I recommend posting a pushlog link rather than a tbpl link as tbpl data isn't necessarily guarantees to persist forever.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=b1e97a8eea0f

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00da124c775d
https://hg.mozilla.org/mozilla-central/rev/4e11fe1c46cf
https://hg.mozilla.org/mozilla-central/rev/e0c6ca4c3e43
https://hg.mozilla.org/mozilla-central/rev/b3d9fd3ba0cc
https://hg.mozilla.org/mozilla-central/rev/b1e97a8eea0f

Comment 73

[Jason Duell]

So I'm exceedingly late to the party here, but a couple comments.

1) re: comment 40: you can tell if a channel is for a document (top-level) page load: its nsIChannel.loadflags will have LOAD_DOCUMENT_URI set:

  https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIChannel.idl?from=nsIChannel.idl#189

Was the idea to throw an error if AddSecurityMessage is called and it's not a document channel?  We could add that (seems like a nice idea)

2) Channels can stick around for a long time (the bfcache keeps them alive IIRC, possibly for a very long time).  If HttpBaseChannel::TakeAllSecurityMessages() is called, then all the memory we use for error messages gets owned by the console, which I assume handles everything fine.  Are we guranteed that that's always the case?  Also, do we know if that is guaranteed to happen by the time OnStopRequest is called for the channel?  If we could free the messages if they haven't been used by the time OnStopRequest is over, that would provide some nice, time-bounded garbage collection here.  That said, given how rare these messages are supposed to be, I'm not super-concerned if it's not possible to do this.

If either #1 or #2 is doable, lets open a new bug for them.

Comment 74

[Tom Tromey :tromey]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #73)

In bug 1096908, Jason asked me to take a look at this.

> Was the idea to throw an error if AddSecurityMessage is called and it's not
> a document channel?  We could add that (seems like a nice idea)

I'm afraid I don't know the answer here.

> 2) Channels can stick around for a long time (the bfcache keeps them alive
> IIRC, possibly for a very long time).  If
> HttpBaseChannel::TakeAllSecurityMessages() is called, then all the memory we
> use for error messages gets owned by the console, which I assume handles
> everything fine.  Are we guranteed that that's always the case?  Also, do we
> know if that is guaranteed to happen by the time OnStopRequest is called for
> the channel? 

I don't know this code extremely well, so apologies if this isn't so useful.

One thing I don't understand here is that HttpBaseChannel::AddSecurityMessage
unconditionally adds the message to mSecurityConsoleMessages, but also tries
to add it to the console using console->LogMessage(error).  I suppose I would have
expected one or the other.

I also couldn't tell if nsDocument::SetScriptGlobalObject (the only caller of TakeAllSecurityMessages)
can be called multiple times, or if it can race with the security messages.
If it is only called once, and the ordering of the calls can vary, then it seems
that some messages can be hang around uselessly forever -- not a leak exactly, as they
will be properly freed when the channel is destroyed, but just needless allocations.

Comment 75

[Ivan Alagenchev :ialagenchev]

:jduell,

This code has been ~ 2 years in the code base now and the context and surrounding internals is long lost to me. Most of the communication about this particular patch is here. Is there something concerning around the patch, or are you suggesting an improvement? I'm not sure why you are commenting on an old patch.