897240 - Add a security console utils class that will be responsible for creating and sending security related messages to the console

Status: RESOLVED WONTFIX

(DevTools :: Console, task, P2)

Description

[Ivan Alagenchev :ialagenchev]

The SecurityReportTool needs a way to be able to add observer events when security related events are encountered https://bugzilla.mozilla.org/show_bug.cgi?id=890224. 
Currently, there isn't a location where we can add the observer events that doesn't require changes across multiple locations and also makes sense from a design perspective. We also need an approach that will reduce effort and required modifications for any future security related messages. 

Changes for https://bugzilla.mozilla.org/show_bug.cgi?id=846918 added the new nsISecurityConsoleMessage type to handle security related error messages. 

We need to create a nsISecurityConsoleUtils (or a different name) class that will have a wrapper method around logic that sends security messages to the console. A suggested name is SendConsoleMessage. This method will have to handle the logging of any of the existing security related messages - HSTS, Insecure Passwords, Mixed Content, CSP, etc. Future security messages related code will also take advantage of it. 
Any current places where security messages are created and sent to the console for display will have to be modified to use the newly created nsISecurityConsoleUtils. This way observers and any additional logic related to security messages can be easily added to one centralized location. 
For reference, CSP uses cspd_log: 
http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#983
to send the messages and other code uses nsContentUtils::ReportToConsole http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#2959.

The new interface should use nsAString for string parameters and not const char* like nsContentUtils::ReportToConsole to reduce the likelihood of memory leaks when using the new interface.

Comment 1

[Ivan Alagenchev :ialagenchev]

I think that providing something like what nsIConsoleService.idl does with nsIScriptError.idl might be a good approach going forward. This would expose the new service to both .js and c++. nsIConsoleService also allows listeners to be added and removed. The methods would have to be different, but the overall design can probably be very similar. Here is a good link that shows the changes that add nsIConsoleService nicely: https://github.com/mozilla/mozilla-central/commit/b93c689e6633101918e7b5815c7d0d17d7812e06

Comment 2

[Ivan Alagenchev :ialagenchev]

I've been asked a few questions about the design on IRC, so I am going to add some comments here about the reason why we ended up with a need for a separate security service for logging to the console.
The primary reason why nsISecurityConsoleMessage doesn't inherit from nsIConsoleMessage is that the message attribute in nsIConsoleMessage represents the localized message that will actually be logged to the console whereas nsISecurityConsoleMessage instead contains only the message tag that is used as a lookup into the localization files. For bug 846918, we decided to store only the message localization lookup keys(referred to as messageTags, or messageNames throughout the code base https://bugzilla.mozilla.org/show_bug.cgi?id=875456#c20) in the nsISecurityConsoleMessage, because we used those as parameters for nsContentUtils::ReportToConsole. 
Since I am now adding our own SecurityConsoleService, we can make the conversion to message tags in the constructor and use nsConsoleService to log messages.  
However, since nsConsoleService works on localized nsIConsoleMessages, the observer topics sent by nsConsoleService are unreliable for the purpose of bug 890224. Therefore, I am doing the localization inside nsISecurityConsoleService. After I localize the string, I make a call to nsConsoleService->LogMessage and then raise the observer service event passing on the localization lookup key. This way listeners can be reliably notified about the appropriate security events.
In addition, I have decided to move away from using nsContentUtils::ReportToConsole, because I don't have access to the nsIDocument via XPCOM, which is needed as a parameter to ReportToConsole.  Instead, I am using the innerWindowId as a parameter to nsISecurityConsoleUtils::LogMessage and am no longer using ReportToConsole.

I am planning to submit a WIP patch with the above design in place pretty soon. However, if anyone has suggestions on how to solve this in a better way, please feel free to comment here. I would be very open to suggestions, since apart from documenting things, the purpose of this comment is to also solicit feedback on the approach.

Comment 3

[Ivan Alagenchev :ialagenchev]

I ran into some issues while trying to implement this. The public FormatLocalizedStrings from nsContentUtils takes fixed length arrays, which doesn't work for us. After a long discussion with Ms2ger and Waldo on the reason why I need this in the first place, I have the green light from sicking to go ahead and change the method to public, so I can use it. He is fine with me exposing it as long as the method doesn't use deprecated APIs. To my knowledge, it doesn't and further it's a heavily exercised code since ReportToConsole calls it often.

Comment 4

[Ivan Alagenchev :ialagenchev]

Attachment: 897240.patch

Comment 5

[Garrett Robinson [:grobinson]]

Comment on attachment 791117 [details] [diff] [review]
897240.patch

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

Makefile.in and nsMixedContentBlocker.cpp are bitrotted.

The browser chrome test looks fine, although I wonder if we don't need a new test here as much as we need to check the observer in all of the old tests. Since you change some of that code here, it might be better just to update those tests to also listen for the observer as well as checking the web console.

It seems that you replicate a lot of the data type and functionality of nsIScriptError in nsSecurityConsoleMessage.cpp, and of nsIConsoleService in nsSecurityConsoleService.cpp. Of the two, I think the nsIScriptError replication is more egregious. Is it possible to have nsISecurityConsoleMessage inherit from nsIScriptError, so you can avoid duplicating all of the same fields (and identical boilerplate code)? It looks like the only missing thing from nsIScriptError on nsSecurityConsoleMessage is the innerWindowID.

Where is the implementation of nsISecurityConsoleListener? Again, I feel like we might be able to reuse existing code to implement this, but could be wrong.

Finally, I think it's important to check in with Kailas and make sure this will help his use case. AFAICT, this code reports a localized message string to the console, then sends that same message to an observer topic named "security-console-message-received". Is that really much of an improvement over the previous situation? Previously, he could monitor the nsIConsoleService and parse the message strings. Now he can monitor the "security-console-message-received" topic, and parse the message strings. It stills requires that fragile and annoying parsing step for all types of security errors, which I thought is what we were trying to avoid.

