Reintroduce http-on-opening-request notification in the child

RESOLVED FIXED in Firefox 49

Status

()

Core
Networking: HTTP
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ochameau, Assigned: jsnajdr)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox49 fixed)

Details

(Whiteboard: [necko-backlog])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Bug 806753 disabled "http-*" notification in childs.
We had to workaround that for the Network Monitor in devtools.
We end up involving a complex and hard to maintain dance between the child process and the parent process.

For reasons specific to devtools, we have to generate informations about the http requests from the child process. So we pipe all nsIHttpChannel information from parent to the child.
It is soon going to be even more complex for the sake of bug 906239, as we will now receive a notification in the child with the stack of a request. And we have to then match this stack with some metadata coming from the parent process via message manager.

From the early support of e10s in the network monitor, I've always suspected the disabling of "http-*" to be justified for addons, but not really for devtools, where we mostly inspect requests.
Wouldn't it be correct to enable these notifications (or new ones) in the child if we only read the nsIHttpChannel objects?
Then we could drop this cruft to synchronize between parent and child via message manager and make it much easier to match stack objects with http channel objects.
(Assignee)

Comment 1

2 years ago
The comment at [1] says that in the child process, information about cookies is not complete. I don't know how true or how difficult to change that is, but not having complete cookies info is obviously not a good deal for NetMonitor...

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1143006#c4
(Assignee)

Updated

2 years ago
Blocks: 1134073
Jason, i am putting this to backlog, and you can decide how urgent this is and find an owner.
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [necko-backlog]

Updated

2 years ago
tracking-e10s: --- → ?
> From the early support of e10s in the network monitor, I've always suspected
> the disabling of "http-*" to be justified for addons, but not really for
> devtools, where we mostly inspect requests.  Wouldn't it be correct to enable
> these notifications (or new ones) in the child if we only read the
> nsIHttpChannel objects?  Then we could drop this cruft to synchronize between
> parent and child via message manager and make it much easier to match stack
> objects with http channel objects.

IIRC we decided to turn off on-***-request notifications in the child for vague security reasons, plus we were hitting some speed bumps with supporting cookies correctly.  Unless I'm misremembering things, the only issues with cookies in the child are those mentioned in bug 806753 comment 0, i.e. 1) by the time on-examine-request would be called on the child, cookies have already set from incoming Set-Cookie headers on the parent (so silently filtering out a Set-Cookie header won't stop the cookie from getting added), and 2) apparently we saw a 5% perf hit from cookie IPDL overhead at some point for some reason (see bug 558624 comment 2). I suspect both of those issues may be solvable. #1 could get fixed by waiting until the child calls on-examine-request to set cookies: the SetCookie IPDL is async, and thus not as painful as the blocking getCookie IPDL calls.  #2 may not even be an issue any more (that profile is 5 years old!  A lot has changed.  I also suspect that it may be time to consider using IPDL shared memory to share the cookie hashtable between all processes: as long as we've got shared memory locks available--I have no idea if we do--that shouldn't be too hard, and would make cookie access very fast from the child).

So one option here is to go for full on-modify-request/on-examine-response support in the child.  But given that this bug at least is just for supporting internal code (Network Monitor), I think the suggestion that we add new, internal-only notifications ("internal-on-modify-request", etc) is a good first step, and we can build on things from that.  That should be as simple as going to the commented out on-modify-request, etc on the child and changing them to fire off a new internal events:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1593

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#459

(Note: we'd want http-on-examine-cached-response and http-on-examine-merged-response too?  We don't have commented out lines for those in HttpChannelChild).

That right there might be enough for this bug. 

