Closed Bug 1096908 Opened 6 years ago Closed 5 years ago

[e10s] Security messages do not display in web console

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(e10s+, firefox40 fixed)

VERIFIED FIXED
Firefox 40
Tracking Status
e10s + ---
firefox40 --- fixed

People

(Reporter: mgoodwin, Assigned: tromey)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [e10s-m6][testday-20150710])

Attachments

(1 file, 5 obsolete files)

Issue:
Security errors (HSTS, HPKP, Weak signature algorithm warnings) don't show in e10s windows.

STR:
1) Open an e10s window
2) Navigate to a page that will give a security message in the web console (e.g. this one: https://people.mozilla.org/~mgoodwin/error/ )
3) Observe the lack of errors in the web console

Expected results:
The message "The site specified an invalid Strict-Transport-Security header." should appear in the web console. See the same STR for a non-e10s window for an example of this.
The problem here is that security messages for HTTP channels are generated by nsHttpChannel (which is in the parent), while the code that interacts with them comes from nsDocument, which is in the child (see the call to TakeAllSecurityMessages).
(In reply to Josh Matthews [:jdm] from comment #1)
> The problem here is that security messages for HTTP channels are generated
> by nsHttpChannel (which is in the parent), while the code that interacts
> with them comes from nsDocument, which is in the child (see the call to
> TakeAllSecurityMessages).

in which case, the fix for this should also take into account bug 1092055 - since the issue in that case is that nsDocument doesn't also get the messages from other channels.
devtools bugs will block the e10s merge to Aurora, but blassey would like them to be tracked by dte10s meta bug 875871, not the tracking-e10s=m6+ flag.
Whiteboard: [e10s-m6]
Duplicate of this bug: 1103556
Gozala said he'd work on this
Assignee: nobody → rFobic
See http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/docs/lazy-actor-modules.md
We need something similar to what has been done for network monitor,
but we now have two helpers to do parent<->child communication.
I imagine we would be using DebuggerServer.setupInParent() to add some hook into the parent process listening for these HTTP errors and pipe them back to the child process.
You can see usage example of this method in patch from bug 980481.
There is no usage of it yet in m-c, but ideally netmon would be refactored to use that instead of hacking into the guts of DebuggerServer.
Ok it took me a while to figure out what is going with webconsole code so I'm going to post my findings here:

- It looks like same webconsole.js actor is loaded in both chrome and content processes, although web console client seems to be talking to one in child process.
- When webconsole.js is running in a child process it uses `NetworkMonitorChild` actor for handling network events. 
- `NetworkMonitorChild` seems to be interacting with `NetworkMonitorManager` actor that is loaded in the chrome process, which in turn loads `NetworkMonitor` actor (in chrome process) and pipes messages from `chrome` to `content` process to  `NetworkMonitorChild` which will then forward them to a web-console client. (It seems like we do chrome -> content -> chrome messaging, which seems odd).
- Further investigation led me realise that it was not network monitor to be blamed for missing security message, but rather a `ConsoleServiceListener`. As far as I can tell from code https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/webconsole.js#L533-L540 it does not handle a case of being loaded in the content process unlike network activity does.
- Given all the fact that observer notification is dispatched in the chrome process, it makes sense that above quoted code won't handle those notifications.

Now this brings me to few open questions & some potential solutions:

- In case of network activity all events happen in chrome process, I wonder if that is the case with a ConsoleServiceListener or if some notifications will be dispatched in chrome process while other in content process ?

- I think there are two possible solutions:

  1. Implement a flow similar to network monitor, by which I mean implement a manager similar to
     `NetworkMonitorManager` that would activate `ConsoleServiceListener` in the chrome process
     on message from a `ConsoleServiceListenerChild` in the child process & pipe all messages back
     to it.

     I'm not super excited with a prospect as it basically replicates network activity setup with no
     code sharing & just seem odd and complicated. While asking questions about it on IRC I was told it's
     "really strange" so seem awkward to repeat same strange thing over.

  2. Implement what ochameau suggested above, which I believe is kind of same as option 1. it's just
     we could reuse building blocks described in the .md file to avoid `NetworkMonitorManager`. Although
     to be honest I'm still not clear on how `ConsoleServiceListener` in the child process could load an
     actor in the chrome process. Also as far as I understand scope of this would be to rewrite a
     `ConsoleServiceListener`.
  
  3. Send those error notifications directly to a console, avoiding the whole message piping loop
     altogether. I thin this seem like the simplest and most logical approach & if I understand
     what I see correctly it's already being pursued in Bug 1092055.
(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #7)
>   3. Send those error notifications directly to a console, avoiding the
> whole message piping loop
>      altogether. I thin this seem like the simplest and most logical
> approach & if I understand
>      what I see correctly it's already being pursued in Bug 1092055.


This approach is fine for subresources since we can get valid window IDs from the LoadInfo on the channel. For top-level loads, we have the problem that the window that's loading the document is not the window that the document will eventually live in (which is why bug 1092055 leaves the existing stuff intact). If we can overcome this, this would be my preferred approach too.
(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #7)
> Ok it took me a while to figure out what is going with webconsole code so
> I'm going to post my findings here:
> 
> - It looks like same webconsole.js actor is loaded in both chrome and
> content processes, although web console client seems to be talking to one in
> child process.
> - When webconsole.js is running in a child process it uses
> `NetworkMonitorChild` actor for handling network events. 
> - `NetworkMonitorChild` seems to be interacting with `NetworkMonitorManager`
> actor that is loaded in the chrome process, which in turn loads
> `NetworkMonitor` actor (in chrome process) and pipes messages from `chrome`
> to `content` process to  `NetworkMonitorChild` which will then forward them
> to a web-console client. (It seems like we do chrome -> content -> chrome
> messaging, which seems odd).
> - Further investigation led me realise that it was not network monitor to be
> blamed for missing security message, but rather a `ConsoleServiceListener`.
> As far as I can tell from code
> https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/
> actors/webconsole.js#L533-L540 it does not handle a case of being loaded in
> the content process unlike network activity does.
> - Given all the fact that observer notification is dispatched in the chrome
> process, it makes sense that above quoted code won't handle those
> notifications.
> 
> Now this brings me to few open questions & some potential solutions:
> 
> - In case of network activity all events happen in chrome process, I wonder
> if that is the case with a ConsoleServiceListener or if some notifications
> will be dispatched in chrome process while other in content process ?
> 
> - I think there are two possible solutions:
> 
>   1. Implement a flow similar to network monitor, by which I mean implement
> a manager similar to
>      `NetworkMonitorManager` that would activate `ConsoleServiceListener` in
> the chrome process
>      on message from a `ConsoleServiceListenerChild` in the child process &
> pipe all messages back
>      to it.
> 
>      I'm not super excited with a prospect as it basically replicates
> network activity setup with no
>      code sharing & just seem odd and complicated. While asking questions
> about it on IRC I was told it's
>      "really strange" so seem awkward to repeat same strange thing over.
> 
>   2. Implement what ochameau suggested above, which I believe is kind of
> same as option 1. it's just
>      we could reuse building blocks described in the .md file to avoid
> `NetworkMonitorManager`.

Exactly.

> Although to be honest I'm still not clear on how `ConsoleServiceListener` in the
> child process could load an
>      actor in the chrome process. Also as far as I understand scope of this
> would be to rewrite a
>      `ConsoleServiceListener`.

We are not loading actors in the parent process, but just loading some js file and communicate with it.

   DebuggerServer.setupInParent({
     module: "devtools/server/actors/director-registry",
     setupParent: "setupParentProcess"
   });

is going to load directory-registry module and call its setupParentProcess method.
Then you can communicate with this module with message manager messages.

>   3. Send those error notifications directly to a console, avoiding the
> whole message piping loop
>      altogether.

If you do that, messages are only going to be visible in the main process console, right?
On b2g, you need to enable unrestricted devtools permissions to get access to this window...
And to enable these permissions you need to wipe your data.
Also if these errors relates to the child process, it is all but handy to have to open the main process console to see them... Note that on webide, you can only see one console at a time so that you would have to juggle between two toolboxes.
So given that 3. did not resolved the issue & I'm not sure how outlined path can be used to resolve it I have started implementing option 2. But I've run into several unknowns, I'm gonna post them here in a hope to get some guidance from ppl more familiar with our devtools actors & e10s stuff:

1. Do messages observed by ConsoleAPIListener are dispatched only either in chrome or a content process or both ? In case it's both that conversion of ConsoleAPIListener to E10S compatible actor becomes more difficult as it would have to aggregate messages from both processes.
2. Can console API messages dispatched via observer notifications be serialised, as that would be necessary to pipe them over message manager as suggested by @ochameau or the way network activity monitor does it. If they can not be, I feel like we would need some sort of object grip implementation between chrome & content processes.
3. Document pointed out by @ochameau show how to register specific module as a global or a tab actor, but it's not yet clear for me how converted `ConsoleAPIListener` actor could be instantiated from the webconsole actor. I imagine there are hooks for that, it's just but I'm not too familiar with the whole actor stuff yet.
Flags: needinfo?(poirot.alex)
Flags: needinfo?(past)
Flags: needinfo?(ejpbruel)
I am gonna reply my own questions with what I have found out to be an answers:

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #10)
> So given that 3. did not resolved the issue & I'm not sure how outlined path
> can be used to resolve it I have started implementing option 2. But I've run
> into several unknowns, I'm gonna post them here in a hope to get some
> guidance from ppl more familiar with our devtools actors & e10s stuff:
> 
> 1. Do messages observed by ConsoleAPIListener are dispatched only either in
> chrome or a content process or both ? In case it's both that conversion of
> ConsoleAPIListener to E10S compatible actor becomes more difficult as it
> would have to aggregate messages from both processes.

At the moment I do know for the fact that some messages are dispatched on the chrome process, which is why `ConsoleAPIListener` does not picks them up in e10s mode. I was unable to find out if messages are dispatched in the content process as well, but I'll assume so given that in e10s mode ConsoleAPIListener is actually observes for messages in the content process.


> 2. Can console API messages dispatched via observer notifications be
> serialised, as that would be necessary to pipe them over message manager as
> suggested by @ochameau or the way network activity monitor does it. If they
> can not be, I feel like we would need some sort of object grip
> implementation between chrome & content processes.

Given that current implementation sends down grips to the client it's likely that messages
can not be serialised. I also learned that apparently I could create an actor in the content
process and system is smart enough to route messages to the content or chrome process. There
for my current plan is to create a valueGrips and forward those instead. Unfortunately this
raises another question:

Which actor pool should valueGrip actors be put in if message happens on the chrome side (in the
content process web-console actor's pool can be used as it is now).

> 3. Document pointed out by @ochameau show how to register specific module as
> a global or a tab actor, but it's not yet clear for me how converted
> `ConsoleAPIListener` actor could be instantiated from the webconsole actor.
> I imagine there are hooks for that, it's just but I'm not too familiar with
> the whole actor stuff yet.

It looks like `ConsoleAPIListener` does not really needs to be converted to an actor to make use of
the apis described in the document. `ConsoleAPIListener` could just be extended to make use of the
new APIs described in the document.

With that only thing that stands in my way is to figure out how to handle ownership of value grips. If anyone have answers please post them here, otherwise I'll keep looking.
I don't know for sure that observer notifications can be serialized, but I can't see why the couldn't. At the end of the day what we really want from the code running in the main process is to transfer some strings or numbers back to the child process, so that the whole client-to-child-actor machinery can work as expected. So if automatic serialization fails for some reason, let's do the dumbest cloning in the world and be done with it.

About grips ([1]), they don't need to be stored in a pool, unless they refer to actual objects the client needs to interact with, in which case they are grips of object actors. In this case we are just dealing with strings (and maybe numbers). If for some reason that I can't see we do need to use object actors, then I would expect them to reside on the child process. Actors and their pools are facilities for easy lifecycle management of related objects, so that when we no longer care about an actor, we can safely destroy all of its pools (with dependent actors) in one shot. Since you won't be creating any actors in the main process, the only place that can naturally hold these relationships is the child.

[1]: https://wiki.mozilla.org/Remote_Debugging_Protocol#Grips
Flags: needinfo?(past)
Given the :past's response I'll assume that observer notifications received are JSON's so I'll be using message manager to forward them from chrome process to content process.
I've run into few more unknowns, here is an overview:

So web-console actor uses `ConsoleAPIListener` to observe `console-api-log-event` API notifications in the content process. To fix this bug, it will also start listening to a same observer notifications in the chrome process and forwarding those notifications to a `ConsoleAPIListener` (running in the content process).

ConsoleAPIListener's checks if observer notification is relevant to a web console instance by checking if
`innerWindowID` of the observer notification message target is with in the tree of the window that console instance is attached to. To do so it get's window from innerWindowID and walks up a parent chain until it reaches top or a window that matches the one that console is attached to:
https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/webconsole/utils.js#L1440-L1459

Now if we don't want to pipe every notification from chrome to content (which will forward each message to every `ConsoleAPIListener` instance, not just chrome process to content process) and then filter those in the content process we'll need to find a way if `innerWindowID` message is under a window that console is attached to in the chrome process. But only thing we could do is pass `outerWindowID` of the `ConsoleAPIListener`'s target window to the chrome process, but then I don't believe there is a way to check if window in content process with `innerWindowID` is under the window with `outerWindowID` in the chrome process.

I can only imagine :bz having some idea if that is somehow possible in the chrome process.

If filtering in chrome process turns out to be impossible, I'm back to the point where I think that it would be best to make platform changes to dispatch observer notifications in the content process itself, because otherwise we will end up manually piping all those message using message managers.
Flags: needinfo?(bzbarsky)
I've run into few more unknowns, here is an overview:

So web-console actor uses `ConsoleAPIListener` to observe `console-api-log-event` API notifications in the content process. To fix this bug, it will also start listening to a same observer notifications in the chrome process and forwarding those notifications to a `ConsoleAPIListener` (running in the content process).

ConsoleAPIListener's checks if observer notification is relevant to a web console instance by checking if
`innerWindowID` of the observer notification message target is with in the tree of the window that console instance is attached to. To do so it get's window from innerWindowID and walks up a parent chain until it reaches top or a window that matches the one that console is attached to:
https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/webconsole/utils.js#L1440-L1459

Now if we don't want to pipe every notification from chrome to content (which will forward each message to every `ConsoleAPIListener` instance, not just chrome process to content process) and then filter those in the content process we'll need to find a way if `innerWindowID` message is under a window that console is attached to in the chrome process. But only thing we could do is pass `outerWindowID` of the `ConsoleAPIListener`'s target window to the chrome process, but then I don't believe there is a way to check if window in content process with `innerWindowID` is under the window with `outerWindowID` in the chrome process.

I can only imagine :bz having some idea if that is somehow possible in the chrome process.

If filtering in chrome process turns out to be impossible, I'm back to the point where I think that it would be best to make platform changes to dispatch observer notifications in the content process itself, because otherwise we will end up manually piping all those message using message managers.
Can we not have the chrome process maintain the set of inner window ids for windows under a given root content process window?  The content process will obviously need to send messages to the chrome process when windows are added/removed in that window tree so the set can be maintained.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #16)
> Can we not have the chrome process maintain the set of inner window ids for
> windows under a given root content process window?  The content process will
> obviously need to send messages to the chrome process when windows are
> added/removed in that window tree so the set can be maintained.

I guess we could listen for `content-document-global-created ` and `inner-window-destroyed` observer notifications in the content process and send messages to chrome process to maintain a tree structure
of `innerWindowID`'s which then could be used in chrome process to implement `isOwnerOf(parentInnerID, childInnerID)`. Although I'm not sure how to deal with a fact that `innerWindowID` of the window console is attached to could change by navigating away. I guess that could be observed as well somehow and then communicated to an observer in the chrome process to update the `innerWindowID` it's tracking notifications for.

Does that sounds like an adequate solution though ? Would not it make more sense to just dispatch observer
notifications in the chrome process instead ?
Flags: needinfo?(bzbarsky)
Dispatch which observer notifications in the parent process?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #18)
> Dispatch which observer notifications in the parent process?


Quoting a comment from :jdm which is a source of the problem:

(In reply to Josh Matthews [:jdm] from comment #1)
> The problem here is that security messages for HTTP channels are generated
> by nsHttpChannel (which is in the parent), while the code that interacts
> with them comes from nsDocument, which is in the child (see the call to
> TakeAllSecurityMessages).

So my understanding is that something in the chrome process calls nsIConsoleService.logMessage
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIConsoleService#logMessage()

If it would somehow do that in the content process on behalf of the document associated
with a nsIChannel all of the issues will be resolved.
Flags: needinfo?(bzbarsky)
Ah, I see.  Well, one option would be to change HttpBaseChannel::AddSecurityMessage to ship the message to the content process if we're the parent-side reflection of a content channel.  The thing is, that would then involve shipping the message right back to actually show it in the console, right?
Flags: needinfo?(bzbarsky)
Would it help to associate with each channel (content and chrome side) the window id of the toplevel window that it's for?
(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #10)
> So given that 3. did not resolved the issue & I'm not sure how outlined path
> can be used to resolve it I have started implementing option 2. But I've run
> into several unknowns, I'm gonna post them here in a hope to get some
> guidance from ppl more familiar with our devtools actors & e10s stuff:
> 
> 1. Do messages observed by ConsoleAPIListener are dispatched only either in
> chrome or a content process or both ? In case it's both that conversion of
> ConsoleAPIListener to E10S compatible actor becomes more difficult as it
> would have to aggregate messages from both processes.
> 2. Can console API messages dispatched via observer notifications be
> serialised, as that would be necessary to pipe them over message manager as
> suggested by @ochameau or the way network activity monitor does it. If they
> can not be, I feel like we would need some sort of object grip
> implementation between chrome & content processes.

I don't really know the webconsole code well enough yet to answer these questions.

> 3. Document pointed out by @ochameau show how to register specific module as
> a global or a tab actor, but it's not yet clear for me how converted
> `ConsoleAPIListener` actor could be instantiated from the webconsole actor.
> I imagine there are hooks for that, it's just but I'm not too familiar with
> the whole actor stuff yet.

There are basically two ways to instantiate actors. Either you register a factory for a global or a tab actor, and these can then be instantiated via the protocol. For instance, the actorIds for global actor are automatically reported by the root actor, and sending a message to one of the actorIds causes the global actor to be instantiated from its factory.

The other way is simply to instantiate the actor directly, which is usually what we do when instantiating an actor that is not a top-level or global actor. For instance, in script.js, BreakpointActors are explicitly instantiated from SourceActors (of course, if you need to instantiate an actor thats defined in a different file, it's fine to just require it).
Flags: needinfo?(ejpbruel)
(In reply to Not doing reviews right now from comment #20)
> Ah, I see.  Well, one option would be to change
> HttpBaseChannel::AddSecurityMessage to ship the message to the content
> process if we're the parent-side reflection of a content channel.  The thing
> is, that would then involve shipping the message right back to actually show
> it in the console, right?

I imagine that would fix this issue. I don't know the http* code much, but I imagine it will be forwarded to the child, which will call the child console, like other log messages that already works from child process.

(In reply to Not doing reviews right now from comment #21)
> Would it help to associate with each channel (content and chrome side) the
> window id of the toplevel window that it's for?

Otherwise, if we don't forward security message in http codebase, this would help doing the piping workaround in JS, in devtools codebase, like what we did for the network monitor.
Flags: needinfo?(poirot.alex)
I agree with ochameau's response.

So the thing is if we don't forward this messages in the http code we still would need to forward them, but in JS code and add (I imagine much larger overhead) as each console instance will still have to subscribe and even do it's own filtering etc...


Having an toplevel window id for the notification would help indeed, as that way we could do some filtering in the chrome code to avoid forwarding unrelated messages. Although there still going to be case where devtools is targeted at iframe with-in a page, so in those cases we still end up sending more data that we would need to filter out.

Who could I talk to about changes to `HttpBaseChannel::AddSecurityMessage` you've mentioned ? My C++ is rusty, but I'd be willing to give it a try if someone would be willing to mentor ? I'm also happy to let someone else do those changes if that is an option.
Flags: needinfo?(bzbarsky)
> Who could I talk to about changes to `HttpBaseChannel::AddSecurityMessage`

The necko module owner?
Flags: needinfo?(bzbarsky)
Hi Tom,

Joe told me you maybe able to look into the necko side of things ? Do you think you'll have a chance ?
(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #26)

> Joe told me you maybe able to look into the necko side of things ? Do you
> think you'll have a chance ?

Sorry about the delay on this.  I'll start looking at it today.
Blocks: 1068944
This patch arranges to forward AddSecurityMessage calls from the
parent http channel back to the child.

The existing browser_webconsole_show_subresource_security_errors.js test
seemed sufficient; with this patch it can be enabled for e10s.
Attachment #8577345 - Flags: review?(hurley)
Comment on attachment 8577345 [details] [diff] [review]
forward network security messages to the content process

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

Thanks, this will definitely be useful to have! As I mention in one of the inline comments below, I'm not sure this is the right way to go about making this change, though it's close. I need to think a bit more on the right code architecture here, but in the meantime, here are a few comments to get started.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1662,5 @@
>   * More information can be found here:
>   * https://bugzilla.mozilla.org/show_bug.cgi?id=846918
>   */
>  nsresult
> +HttpBaseChannel::AddSecurityMessage(const nsString &aMessageTag,

Is changing the argument type here really necessary?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6466,5 @@
> +nsresult
> +nsHttpChannel::AddSecurityMessage(const nsString &aMessageTag,
> +                                  const nsString &aMessageCategory)
> +{
> +    if (mWarningReporter != nullptr) {

if (mWarningReporter)

@@ +6469,5 @@
> +{
> +    if (mWarningReporter != nullptr) {
> +        return mWarningReporter->AddSecurityMessage(aMessageTag,
> +                                                    aMessageCategory);
> +    } else {

Don't else after return.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +45,5 @@
>    0x4ae1,                                          \
>    {0xa9, 0x71, 0x40, 0xbc, 0xfa, 0x81, 0xde, 0x12} \
>  }
>  
> +class HttpChannelSecurityWarningReporter

This seems like not a good idea. AddSecurityMessage is already declared by nsIHttpChannelInternal, which all the classes you're changing already inherit from (indirectly). We should re-think the right way to go about this.

@@ +151,5 @@
>  
> +    nsresult AddSecurityMessage(const nsString &aMessageTag,
> +                                const nsString &aMessageCategory) MOZ_OVERRIDE;
> +
> +    void SetWarningReporter(HttpChannelSecurityWarningReporter* aReporter) {

Style this inline definition the same as the ones a few lines down, please (with braces on the same line as the assignment)
I rebased the patch and fixed the styling problems.  I reverted the
nsString change.  I also fixed the placement of the "&"s.
Attachment #8577345 - Attachment is obsolete: true
Attachment #8577345 - Flags: review?(hurley)
(In reply to Nicholas Hurley [:hurley] from comment #29)

> >  nsresult
> > +HttpBaseChannel::AddSecurityMessage(const nsString &aMessageTag,
> 
> Is changing the argument type here really necessary?

I remember thinking that this would be ugly, but I tried it just now
and I don't know what I was thinking, since it is just a couple explicit
casts to nsString.  The new patch doesn't include this change.

I think I've fixed all the styling problems.

> > +class HttpChannelSecurityWarningReporter
> 
> This seems like not a good idea. AddSecurityMessage is already declared by
> nsIHttpChannelInternal, which all the classes you're changing already
> inherit from (indirectly). We should re-think the right way to go about this.

I don't see it in nsIHttpChannelInternal.  It's declared in HttpBaseChannel.

My understanding of the code -- take with a grain of salt, etc -- is that the
redirect has to be done conditionally.  Specifically, HttpChannelParent::DoAsyncOpen
needs to intercept security messages on the underlying channel, but no other user
does.  This led to the idea in the patch.

It seemed that another approach would require a subclass of nsHttpChannel and then
maybe code in nsHttpHandler::NewProxiedChannel2 to instantiate this subclass.
I was a lot less comfortable with this approach since it wasn't clear to me that
this was the only spot needing an update.  Also, come to think of it, I don't know
how to tell if the current process is a parent process (and there is a child process).

NI'ing you so this doesn't drop off your queue :-)
Flags: needinfo?(hurley)
That's looking way better, and I think I've figured out how to go about doing this in pretty much the same way, but without the name confusion from both HttpBaseChannel and HttpChannelSecurityWarningReporter.

First, let's rename the one function defined by HttpChannelSecurityWarningReporter to something like ReportSecurityMessage. Then, only HttpChannelParent will inherit HttpChannelSecurityWarningReporter.

Second, make AddSecurityMessage in HttpBaseChannel virtual, and override it in nsHttpChannel like you're doing now (of course changing the call to mSecurityReporter->AddSecurityMessage to whatever new name you come up with for that method).

The other thing we need to think about is how often these messages come in, and whether we should batch the messages to the child process up, either in one IPC message specific to security messages, or perhaps with something like OnStopRequest. I'm ni'ing Jason (who has a better picture of the full necko e10s state than I do) to get an opinion on that.
Flags: needinfo?(hurley) → needinfo?(jduell.mcbugs)
Assignee: rFobic → ttromey
This implements the plan from comment #32.  I also renamed the ipdl
method to ReportSecurityMessage, since I thought it seemed a bit
clearer.
Attachment #8579358 - Attachment is obsolete: true
(In reply to Tom Tromey :tromey from comment #33)
> Created attachment 8579441 [details] [diff] [review]
> forward network security messages to the content process
> 
> This implements the plan from comment #32.  I also renamed the ipdl
> method to ReportSecurityMessage, since I thought it seemed a bit
> clearer.

Shouldn't this be flagged for review?
(In reply to Eddy Bruel [:ejpbruel] from comment #34)
> (In reply to Tom Tromey :tromey from comment #33)

> Shouldn't this be flagged for review?

It's waiting on a needinfo, see comment #32.
(In reply to Nicholas Hurley [:hurley] from comment #32)
> That's looking way better, and I think I've figured out how to go about
> doing this in pretty much the same way, but without the name confusion from
> both HttpBaseChannel and HttpChannelSecurityWarningReporter.
> 
> First, let's rename the one function defined by
> HttpChannelSecurityWarningReporter to something like ReportSecurityMessage.
> Then, only HttpChannelParent will inherit HttpChannelSecurityWarningReporter.
> 
> Second, make AddSecurityMessage in HttpBaseChannel virtual, and override it
> in nsHttpChannel like you're doing now (of course changing the call to
> mSecurityReporter->AddSecurityMessage to whatever new name you come up with
> for that method).
> 
> The other thing we need to think about is how often these messages come in,
> and whether we should batch the messages to the child process up, either in
> one IPC message specific to security messages, or perhaps with something
> like OnStopRequest. I'm ni'ing Jason (who has a better picture of the full
> necko e10s state than I do) to get an opinion on that.

Jason, could you please answer this? This bug is blocking the E10S uplift to Aurora for DevTools. Thanks!
Summary: Security messages do not display in web console with e10s → [e10s] Security messages do not display in web console
Sorry for the delay.  In a perfect world we could piggyback these messages as part of the RecvOnStartRequest, or something like that.  But given bug 846918 comment 38, it sounds like these messages are quite rare, so I think it's expedient to just land what we've got here.

+f
Flags: needinfo?(jduell.mcbugs)
Tom, if you have the time and you know this code well enough, perhaps you could look at bug 846918 comment 73? I needinfo'd alagenchev but it looks like he may not be an active contributor any more.
Attachment #8579441 - Attachment is obsolete: true
Comment on attachment 8591704 [details] [diff] [review]
forward network security messages to the content process

Rebased and fixed up due for the MOZ_OVERRIDE -> override change.
Attachment #8591704 - Flags: review?(hurley)
Comment on attachment 8591704 [details] [diff] [review]
forward network security messages to the content process

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

This looks pretty good. One minor thing below, r=me with that change.

Also, take a look at your try results - all the OS X builds failed. The failures seem innocuous, but might want to retrigger them all to be sure (unless, of course, you know something I don't - quite possible!)

::: netwerk/protocol/http/nsHttpChannel.h
@@ +45,5 @@
>    0x4ae1,                                          \
>    {0xa9, 0x71, 0x40, 0xbc, 0xfa, 0x81, 0xde, 0x12} \
>  }
>  
> +class HttpChannelSecurityWarningReporter

Move this declaration up, above the giant comment that says "nsHttpChannel" (just above the IID definition) - let's not let this get confused in with the main point of this file :)
Attachment #8591704 - Flags: review?(hurley) → review+
This addresses the review comment.  I'll carry over the r+.

I think those MacOS build problems were a problem with the build
infrastructure.  I'm redoing them though to be sure.
Attachment #8591704 - Attachment is obsolete: true
Attachment #8592424 - Flags: review+
Keywords: checkin-needed
Hi,

this doesn't apply cleanly to fx-team :

applying Bug-1096908---forward-network-security-messages-to.patch
patching file netwerk/protocol/http/HttpBaseChannel.h
Hunk #1 FAILED at 174
1 out of 1 hunks FAILED -- saving rejects to file netwerk/protocol/http/HttpBaseChannel.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh Bug-1096908---forward-network-security-messages-to.patch

could you take a look, thanks!
Flags: needinfo?(ttromey)
Rebased but not otherwise changed.
Attachment #8592424 - Attachment is obsolete: true
Flags: needinfo?(ttromey)
Keywords: checkin-needed
I rebased this wrong and backed it out instead of fixing it:
https://hg.mozilla.org/integration/fx-team/rev/8f643838421a

I'll reland this fixed when I do my next batch of checkins.
Keywords: checkin-needed
Whiteboard: [e10s-m6][fixed-in-fx-team] → [e10s-m6]
Keywords: checkin-needed
Whiteboard: [e10s-m6] → [e10s-m6][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/cc1202199a83
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [e10s-m6][fixed-in-fx-team] → [e10s-m6]
Target Milestone: --- → Firefox 40
QA Whiteboard: [good first verify][verify in Nightly only]
Reproduced the bug in 36.0a1 (2014-11-11) on Linux x64

Verified as fixed with 
Latest Beta 40.0b3

Build ID: 20150709163524
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
QA Whiteboard: [good first verify][verify in Nightly only] → [good first verify][testday-20150710]
Whiteboard: [e10s-m6] → [e10s-m6][testday-20150710]
I have reproduced the bug in nightly 36.0a1 (2014-11-11) with the instruction from comment 0 and linux 32 bit

Bug is fixed now on latest nightly 42.0a1 (2015-07-10)

Build ID: 20150710030206
Mozilla/5.0 (X11; Linux i686; rv:42.0) Gecko/20100101 Firefox/42.0
I have reproduced this bug with Firefox Nightly 36.0a1(Build: 20141111030203)on 
windows 8.1 pro x64 with the instructions from comment 0 .

Verified as fixed with Latest Firefox Nightly 42.0a1 (Build ID: 20150711030210)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Reproduced with Firefox nightly 36.0a1(20141111030203) on Windows 7 64 Bit.

The fix is working for latest Aurora 41.0a2(20150711004006)

UA: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0

As per comment 50 to comment 53 , updating the status to verified fixed.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.