::: content/base/src/nsDocument.cpp
@@ +2463,5 @@
>  void
>  CSPErrorQueue::Flush(nsIDocument* aDocument)
>  {
> +  nsCOMPtr<nsISecurityConsoleService> console(
> +      do_GetService(NS_SECURITY_CONSOLESERVICE_CONTRACTID));

Nit: I would format this:

  nsCOMPtr<nsISecurityConsoleService> console(do_GetService(
       NS_SECURITY_CONSOLESERVICE_CONTRACTID));

Makes the top line has close to 80 as possible.

@@ +2465,5 @@
>  {
> +  nsCOMPtr<nsISecurityConsoleService> console(
> +      do_GetService(NS_SECURITY_CONSOLESERVICE_CONTRACTID));
> +  if(!console) {
> +    return;

If we return early here, then we never call mErrors.Clear(). Is that a potential memory leak? I would put the for loop below in an if (console) { } block, and then you call .Clear() either way.

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +45,4 @@
>  
> +    let consoleService = Cc["@mozilla.org/securityconsole/service;1"]
> +      .createInstance(Ci.nsISecurityConsoleService);
> +

Nit: Unnecessary newline.

::: xpcom/base/nsISecurityConsoleMessage.idl
@@ +20,5 @@
> +  attribute AString sourceName;
> +  attribute uint32_t lineNumber;
> +  attribute uint32_t columnNumber;
> +
> +

Nit: remove unnecessary newlines.

::: xpcom/base/nsISecurityConsoleService.idl
@@ +5,5 @@
> +
> +#include "nsISupports.idl"
> +#include "nsISecurityConsoleMessage.idl"
> +
> +interface nsIDocument;

Why is this necessary?

::: xpcom/base/nsSecurityConsoleService.cpp
@@ +32,5 @@
> +//      aMessage - the nsISecurityConsoleMessage that is to be passed
> +//      to the observers with the "security-console-message-received"
> +//      observer topic.
> +nsresult
> +nsSecurityConsoleService::sendObserverNotification(nsISecurityConsoleMessage* aMessage)

Style convention is to capitalize "send".

@@ +41,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  observerService->NotifyObservers(aMessage,
> +      "security-console-message-received", NULL);

Is this the most useful behavior? I thought the original desired behavior was to have different topics for different types of errors (SSL, MCB, etc.). Doesn't this end up being the same use case for Kailas as before, i.e. listening on one topic and parsing the message (fragile, annoying) to figure out what's going on?

@@ +53,5 @@
> +//      error message.
> +//      aErrorText - Stores the localized error text after the operations are
> +//      complete.
> +nsresult
> +nsSecurityConsoleService::getLocalizedMessage(nsXPIDLString* aErrorText,

Style convention is to capitalize "get".

@@ +67,5 @@
> +  aMessage->GetParams(&params);
> +
> +  NS_ASSERTION((params && paramsLength) || (!params && !paramsLength),
> +               "Supply either both parameters and their number or no"
> +               "parameters and 0.");

Generally, you should not split messages across lines like this (even in the interest of keeping the line length < 80 characters). It makes it impossible to grep for this error when someone encounters it.

Comment 6

[Garrett Robinson [:grobinson]]

^^ Kailas, please see the last paragraph of the first part of my review. What do you think? Am I missing something here?

Comment 7

[Ivan Alagenchev :ialagenchev]

(In reply to Garrett Robinson [:grobinson] from comment #5)
> Comment on attachment 791117 [details] [diff] [review]
> 897240.patch
> 
> Review of attachment 791117 [details] [diff] [review]:
> -----------------------------------------------------------------

I wanted to get some of the replies in to keep the discussion going. 
I'll address the rest of your concerns when I have a bit more time. 

> It seems that you replicate a lot of the data type and functionality of
> nsIScriptError in nsSecurityConsoleMessage.cpp, and of nsIConsoleService in
> nsSecurityConsoleService.cpp. Of the two, I think the nsIScriptError
> replication is more egregious. Is it possible to have
> nsISecurityConsoleMessage inherit from nsIScriptError, so you can avoid
> duplicating all of the same fields (and identical boilerplate code)? It
> looks like the only missing thing from nsIScriptError on
> nsSecurityConsoleMessage is the innerWindowID.
Yes, I was having the same exact thoughts, but there are two crucial differences from nsIScriptError. One is that nsIScriptError stores the localized string in nsIScriptError.errorMessage. Here, we keep only the lookup key, which is actually what makes this useful for the SecurityReportTool in the first place. I will elaborate further when answering you other questions. The other important difference is that unless we want to use the same errorMessage property from nsIScriptError to store the localized string in most scenarios and the lookup key in the case of the security console, then we have to keep these objects separated in the hierarchy. I don't want tole the usage of the property since that would just make things confusing. An ideal solution is to introduce a new idl, let's say nsIError that is a base interface for both nsIScriptError and nsISecurityConsoleMessage that contains all the common fields - line #, source etc. However, that might require even more global changes.  

> Where is the implementation of nsISecurityConsoleListener? Again, I feel
> like we might be able to reuse existing code to implement this, but could be
> wrong.
We don't need one, since I reuse the observer service. The mochitest is an example of how the client can register listeners. I shouldn't have included that particular commit in the patch. I apologize for the mishap. 

> Finally, I think it's important to check in with Kailas and make sure this
> will help his use case. AFAICT, this code reports a localized message string
> to the console, then sends that same message to an observer topic named
> "security-console-message-received". Is that really much of an improvement
> over the previous situation? Previously, he could monitor the
> nsIConsoleService and parse the message strings. Now he can monitor the
> "security-console-message-received" topic, and parse the message strings. It
> stills requires that fragile and annoying parsing step for all types of
> security errors, which I thought is what we were trying to avoid.
The nsISecurityConsoleMessage doesn't contain the localized string, but the lookup key. 
If you notice here: 

+  rv = getLocalizedMessage(&errorText, aMessage);

I use the message object to extract the localized errorText, but I send the non-localized object to the observer service: 
+    rv = sendObserverNotification(aMessage);

I can add some comments around sendObserverNotification to make that clearer.

Comment 8

[Garrett Robinson [:grobinson]]

(In reply to Ivan Alagenchev :ialagenchev from comment #7)
> (In reply to Garrett Robinson [:grobinson] from comment #5)
> > Comment on attachment 791117 [details] [diff] [review]
> > 897240.patch
> > 
> > Review of attachment 791117 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> I wanted to get some of the replies in to keep the discussion going. 
> I'll address the rest of your concerns when I have a bit more time. 
> 
> > It seems that you replicate a lot of the data type and functionality of
> > nsIScriptError in nsSecurityConsoleMessage.cpp, and of nsIConsoleService in
> > nsSecurityConsoleService.cpp. Of the two, I think the nsIScriptError
> > replication is more egregious. Is it possible to have
> > nsISecurityConsoleMessage inherit from nsIScriptError, so you can avoid
> > duplicating all of the same fields (and identical boilerplate code)? It
> > looks like the only missing thing from nsIScriptError on
> > nsSecurityConsoleMessage is the innerWindowID.
> Yes, I was having the same exact thoughts, but there are two crucial
> differences from nsIScriptError. One is that nsIScriptError stores the
> localized string in nsIScriptError.errorMessage. Here, we keep only the
> lookup key, which is actually what makes this useful for the
> SecurityReportTool in the first place. I will elaborate further when
> answering you other questions. The other important difference is that unless
> we want to use the same errorMessage property from nsIScriptError to store
> the localized string in most scenarios and the lookup key in the case of the
> security console, then we have to keep these objects separated in the
> hierarchy. I don't want tole the usage of the property since that would just
> make things confusing. An ideal solution is to introduce a new idl, let's
> say nsIError that is a base interface for both nsIScriptError and
> nsISecurityConsoleMessage that contains all the common fields - line #,
> source etc. However, that might require even more global changes.  
> 

Yeah, it is tricky. It might just be a necessary evil until we coerce some poor intern into completely rewriting Gecko's logging ;)

> > Where is the implementation of nsISecurityConsoleListener? Again, I feel
> > like we might be able to reuse existing code to implement this, but could be
> > wrong.
> We don't need one, since I reuse the observer service. The mochitest is an
> example of how the client can register listeners. I shouldn't have included
> that particular commit in the patch. I apologize for the mishap. 
> 

Got it, so that was left over.

> > Finally, I think it's important to check in with Kailas and make sure this
> > will help his use case. AFAICT, this code reports a localized message string
> > to the console, then sends that same message to an observer topic named
> > "security-console-message-received". Is that really much of an improvement
> > over the previous situation? Previously, he could monitor the
> > nsIConsoleService and parse the message strings. Now he can monitor the
> > "security-console-message-received" topic, and parse the message strings. It
> > stills requires that fragile and annoying parsing step for all types of
> > security errors, which I thought is what we were trying to avoid.
> The nsISecurityConsoleMessage doesn't contain the localized string, but the
> lookup key. 
> If you notice here: 
> 
> +  rv = getLocalizedMessage(&errorText, aMessage);
> 
> I use the message object to extract the localized errorText, but I send the
> non-localized object to the observer service: 
> +    rv = sendObserverNotification(aMessage);
> 
> I can add some comments around sendObserverNotification to make that clearer.

Ah, I see, you're sending the entire object as the subject. That's nice. It is different from Kaila's approach in Bug 890224, where he is using different topics for each type of error. This might be better. A thought - maybe you could pull the category off the SecurityConsoleMessage and use that as the topic? That might make Kaila's code nicer, but also might make things a little harder to reason about and could lead to observer naming conflicts. I'm still interested to hear what Kailas has to say.

Comment 9

[Ivan Alagenchev :ialagenchev]

(In reply to Garrett Robinson [:grobinson] from comment #8)
> (In reply to Ivan Alagenchev :ialagenchev from comment #7)
> > (In reply to Garrett Robinson [:grobinson] from comment #5)
> Yeah, it is tricky. It might just be a necessary evil until we coerce some
> poor intern into completely rewriting Gecko's logging ;)

:-)

> Got it, so that was left over.
Yep. I'll make sure to remove that. 
 
> Ah, I see, you're sending the entire object as the subject. That's nice. It
> is different from Kaila's approach in Bug 890224, where he is using
> different topics for each type of error. This might be better. A thought -
> maybe you could pull the category off the SecurityConsoleMessage and use
> that as the topic? That might make Kaila's code nicer, but also might make
> things a little harder to reason about and could lead to observer naming
> conflicts. I'm still interested to hear what Kailas has to say.

Interesting idea, though that would need an observer for each topic. It might be better to have one topic and have the observer switch the categories from the object. Let's see what Kailas has to say.

Comment 10

[Kailas]

(In reply to Garrett Robinson [:grobinson] from comment #6)
> ^^ Kailas, please see the last paragraph of the first part of my review.
> What do you think? Am I missing something here?

>observerService->NotifyObservers(aMessage,
> +      "security-console-message-received", NULL);

Yes, I agree with grobinson. 
The net effect of this observer will be same as nsIConsoleService, where I need to parse strings to identify error category. 
IMHO, error category should be sent in "aTopic" parameter of observer to make the identification of an error type easier.

Comment 11

[Kailas]

(In reply to Ivan Alagenchev :ialagenchev from comment #9)
> (In reply to Garrett Robinson [:grobinson] from comment #8)
> > (In reply to Ivan Alagenchev :ialagenchev from comment #7)
> > > (In reply to Garrett Robinson [:grobinson] from comment #5)
> > Yeah, it is tricky. It might just be a necessary evil until we coerce some
> > poor intern into completely rewriting Gecko's logging ;)
> 
> :-)
> 
> > Got it, so that was left over.
> Yep. I'll make sure to remove that. 
>  
> > Ah, I see, you're sending the entire object as the subject. That's nice. It
> > is different from Kaila's approach in Bug 890224, where he is using
> > different topics for each type of error. This might be better. A thought -
> > maybe you could pull the category off the SecurityConsoleMessage and use
> > that as the topic? That might make Kaila's code nicer, but also might make
> > things a little harder to reason about and could lead to observer naming
> > conflicts. I'm still interested to hear what Kailas has to say.
> 
> Interesting idea, though that would need an observer for each topic. It
> might be better to have one topic and have the observer switch the
> categories from the object. Let's see what Kailas has to say.

I agree with ialagenchev, IMHO, it might be better to have one topic and allow observer to switch categories using the "aTopic" object.

Comment 12

[Ivan Alagenchev :ialagenchev]

(In reply to Kailas from comment #10)
> (In reply to Garrett Robinson [:grobinson] from comment #6)
> > ^^ Kailas, please see the last paragraph of the first part of my review.
> > What do you think? Am I missing something here?
> 
> >observerService->NotifyObservers(aMessage,
> > +      "security-console-message-received", NULL);
> 
> Yes, I agree with grobinson. 
> The net effect of this observer will be same as nsIConsoleService, where I
> need to parse strings to identify error category. 
> IMHO, error category should be sent in "aTopic" parameter of observer to
> make the identification of an error type easier.

Hi Kailas,

As per my reply to grobinson, you won't have to parse strings to identify the error category. You would have to switch categories on a string lookup key, which doesn't get localized. Let me know if that makes sense, or if you would need some other capability.

Comment 13

[Kailas]

(In reply to Ivan Alagenchev :ialagenchev from comment #12)
> (In reply to Kailas from comment #10)
> > (In reply to Garrett Robinson [:grobinson] from comment #6)
> > > ^^ Kailas, please see the last paragraph of the first part of my review.
> > > What do you think? Am I missing something here?
> > 
> > >observerService->NotifyObservers(aMessage,
> > > +      "security-console-message-received", NULL);
> > 
> > Yes, I agree with grobinson. 
> > The net effect of this observer will be same as nsIConsoleService, where I
> > need to parse strings to identify error category. 
> > IMHO, error category should be sent in "aTopic" parameter of observer to
> > make the identification of an error type easier.
> 
> Hi Kailas,
> 
> As per my reply to grobinson, you won't have to parse strings to identify
> the error category. You would have to switch categories on a string lookup
> key, which doesn't get localized. Let me know if that makes sense, or if you
> would need some other capability.

Hi  Ivan Alagenchev,
For security report tool I need error category information without doing error string parsing.  Currently, messages (such as SSL) sent to browser console using nsIconsoleMessage doesn't contain category information. It might be better to have one topic only and message object of that topic provides error category information as well as error message. In this way, as you mentioned in comment #9, we don't need to have an observer for each topic. 
Another alternative would be observer service takes three parameters, so we trigger observer notification with parameters: "text error message", "unique-topic-id" and "error category". In this approach, we don't need an observer for each error category.

Comment 14

[Ivan Alagenchev :ialagenchev]

(In reply to Kailas from comment #13)
> "unique-topic-id" and "error category". In this approach, we don't need an
> observer for each error category.

Doesn't that mean that you would need an observer for each topic though?

Comment 15

[Kailas]

(In reply to Ivan Alagenchev :ialagenchev from comment #14)
> (In reply to Kailas from comment #13)
> > "unique-topic-id" and "error category". In this approach, we don't need an
> > observer for each error category.
> 
> Doesn't that mean that you would need an observer for each topic though?

No. We can use 3rd parameter to specify error category. Topic parameter will be same "security-console-message-received". 

For example:
observerService->NotifyObservers(aMessage,
         "security-console-message-received", "SSL");
observerService->NotifyObservers(aMessage,
         "security-console-message-received", "CSP");
observerService->NotifyObservers(aMessage,
         "security-console-message-received", "Mixed-content");

Comment 16

[Ivan Alagenchev :ialagenchev]

(In reply to Kailas from comment #15)
> (In reply to Ivan Alagenchev :ialagenchev from comment #14)
> > (In reply to Kailas from comment #13)
> > > "unique-topic-id" and "error category". In this approach, we don't need an
> > > observer for each error category.
> > 
> > Doesn't that mean that you would need an observer for each topic though?
> 
> No. We can use 3rd parameter to specify error category. Topic parameter will
> be same "security-console-message-received". 
> 
> For example:
> observerService->NotifyObservers(aMessage,
>          "security-console-message-received", "SSL");
> observerService->NotifyObservers(aMessage,
>          "security-console-message-received", "CSP");
> observerService->NotifyObservers(aMessage,
>          "security-console-message-received", "Mixed-content");

Hi Kailas, 

You can extract the category from the object itself. I am pretty convinced that the code I currently have should do what you need. If you want, I can update the patch to make sure it applies for you and you can test it out before I send it up for review.

Comment 17

[Kailas]

(In reply to Ivan Alagenchev :ialagenchev from comment #16)
> (In reply to Kailas from comment #15)
> > (In reply to Ivan Alagenchev :ialagenchev from comment #14)
> > > (In reply to Kailas from comment #13)
> > > > "unique-topic-id" and "error category". In this approach, we don't need an
> > > > observer for each error category.
> > > 
> > > Doesn't that mean that you would need an observer for each topic though?
> > 
> > No. We can use 3rd parameter to specify error category. Topic parameter will
> > be same "security-console-message-received". 
> > 
> > For example:
> > observerService->NotifyObservers(aMessage,
> >          "security-console-message-received", "SSL");
> > observerService->NotifyObservers(aMessage,
> >          "security-console-message-received", "CSP");
> > observerService->NotifyObservers(aMessage,
> >          "security-console-message-received", "Mixed-content");
> 
> Hi Kailas, 
> 
> You can extract the category from the object itself. I am pretty convinced
> that the code I currently have should do what you need. If you want, I can
> update the patch to make sure it applies for you and you can test it out
> before I send it up for review.

Hi Ivan Alagenchev,
IMHO, Use error category in the error message is a better approach. And that's what your patch is doing. So it is a good fix. +1

Comment 18

[Tanvi Vyas[:tanvi]]

Comment on attachment 791117 [details] [diff] [review]
897240.patch

Will review once Ivan has updated his patch.

Comment 19

[Ivan Alagenchev :ialagenchev]

(In reply to Garrett Robinson [:grobinson] from comment #5)
> Comment on attachment 791117 [details] [diff] [review]
> 897240.patch

> If we return early here, then we never call mErrors.Clear(). Is that a
> potential memory leak? I would put the for loop below in an if (console) { }
> block, and then you call .Clear() either way.

or is it OK to do the for loop as 
for (uint32_t i = 0; console && i < mErrors.Length(); i++) { ? to save the need for another nested block?

> 
> 
> ::: xpcom/base/nsISecurityConsoleService.idl
> @@ +5,5 @@
> > +
> > +#include "nsISupports.idl"
> > +#include "nsISecurityConsoleMessage.idl"
> > +
> > +interface nsIDocument;
> 
> Why is this necessary?
I think this was left over from when I used nsIDocument as parameter. Removed.


I fixed the rest of the issues too. Thank you for your feedback.

Comment 20

[Ivan Alagenchev :ialagenchev]

(In reply to Garrett Robinson [:grobinson] from comment #5)
> Comment on attachment 791117 [details] [diff] [review]
> 897240.patch
> 
> Review of attachment 791117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Makefile.in and nsMixedContentBlocker.cpp are bitrotted.
> 
> The browser chrome test looks fine, although I wonder if we don't need a new
> test here as much as we need to check the observer in all of the old tests.
> Since you change some of that code here, it might be better just to update
> those tests to also listen for the observer as well as checking the web
> console.

I thought about that too. I think it is OK to not add validation for the observer in the other tests, since we would be testing the same code path. One thing to consider is that if we start having a lot of tests failing because of one degraded functionality, that might make things harder to track down. It's also additional work that doesn't seem to add much value. I don't have a very strong opinion on this though, so I can be easily persuaded of the opposite if there is a compelling reason.

Comment 21

[Ivan Alagenchev :ialagenchev]

Attachment: 897240.patch

Comment 22

[Ivan Alagenchev :ialagenchev]

Attachment: 897240.patch

Comment 23

[Garrett Robinson [:grobinson]]

(In reply to Ivan Alagenchev :ialagenchev from comment #19)
> (In reply to Garrett Robinson [:grobinson] from comment #5)
> > Comment on attachment 791117 [details] [diff] [review]
> > 897240.patch
> 
> > If we return early here, then we never call mErrors.Clear(). Is that a
> > potential memory leak? I would put the for loop below in an if (console) { }
> > block, and then you call .Clear() either way.
> 
> or is it OK to do the for loop as 
> for (uint32_t i = 0; console && i < mErrors.Length(); i++) { ? to save the
> need for another nested block?
> 

I think that would be saving code at the expense of readability, and you should not do it. Additionally, it would incur a (tiny) performance cost, as you are now performing a check for console mErrors.Length() times, when you only need to do it once.

Comment 24

[Ivan Alagenchev :ialagenchev]

(In reply to Garrett Robinson [:grobinson] from comment #23)
> (In reply to Ivan Alagenchev :ialagenchev from comment #19)
> > (In reply to Garrett Robinson [:grobinson] from comment #5)
> I think that would be saving code at the expense of readability, and you
> should not do it. Additionally, it would incur a (tiny) performance cost, as
> you are now performing a check for console mErrors.Length() times, when you
> only need to do it once.

Yeah, you are right. I'll use the if-statement.

Comment 25

[Kailas]

ialagenchev:  To display SSL error messages that do not contain innerwindowID, if I want to use nsISecurityConsoleMessage interface, Can i use aInnerWindowID value set to 0 or will it create a problem.

Comment 26

[Ivan Alagenchev :ialagenchev]

(In reply to Kailas from comment #25)
> ialagenchev:  To display SSL error messages that do not contain
> innerwindowID, if I want to use nsISecurityConsoleMessage interface, Can i
> use aInnerWindowID value set to 0 or will it create a problem.

Based on my interpretation of the code in nsContentUtils, you should be fine. 
If your time permits, it might be a good idea to download the patch and test it out locally. This way, if there is something that this doesn't provide you with I can actually roll it in as part of this patch and you won't have to wait for another cycle of reviews in another bug.

Comment 27

[Ivan Alagenchev :ialagenchev]

(In reply to Ivan Alagenchev :ialagenchev from comment #26)
> (In reply to Kailas from comment #25)

Just to remind you that if you decided to do that, you will need to apply the patches for bug 875456 first, since the code here is dependent on those changes. That is unless bug 875456 lands before you start testing this one out.

Comment 28

[Ivan Alagenchev :ialagenchev]

Attachment: 897240_DOM.patch

Comment 29

[Ivan Alagenchev :ialagenchev]

Attachment: 897240_NETWERK.patch

Comment 30

[Ivan Alagenchev :ialagenchev]

Attachment: 897240_TOOLKIT.patch

Comment 31

[Ivan Alagenchev :ialagenchev]

Attachment: 897240_DEVTOOLS.patch

Comment 32

[Ivan Alagenchev :ialagenchev]

Attachment: 897240_XPCOM.patch

Comment 33

[Kailas]

(In reply to Ivan Alagenchev :ialagenchev from comment #27)
> (In reply to Ivan Alagenchev :ialagenchev from comment #26)
> > (In reply to Kailas from comment #25)
> 
> Just to remind you that if you decided to do that, you will need to apply
> the patches for bug 875456 first, since the code here is dependent on those
> changes. That is unless bug 875456 lands before you start testing this one
> out.

I downloaded and applied patches (875456_DEVTOOLS.patch, 875456_DOM.patch, 897240_DOM.patch, 897240_NETWERK.patch , 897240_TOOLKIT.patch, 897240_DEVTOOLS.patch, and 897240_XPCOM.patch ). 

To test these patches I used a webpage with mixed-cotnent: https://people.mozilla.com/~mgoodwin/mixed

Output: Mixed-content error message was not reported in the Browser console. 

Is it an expected outcome or I might have made a mistake in my test environment? 

To further test this: 
I used this code: http://pastebin.mozilla.org/2908653
and tested using scratchpad and terminal dump messages.

Terminal dump messages show an error Category and a lookupKey. I don't understand the purpose of "lookupKey" though. But, I was not able to display errorText message, So I checked "xpcom/base/nsISecurityConsoleMessage.idl" file, but could not figure out which one is errorText message sent to console. 
  Or 
"nsISecurityConsoleMessage.idl" doesn't contain error strings that previously used to display in Browser Console.

Comment 34

[Ivan Alagenchev :ialagenchev]

(In reply to Kailas from comment #33)
> (In reply to Ivan Alagenchev :ialagenchev from comment #27)
> > (In reply to Ivan Alagenchev :ialagenchev from comment #26)
> > > (In reply to Kailas from comment #25)
> > 
> > Just to remind you that if you decided to do that, you will need to apply
> > the patches for bug 875456 first, since the code here is dependent on those
> > changes. That is unless bug 875456 lands before you start testing this one
> > out.
> 
> I downloaded and applied patches (875456_DEVTOOLS.patch, 875456_DOM.patch,
> 897240_DOM.patch, 897240_NETWERK.patch , 897240_TOOLKIT.patch,
> 897240_DEVTOOLS.patch, and 897240_XPCOM.patch ). 
> 
> To test these patches I used a webpage with mixed-cotnent:
> https://people.mozilla.com/~mgoodwin/mixed
> 
> Output: Mixed-content error message was not reported in the Browser console. 
> 
> Is it an expected outcome or I might have made a mistake in my test
> environment? 
I don't know to be honest. I get this message: [11:08:21.282] Blocked loading mixed active content "http://people.mozilla.org/~mgoodwin/mixed.js"
When I load the page, so it appears to work fine for me. 

> To further test this: 
> I used this code: http://pastebin.mozilla.org/2908653
> and tested using scratchpad and terminal dump messages.
> 
> Terminal dump messages show an error Category and a lookupKey. I don't
> understand the purpose of "lookupKey" though. 
The lookup key is what we discussed above that you will use to determine what type of security message you are getting. It's the key used to lookup in the localization files to get the localized strings for the error messages. 

But, I was not able to display
> errorText message, So I checked "xpcom/base/nsISecurityConsoleMessage.idl"
> file, but could not figure out which one is errorText message sent to
> console. 
>   Or 
> "nsISecurityConsoleMessage.idl" doesn't contain error strings that
> previously used to display in Browser Console.
I don't see you dumping that in your code, but you shouldn't care about that anyways. That's the localized string. 

Also, please send me an email if you have questions next time. Let's not pollute this bug with unrelated discussions.

Comment 35

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 794943 [details] [diff] [review]
897240_DOM.patch

Jonas told you could do this so he gets to review.

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 794954 [details] [diff] [review]
897240_XPCOM.patch

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

::: xpcom/base/nsISecurityConsoleMessage.idl
@@ +16,2 @@
>    attribute AString category;
> +  attribute StringArray params;

This is terrible.  This can't be sanely used from JS, for one.  It's also totally unclear what the memory ownership model is here.

@@ +23,5 @@
> +
> +  void init(in AString lookupKey, in AString category, in AString sourceLine,
> +            in AString sourceName, in uint32_t lineNumber,
> +            in uint32_t columnNumber, in StringArray params,
> +            in uint32_t paramsLength);

And init also can't be used from JS because of StringArray.  If this doesn't need to be used from JS then I think it shouldn't be on the interface at all.  Put it on the concrete C++ type instead.

::: xpcom/base/nsSecurityConsoleMessage.cpp
@@ +7,5 @@
>  NS_IMPL_ISUPPORTS1(nsSecurityConsoleMessage, nsISecurityConsoleMessage)
>  
>  nsSecurityConsoleMessage::nsSecurityConsoleMessage()
>  {
> +  mParams = NULL;

This doesn't actually free mParams ... it just leaks it.

Comment 37

[Ivan Alagenchev :ialagenchev]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #36)
> Comment on attachment 794954 [details] [diff] [review]
> 897240_XPCOM.patch
> 
> Review of attachment 794954 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/base/nsISecurityConsoleMessage.idl
> @@ +16,2 @@
> >    attribute AString category;
> > +  attribute StringArray params;
> 
> This is terrible.  This can't be sanely used from JS, for one.  It's also
> totally unclear what the memory ownership model is here.
OK. Could you be more specific and perhaps help me get this to a point where it's acceptable for you? Is there a way that we can create something that can be generic and potentially be usable by .js? This particular parameter isn't used by js right now, but I can see that logging formatted strings from .js is something that will be needed.

> @@ +23,5 @@
> > +
> > +  void init(in AString lookupKey, in AString category, in AString sourceLine,
> > +            in AString sourceName, in uint32_t lineNumber,
> > +            in uint32_t columnNumber, in StringArray params,
> > +            in uint32_t paramsLength);
> 
> And init also can't be used from JS because of StringArray.  If this doesn't
> need to be used from JS then I think it shouldn't be on the interface at
> all.  Put it on the concrete C++ type instead.
I want to be able to expose this to js if it's needed at some point. How can I go about it?

> ::: xpcom/base/nsSecurityConsoleMessage.cpp
> @@ +7,5 @@
> >  NS_IMPL_ISUPPORTS1(nsSecurityConsoleMessage, nsISecurityConsoleMessage)
> >  
> >  nsSecurityConsoleMessage::nsSecurityConsoleMessage()
> >  {
> > +  mParams = NULL;
> 
> This doesn't actually free mParams ... it just leaks it.
How do I rectify that? Do I add nsMemory::Free in the destructor?

Comment 38

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

bsmedberg, can you answer Ivan's question about what the "correct" way to do string array parameters/attributes is in xpidl?

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 794943 [details] [diff] [review]
897240_DOM.patch

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

I won't have time to get to this very soon. Could you find another DOM peer?

Comment 40

[Ivan Alagenchev :ialagenchev]

back to khuey

Comment 41

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 794943 [details] [diff] [review]
897240_DOM.patch

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

::: content/base/src/nsDocument.cpp
@@ +2472,5 @@
> +    for (uint32_t i = 0; i < mErrors.Length(); i++) {
> +      nsCOMPtr<nsISecurityConsoleMessage> message(
> +          do_CreateInstance(NS_SECURITY_CONSOLE_MESSAGE_CONTRACTID));
> +      nsAutoString messageLookupKey;
> +      messageLookupKey.Assign(NS_ConvertUTF8toUTF16(mErrors[i]));

NS_ConvertUTF8toUTF16 messageLookupKey(mErrors[i]);

@@ +2489,5 @@
> +  nsCOMPtr<nsISecurityConsoleService> console(
> +      do_GetService(NS_SECURITY_CONSOLESERVICE_CONTRACTID));
> +  if(!console) {
> +    return;
> +  }

if (

::: content/base/src/nsMixedContentBlocker.cpp
@@ +162,5 @@
> +  nsCOMPtr<nsISecurityConsoleService> console(
> +      do_GetService(NS_SECURITY_CONSOLESERVICE_CONTRACTID));
> +  if(!console) {
> +    return;
> +  }

here too

Comment 42

[Ivan Alagenchev :ialagenchev]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #41)
> Comment on attachment 794943 [details] [diff] [review]
> 897240_DOM.patch
> 
> Review of attachment 794943 [details] [diff] [review]:
> -----------------------------------------------------------------
Thanks for the r+. I'll make those changes.

Comment 43

[Ivan Alagenchev :ialagenchev]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #36)
> Comment on attachment 794954 [details] [diff] [review]
> 897240_XPCOM.patch
> 
> Review of attachment 794954 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: xpcom/base/nsSecurityConsoleMessage.cpp
> @@ +7,5 @@
> >  NS_IMPL_ISUPPORTS1(nsSecurityConsoleMessage, nsISecurityConsoleMessage)
> >  
> >  nsSecurityConsoleMessage::nsSecurityConsoleMessage()
> >  {
> > +  mParams = NULL;
> 
> This doesn't actually free mParams ... it just leaks it.

I looked at this closer and I am not sure that there is a leak here. 
mParams corresponds to strings in Init:

message->Init(messageLookupKey, messageCategory, NS_LITERAL_STRING(""),
+                NS_LITERAL_STRING(""), 0, 0, strings, ArrayLength(strings));

Which was on the stack:
const PRUnichar* strings[] = { locationSpecUTF16.get() };

Am I missing something?

Comment 44

[Doug Turner (:dougt)]

Comment on attachment 794954 [details] [diff] [review]
897240_XPCOM.patch

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

per khuey's comments.

Comment 45

[Doug Turner (:dougt)]

Comment on attachment 794952 [details] [diff] [review]
897240_TOOLKIT.patch

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

::: toolkit/components/passwordmgr/InsecurePasswordUtils.jsm
@@ +45,3 @@
>  
> +    let consoleService = Cc["@mozilla.org/securityconsole/service;1"]
> +      .createInstance(Ci.nsISecurityConsoleService);

.getService() instead, right?

Comment 46

[Doug Turner (:dougt)]

-    Services.console.logMessage(consoleMsg);
+    let consoleService = Cc["@mozilla.org/securityconsole/service;1"]
+      .createInstance(Ci.nsISecurityConsoleService);
+    consoleService.logMessage(consoleMsg, windowId);

It would be really nice if you added:

Services.securityConsole.logMessage()  Or something similar.

Comment 47

[Benjamin Smedberg]

There isn't a good way to do arrays of strings in XPIDL. You can use [array, size_is] wstring but you have to pass the length separately, so it's not very JS-y.

The "good" way is to use jsval; I suspect that there is a helper method somewhere, but I don't know of one in particular.

Comment 48

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

There is nsTArrayToJSArray in nsTArrayHelpers.h.

Comment 49

[Garrett Robinson [:grobinson]]

(In reply to Ivan Alagenchev :ialagenchev from comment #43)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #36)
> > Comment on attachment 794954 [details] [diff] [review]
> > 897240_XPCOM.patch
> > 
> > Review of attachment 794954 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > ::: xpcom/base/nsSecurityConsoleMessage.cpp
> > @@ +7,5 @@
> > >  NS_IMPL_ISUPPORTS1(nsSecurityConsoleMessage, nsISecurityConsoleMessage)
> > >  
> > >  nsSecurityConsoleMessage::nsSecurityConsoleMessage()
> > >  {
> > > +  mParams = NULL;
> > 
> > This doesn't actually free mParams ... it just leaks it.
> 
> I looked at this closer and I am not sure that there is a leak here. 
> mParams corresponds to strings in Init:
> 
> message->Init(messageLookupKey, messageCategory, NS_LITERAL_STRING(""),
> +                NS_LITERAL_STRING(""), 0, 0, strings, ArrayLength(strings));
> 
> Which was on the stack:
> const PRUnichar* strings[] = { locationSpecUTF16.get() };
> 
> Am I missing something?

Yes - as khuey pointed out prevoiusly, it is unclear what the memory ownership model is here (I'm not even sure there is one). In this case, your object constructor is assigning a member variable of type PRUnichar** to a stack-allocated pointer in the calling function of the constructor. These pointers become invalid when the function returns (they may still point to the data you expect, but this is accidental and not reliable).

Roughly, you need a memory ownership module in which the nsSecurityConsoleMessage dynamically allocates the memory it needs to stores these strings on construction, and frees it on destruction. I am not sure what the best actual construction would be - perhaps an nsTArray of nsStrings? If you insist on passing PRUnichar** as a parameter to the constructor, you will need to *copy* the strings (make sure they're null-terminated!) into your object's member variable. You will also need to do some temporary conversion when you call ReportToConsole, since it expects PRUnichar** (I know that is why you are using it in the first place).

IMHO, code like "mParams = NULL" is generally only good for hiding memory bugs.

Sorry I didn't catch this in my initial review.

Comment 50

[Mihai Sucan [:msucan]]

Comment on attachment 794953 [details] [diff] [review]
897240_DEVTOOLS.patch

I am not sure about the actual necessity for a new nsISecurityConsoleService and observer notifications for security-console-messages. However, I see you've discussed this with Gecko people and they seem to have agreed.

The test from this patch checks that the observer notification works. It is not clear to me if this test shouldn't be in a different folder? This is not a web console test. Maybe tests for these new notifications should be closer to the new idl - xpcom/base or somewhere in toolkit? Do you plan to also change the web console actors to use these new notifications?

Thank you!

Comment 51

[Ivan Alagenchev :ialagenchev]

(In reply to Mihai Sucan [:msucan] from comment #50)
> Comment on attachment 794953 [details] [diff] [review]
> 897240_DEVTOOLS.patch
> 
> I am not sure about the actual necessity for a new nsISecurityConsoleService
> and observer notifications for security-console-messages. However, I see
> you've discussed this with Gecko people and they seem to have agreed.
> 
> The test from this patch checks that the observer notification works. It is
> not clear to me if this test shouldn't be in a different folder? This is not
> a web console test. Maybe tests for these new notifications should be closer
> to the new idl - xpcom/base or somewhere in toolkit? Do you plan to also
> change the web console actors to use these new notifications?
> 
> Thank you!

Hi Mihai, thanks for your review. The reason why we are introducing a new security related service is that the messages sent by the console service are unreliable when localization is used. With the current design, the security report tool would have to do string matching on the message in order to determine what type of error is being reported. This code aims to change that. 

Thanks for pointing out that the test is in the wrong folder. I will change that. 

I don't know enough to answer your last question. What are the web console actors and what are they used for?

Comment 52

[Mihai Sucan [:msucan]]

(In reply to Ivan Alagenchev :ialagenchev from comment #51)
> Hi Mihai, thanks for your review. The reason why we are introducing a new
> security related service is that the messages sent by the console service
> are unreliable when localization is used. With the current design, the
> security report tool would have to do string matching on the message in
> order to determine what type of error is being reported. This code aims to
> change that.
>
> Thanks for pointing out that the test is in the wrong folder. I will change
> that. 

Thank you for the explanation!

> I don't know enough to answer your last question. What are the web console
> actors and what are they used for?

The web console actors are the server-side pieces of code that listen to all of the kinds of messages that the web console can display. They do the network monitoring, and they have the nsIConsoleService listeners, etc. I was interested if you plan to use these new notifications with the web console or if these are meant only for the security report tool?

Thanks!

Comment 53

[Ivan Alagenchev :ialagenchev]

(In reply to Mihai Sucan [:msucan] from comment #52)
> (In reply to Ivan Alagenchev :ialagenchev from comment #51)
> was interested if you plan to use these new notifications with the web
> console or if these are meant only for the security report tool?
> 
> Thanks!

As of right now, we plan to use this only with the security report tool and not make modifications to other listeners.

Comment 54

[Ivan Alagenchev :ialagenchev]

quick update regarding the inactivity - I will be busy with school work for the next couple of weeks and will get back to trying to resolve this one afterwards.

Comment 55

[Ivan Alagenchev :ialagenchev]

Continuing on the discussion started in https://bugzilla.mozilla.org/show_bug.cgi?id=897240#c36
and continued at https://bugzilla.mozilla.org/show_bug.cgi?id=897240#c37, https://bugzilla.mozilla.org/show_bug.cgi?id=897240#c38 and https://bugzilla.mozilla.org/show_bug.cgi?id=897240#c47

If I change the parameters to the Init method to JSVal, I would have to convert the array of strings to JSval every time when I need to call the Init method from cpp. This means strings would have to be converted to JSStrings and then using JS_NewArrayObject to convert the array to JSObject, which then would have to be converted to JSValue. This sounds like a terrible thing to do every time we need to log something from cpp. Should we have one native Init method for cpp and a separate scriptable method for js?

Either way, coming up with a solution that would work is way over my knowledge of all of the components and I would need some guidance as to what is the right approach before trying to finish this off. Any help is appreciated.

Comment 56

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 6 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Comment 57

[Alexandre Poirot [:ochameau]]

The feature behind this patch queue has been dropped (bug 811878).