What exactly about cookies does the Network Monitor want to see?  Just the Cookie: header from the request and any Set-Cookie headers from the server?  Those are readable from the channel's HTTP request & response HTTP headers.  If you're doing nsICookieService::GetCookie() calls those will be less efficient (GetCookie does a sync IPDL round trip to the parent that blocks the child until it's done).  But I'm guessing you're not doing that?

Valentin, do you have cycles to take this any time soon?
Flags: needinfo?(jduell.mcbugs) → needinfo?(valentin.gosu)
(Assignee)

Comment 4

2 years ago
The "http-on-*" events would be very nice if they helped us to fully implement the NetworkMonitor in the child process. Then we could monitor the network solely using the HttpChannelChild instances, using the "http-on-*" events they fire and the nsITraceableChannel listeners. Then, we wouldn't have to run any NetworkMonitor code from the parent, making the NetworkMonitor much simpler.

Unfortunately, most of the monitoring work is done by the nsHttpActivityDistributor, which lives in the parent. We use its events to gather timing info about the HTTP events (REQUEST_HEADER, REQUEST_BODY_SENT, RESPONSE_START, RESPONSE_HEADER, RESPONSE_COMPLETE, TRANSACTION_CLOSE) and the underlying socket transport events (DNS resolving, establishing TCP connection, WAITING_FOR, RECEIVING_FROM, ...), and also to read the HTTP request/response headers on the *_HEADER events.

Then we need to read the request body (using the nsIUploadChannel) and the response body (using the nsITraceableChannel). They both seem to be available on HttpChannelChild, but I'm not sure if they really work.

We use only three of the "http-on-*" events, and their role is quite minor:

http-on-examine-response: we use this to read the parsed response headers using the header visitor. If we really wanted, we could do this in the ActivityDistributor's REQUEST_HEADER handler and get rid of this event.

http-on-examine-cached-response: this lets us access responses served from cache, which never hit the network and therefore never caused any httpActivity.

http-on-opening-request: this is a new thing in bug 1134073, used to get the JS stack trace for XHR requests. We rely on this event to be fired synchronously in the AsyncOpen call. Patch 1 in bug 1134073 adds support for this event also to the HttpChannelChild - we need it there in e10s.

So, the main task for this bug, if it gets implemented, is to implement the nsHttpActivityDistributor in the child process, in such a way that all relevant events are sent over IPDL and then distributed in the child with HttpChannelChild as the subject. Seems pretty complex to me, what do other people think?

Alternatively, we could continue to run the NetworkMonitor in the parent, but then we need some reliable coordination mechanism between HttpChannelParent and HttpChannelChild. Because this is how we monitor channels and how we collect the stack:

1. JS code sends a XHR request, which causes HttpChannelChild::AsyncOpen call
2. This call synchronously fires http-on-opening-request, and our NetworkMonitorChild handler records the stack. No network event exists yet and the only unique key for the stack data value is the HttpChannelChild instance object.
3. HttpChannelChild::AsyncOpen sends a constructor request to the parent over IPC, creates a HttpChannelParent object.
4. The HttpChannelParent starts HTTP activity, events get fired, and info about the new HttpChannel arrives to the child over messageManager.
5. But there's no way how to pair this new info with the stack we recorded earlier! I don't know which HttpChannelChild instance this info belongs to. The HttpChannel doesn't have any unique ID shared by parent and child, except for maybe some IPC-internal ActorHandle.

It would help if the HttpChannel had a channelId property. Channels that are created during redirection are registered in RedirectChannelRegistrar, and the resulting channelId is used in IPC calls when performing the redirect. What if we started registering all channels and made the channelId a read-only property in PHttpChannel.ipdl?
Flags: needinfo?(poirot.alex)
Flags: needinfo?(jduell.mcbugs)

Comment 5

2 years ago
(In reply to Jarda Snajdr [:jsnajdr] from comment #4)

> We use only three of the "http-on-*" events, and their role is quite minor:

In bug 1244227, we're also adding a use of http-on-modify-request, along with
some new code that is (currently) parent-process-only to do upload throttling.
(Assignee)

Comment 6

2 years ago
(In reply to Tom Tromey :tromey from comment #5)
> In bug 1244227, we're also adding a use of http-on-modify-request, along with
> some new code that is (currently) parent-process-only to do upload
> throttling.

You do this in order to set the throttleQueue attribute, and it needs to be done before the request is sent over the network, right? This could be done (if needed) also in the http-on-opening-request, because 1) it's sync 2) happens before network activity starts. The only issue is that http-on-opening-request is not called for redirection channels that have the LOAD_REPLACE loadFlag.

Tom, do you think that the throttle code shouldn't be parent-process-only? It seems natural to me that the monitoring and throttling code lives in the same process where the network activity happens, and avoids unneccesary IPC.
Flags: needinfo?(ttromey)
(Reporter)

Comment 7

2 years ago
(In reply to Jarda Snajdr [:jsnajdr] from comment #4)
> So, the main task for this bug, if it gets implemented, is to implement the
> nsHttpActivityDistributor in the child process, in such a way that all
> relevant events are sent over IPDL and then distributed in the child with
> HttpChannelChild as the subject. Seems pretty complex to me, what do other
> people think?

I totally forgot about the ActivityDistributor! As it is mostly about timings of things that only happen in the parent (dns, connection/http steps). It looks like asking to do the manual sync between parent and child in C++ instead of JS, which I think is a bad idea. I don't think the parent currently notify the child about all these steps as it should'nt care about most of them?

For the http-on notifications it is different as we should have an equivalent of these events in the child in order to be functional. So it is only about letting the JS in child be able to know about theve events. Then if we have to keep the parent <-> child piping just for the activitydistributor, it sounds more like an optimization rather than a real simplification of network monitor story.

> Alternatively, we could continue to run the NetworkMonitor in the parent,
> but then we need some reliable coordination mechanism between
> HttpChannelParent and HttpChannelChild. Because this is how we monitor
> channels and how we collect the stack:
> ..
> It would help if the HttpChannel had a channelId property.

It looks like a good idea to make progress on bug 1134073.
Flags: needinfo?(poirot.alex)

Comment 8

2 years ago
(In reply to Jarda Snajdr [:jsnajdr] from comment #6)

> Tom, do you think that the throttle code shouldn't be parent-process-only?
> It seems natural to me that the monitoring and throttling code lives in the
> same process where the network activity happens, and avoids unneccesary IPC.

I don't really have an opinion on where the throttling code could be enabled.
In the other bug I was asked to make it parent-only; if this needs to be lifted,
I don't mind.  I feel that task could readily be done as part of this bug.

I don't think the IPC is going to be significant enough to worry about, particularly
considering that the throttling feature exists to slow things down.  The main
benefit from this bug, on the other hand, seems to be to simplify the network monitor;
which makes sense to me as a goal.
Flags: needinfo?(ttromey)
(Assignee)

Updated

2 years ago
Flags: needinfo?(jduell.mcbugs)
So is there anything that is immediately needed for this bug?
Flags: needinfo?(valentin.gosu)
(Assignee)

Comment 10

2 years ago
(In reply to Valentin Gosu [:valentin] from comment #9)
> So is there anything that is immediately needed for this bug?

Hello Valentin, not exactly for this bug, we'll probably close it as WONTFIX, but I did some work in Necko that adds channelId property to nsIHttpChannel, in bug 1134073. It would help me a lot if I got some feedback from the Necko team and eventually someone who would review my patch when it's final.

Updated

2 years ago
tracking-e10s: ? → -
(Assignee)

Comment 11

2 years ago
Created attachment 8757792 [details] [diff] [review]
Notify http-on-opening-request in content process
Attachment #8757792 - Flags: review?(valentin.gosu)
(Assignee)

Updated

2 years ago
Assignee: nobody → jsnajdr
(Assignee)

Updated

2 years ago
Summary: Reintroduce http request notification in the child → Reintroduce http-on-opening-request notification in the child
(Assignee)

Comment 12

2 years ago
So at the end, the only change in notifications we really need for bug 1134073 is a synchronous "http-on-opening-request" notification triggered in the child inside the HttpChannel's AsyncOpen method. In the notification handler, we can look at "Components.stack" to retrieve the JS stack trace of the place where the HTTP request was issued.

This works great for XMLHttpRequest, because the HttpChannelChild::AsyncOpen is directly, synchronously called by the XHR's "send" method.

However, it's not so great when using the Fetch API, because at [1], the FetchDriver::ContinueFetch (and HttpFetch) is not called directly, but rather postponed into the next event loop tick. At that time, when the nsIHttpChannel is created and opened, and "http-on-opening-request" fired, the JS stack is already gone.

Jonas, is there any reason why the ContinueFetch method cannot be called directly? If it was, it would help us to provide better information about the request in devtools.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#86
Flags: needinfo?(jonas)
Comment on attachment 8757792 [details] [diff] [review]
Notify http-on-opening-request in content process

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

Looks good to me. Thanks.
Attachment #8757792 - Flags: review?(valentin.gosu) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f962268dd6dc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
> Jonas, is there any reason why the ContinueFetch method cannot be called directly? If it was, it would
> help us to provide better information about the request in devtools.

If this question is still relevant, I'd recommend checking with Ben Kelly (:bkelly)
Flags: needinfo?(jonas)
(Assignee)

Comment 17

2 years ago
(In reply to Jonas Sicking (:sicking) from comment #16)
> If this question is still relevant, I'd recommend checking with Ben Kelly (:bkelly)

Still relevant, filed bug 1278778 for it.
See Also: → bug 1278778
You need to log in before you can comment on or make changes to this bug.