1274556 - Add a channelId attribute to nsIHttpChannel

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jarda Snajdr [:jsnajdr]]

To properly monitor network requests in devtools Network Monitor, we need an unique identifier for each nsIChannel. This channelId will be shared across process boundaries and used to connect together information that is available only in parent or in child.

For example, only child process knows what is the JS call stack when the request is initiated. And only the parent has information about the network activity etc. Without a channelId, there is no way how to tell whether given HttpChannelParent and HttpChannelChild.

See bug 1134073 for example usage and a detailed discussion that led to the channelId solution.

Comment 1

[Patrick McManus [:mcmanus]]

nick had to do something like this for load group. his experience should guide the implementation.

Comment 2

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 1: add support for nsIChannel::channelId

Comment 3

[Jarda Snajdr [:jsnajdr]]

Attachment: Part 2: use channelId in RedirectChannelRegistrar

Comment 4

[Jarda Snajdr [:jsnajdr]]

This is my WIP patch for the channelId:
- adds a channelId to nsIChannel
- implements unique ID allocation for HTTP channels. Currently, parent assigns even numbers, child assigns odd
- non-HTTP channels implement GetChannelId as NOT_IMPLEMENTED
- RedirectChannelRegistrar doesn't assign its own IDs any more and uses the nsIChannel's channelId
- when child initiates channel creation, it assigns the channelId and sends it over as a new member in HttpChannelOpenArgs
- when parent initiates channel creation (i.e., redirect), it assigns the channelId and sends it over as a Redirect1Begin parameter - I'm reusing the existing newChannelId, which is the id assigned by RedirectChannelRegistrar

Problems:
- if the channel doesn't implement the channelId, it cannot be registered with RedirectChannelRegistrar and redirects to non-HTTP URLs will fail. This problem is real and is very visible in this try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1727960d1b5
- implementing channelId for all nsIChannel is probably not feasible - there are many independent implementations across the code base and as I understand it, every add-on author can implement their own nsIChannel, for example in an about module.

Because devtools Network Monitor only cares about nsIHttpChannel's, a possible solution is to limit channelIds only to nsIHttpChannels. Then RedirectChannelRegistrar would have to continue assigning its own independent IDs to nsIChannels and there would be two channelIds. The IPDL API would get confusing:

- HttpChannelOpenArgs::channelId is the internal ID of the channel
- HttpChannelConnectArgs::channelId is the Registrar ID
- PHttpChannel::Redirect1Begin would have to send over two IDs - the Registrar ID (to be sent back as part of HttpChannelConnectArgs after verification) and the internal ID (so that the new HttpChannelChild has the same ID as the HttpChannelParent)

To resolve this confusion and make the API comprehensible, we'd have to do some parameter renaming.

Comment 5

[Honza Bambas (:mayhemer)]

Jardo, is the channel id also usefull in non-e10s environment?  I'm thinking of moving your new id to nsIParentChannel/nsIChildChannel.  See also bug 590682 for how we want to support custom protocols in e10s, the id impl would be completely under our control.

I personally would not be against to having a new id (yours) assigned to a channel while also having the redirect registrar exchange id held separately.  The redirect id's lifetime (by means of how long we need to keep it) is anyway relatively short and only for purpose of the redirect logic.  If explained in comments what for there are two ids, I think we can live with it.

Comment 6

[Jarda Snajdr [:jsnajdr]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Jardo, is the channel id also usefull in non-e10s environment?  I'm thinking
> of moving your new id to nsIParentChannel/nsIChildChannel.

No, it's useful only for e10s. In non-e10s, I can directly compare the two nsIHttpChannel JS objects (channel1 == channel2). Having channelId also in non-e10s would only simplify the NetworkMonitor code, as we wouldn't have to handle both cases separately. But of course, it's not worthwhile if it would add complexity to the C++/IPDL code.

I'll have a look at the nsIParentChannel/nsIChildChannel approach.

Comment 7

[u408661]

Comment on attachment 8754810 [details] [diff] [review]
Part 1: add support for nsIChannel::channelId

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

So overall - I don't understand a lot of the design decisions here. Why do we need to have this on all nsIChannels, instead of just making it available on nsIHttpChannel and QI'ing when you need to? Also, why are you using a locked, serialized integer? Why does it have to be even on the parent and odd on the child? What happens when we have more than one child process (and therefore could have overlapping channel ID spaces)?

Take a look at how nsIRequestContext works - using an nsID for its identifier (which is guaranteed to be unique across any number of processes, and doesn't require a lock and static ctor). I think that's a better example for how you should be going about this.

::: netwerk/base/nsIChannel.idl
@@ +34,5 @@
>  {
>      /**
> +     * Unique ID of the channel, shared between parent and child
> +     */
> +    readonly attribute unsigned long channelId;

Is there a particular reason to have this on all channels, instead of just HTTP channels? Seems like a lot of boilerplate being added in this patch for no apparent reason.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +58,5 @@
>  
>  namespace mozilla {
>  namespace net {
>  
> +Mutex channelIdLock("ChannelId");

Doesn't this add a static ctor? Seems bad...

@@ +63,5 @@
> +static uint32_t channelId = 0;
> +static uint32_t getNextChannelId() {
> +  MutexAutoLock lock(channelIdLock);
> +  uint32_t nextId = ++channelId * 2;
> +  // Even IDs in parent, odd IDs in child.

Why?

@@ +373,5 @@
> +HttpBaseChannel::GetChannelId(uint32_t *aChannelId)
> +{
> +  NS_ENSURE_ARG_POINTER(aChannelId);
> +
> +  if (mChannelId == 0) {

!mChannelId

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1847,5 @@
>  
>    HttpChannelOpenArgs openArgs;
>    // No access to HttpChannelOpenArgs members, but they each have a
>    // function with the struct name that returns a ref.
> +  openArgs.channelId() = mChannelId;

Add this at the end of the list

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +118,5 @@
>    switch (aArgs.type()) {
>    case HttpChannelCreationArgs::THttpChannelOpenArgs:
>    {
>      const HttpChannelOpenArgs& a = aArgs.get_HttpChannelOpenArgs();
> +    return DoAsyncOpen(a.channelId(), a.uri(), a.original(), a.doc(), a.referrer(),

Again, end of list

@@ +227,5 @@
>  // HttpChannelParent::PHttpChannelParent
>  //-----------------------------------------------------------------------------
>  
>  bool
> +HttpChannelParent::DoAsyncOpen(  const uint32_t&            aChannelId,

End

::: netwerk/protocol/http/HttpChannelParent.h
@@ +102,5 @@
>    // used to connect redirected-to channel in parent with just created
>    // ChildChannel.  Used during redirects.
>    bool ConnectChannel(const uint32_t& channelId, const bool& shouldIntercept);
>  
> +  bool DoAsyncOpen(const uint32_t&            channelId,

end

Comment 8

[Jarda Snajdr [:jsnajdr]]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #7)
> So overall - I don't understand a lot of the design decisions here. Why do
> we need to have this on all nsIChannels, instead of just making it available
> on nsIHttpChannel and QI'ing when you need to?

My original idea was to merge the IDs assigned by RedirectChannelRegistrar and the nsIChannel::channelId into one identifier. This turned out to be not feasible, because there are too many independent implementations of nsIChannel (many of them in add-ons) and they all would have to change.

I'll move the channelId to nsIHttpChannel and keep the existing RedirectChannelRegistrar IDs.

> Also, why are you using a locked, serialized integer? Why does it have to be
> even on the parent and odd on the child? What happens when we have more than
> one child process (and therefore could have overlapping channel ID spaces)?

I need to assign unique IDs across processes and even/odd looked like a fastest way to a working proof-of-concept. I'll convert them to nsIDs. Another way how to do it are 64bit incremental IDs that have the process ID in the upper bits - that's what NextWindowID at [1] is doing.

If I was using an incremental counter (I won't, I'll use UUIDs instead), I'm not sure if I need to lock the counter. Is the nsIHttpChannel creation restricted to the main thread? NextWindowID() at [1] is not locked, but assigning RedirectChannelRegistrar IDs at [2] is...

I'll post a new patch with all the issues addressed.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#3195
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/RedirectChannelRegistrar.cpp#24

Comment 9

[Jarda Snajdr [:jsnajdr]]

Attachment: Add a channelId attribute to nsIHttpChannel

New version of the patch with the following changes:

- channelId is defined only on nsIHttpChannel
- it is a nsID, exposed as string in IDL, because nsID attributes cannot be accessed from scripts: if the attribute is not nsIDPtr, conversion to nsJSID crashes
- the channelId allocated by RedirectChannelRegistar is renamed to registrarId, to avoid confusion

Comment 10

[Honza Bambas (:mayhemer)]

Can you just explain (or point me to an explanation) why the idea of exposing this on nsIParentChannel and nsIChildChannel has been abandoned?

Comment 11

[Jarda Snajdr [:jsnajdr]]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> Can you just explain (or point me to an explanation) why the idea of
> exposing this on nsIParentChannel and nsIChildChannel has been abandoned?

To be useful for NetworkMonitor, the channelId property must be available on the nsIHttpChannel object (or be QI-able from it) that is passed as subject to notifications the NetworkMonitor receives. I.e., the "http-on-*" notifications from nsIObserverService, and notifications from nsIHttpActivityDistributor.

In the parent, this object is an instance of nsHttpChannel, which doesn't implement nsIParentChannel. nsIParentChannel is implemented only by HttpChannelParent, which is not a nsIChannel at all. It's just a helper class that has a member pointer to the real nsIHttpChannel and that brokers the IPC communication between parent and child.

So, if the channelId was on nsIParentChannel, it wouldn't be reasonably accessible to the NetworkMonitor or to anyone else who listens to the activity notifications.

Maybe it would be solvable using some nsIInterfaceRequestor magic, but that looked overly complex to me.

HttpChannelChild is different - it's a full-fledged nsIHttpChannel and implements nsIChildChannel, too.

Another reason is that (currently) we need channelId only on HTTP channels. And nsIParentChannel/nsIChildChannel are implemented also by other protocols: ftp, rtsp, data.

Comment 12

[Honza Bambas (:mayhemer)]

You're right, thanks.

Comment 13

[u408661]

Comment on attachment 8755834 [details] [diff] [review]
Add a channelId attribute to nsIHttpChannel

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

Looks pretty good, just a few things that need fixing before an r+ happens.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +361,5 @@
>  
>    friend class PrivateBrowsingChannel<HttpBaseChannel>;
>    friend class InterceptFailedOnStop;
>  
> +  nsID                              mChannelId;

Please add new members/arguments/etc to the end of the list

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1278,5 @@
>  
>    nsCString secInfoSerialization;
>    UpdateAndSerializeSecurityInfo(secInfoSerialization);
>  
> +  // Get the channelId if the newChannel is a nsIHttpChannel

This comment explains what is happening (which is obvious from the code). Please explain why (or, in this case, why we only need it from an HTTP channel)

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +17,5 @@
>  [builtinclass, scriptable, uuid(c5a4a073-4539-49c7-a3f2-cec3f0619c6c)]
>  interface nsIHttpChannel : nsIChannel
>  {
> +    /**
> +     * Unique ID of the channel, shared between parent and child

Probably good to explain why this is here, or at least point to the bug so people can read the rationale.

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp
@@ +717,5 @@
>  // want various headers like Link: and Refresh: applying to view-source.
>  NS_IMETHODIMP
> +nsViewSourceChannel::GetChannelId(nsACString& aChannelId)
> +{
> +  return !mHttpChannel ? NS_ERROR_NULL_POINTER :

mHttpChannel ? mHttpChannel->GetChannelId(aChannelID) : NS_ERROR_NULL_POINTER;

Using NS_ERROR_NOT_AVAILABLE might also be better.

@@ +724,5 @@
> +
> +NS_IMETHODIMP
> +nsViewSourceChannel::SetChannelId(const nsACString& aChannelId)
> +{
> +  return !mHttpChannel ? NS_ERROR_NULL_POINTER :

Same here - don't use a negative condition, its harder to read and understand.

Comment 14

[Jarda Snajdr [:jsnajdr]]

Attachment: Add a channelId attribute to nsIHttpChannel

Fixed the issues from review.

I didn't make the suggested changes in nsViewSourceChannel.cpp, because all other methods in that class use exactly the same pattern: check for !mHttpChannel, return NS_ERROR_NULL_POINTER if it's not set. I think it's better to keep it consistent for the new methods, too.

Should any tests be written for this patch? And when submitting a try run, which test suites should be selected in trychooser? This is my first Necko patch, so I'm not sure.

Comment 15

[u408661]

(In reply to Jarda Snajdr [:jsnajdr] from comment #14)
> Created attachment 8756009 [details] [diff] [review]
> Add a channelId attribute to nsIHttpChannel
> 
> Fixed the issues from review.
> 
> I didn't make the suggested changes in nsViewSourceChannel.cpp, because all
> other methods in that class use exactly the same pattern: check for
> !mHttpChannel, return NS_ERROR_NULL_POINTER if it's not set. I think it's
> better to keep it consistent for the new methods, too.

Ugh, what a horrible convention (the negative condition that is, the return value is whatever). I'm gonna blame the fact that the original versions of those are 13 years old at this point. Alright.

> Should any tests be written for this patch? And when submitting a try run,
> which test suites should be selected in trychooser? This is my first Necko
> patch, so I'm not sure.

Some testing would be nice - an xpcshell test to make sure when we create a channel that both parent and child channels have the same ID seems to make sense. Take a look at netwerk/test/unit_ipc for examples.

However, if it looks too nasty (quite possible with e10s tests), don't spend too much time on it - this isn't that likely to be an issue.

For a try run, I would just do a T-shaped run (build on all platforms, all unit tests on one platform).

Let me know once you've made a decision on tests, and I'll either review the patch you have up now, or a new one with a test depending on your decision.

Comment 16

[Jarda Snajdr [:jsnajdr]]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #15)
> Ugh, what a horrible convention (the negative condition that is, the return
> value is whatever). I'm gonna blame the fact that the original versions of
> those are 13 years old at this point. Alright.

I can do a cleanup of the whole nsViewSourceChannel.cpp file, and submit it as a separate drive-by patch in this bug. If you think it's worth it.

> Some testing would be nice - an xpcshell test to make sure when we create a
> channel that both parent and child channels have the same ID seems to make
> sense. Take a look at netwerk/test/unit_ipc for examples.

I'll try to add a simple unit test for the channelId. Then I'll attach it as a separate patch, so you can review the main patch without waiting. There's a 9 hours timezone difference between us, so let's avoid unnecessary delays.

Comment 17

[u408661]

(In reply to Jarda Snajdr [:jsnajdr] from comment #16)
> (In reply to Nicholas Hurley [:nwgh][:hurley] from comment #15)
> > Ugh, what a horrible convention (the negative condition that is, the return
> > value is whatever). I'm gonna blame the fact that the original versions of
> > those are 13 years old at this point. Alright.
> 
> I can do a cleanup of the whole nsViewSourceChannel.cpp file, and submit it
> as a separate drive-by patch in this bug. If you think it's worth it.

Probably not - view-source is pretty stable (the blame for it is mostly code from 2003), I don't think it's worth messing around just because I get a yucky taste in my mouth from its style :)

> > Some testing would be nice - an xpcshell test to make sure when we create a
> > channel that both parent and child channels have the same ID seems to make
> > sense. Take a look at netwerk/test/unit_ipc for examples.
> 
> I'll try to add a simple unit test for the channelId. Then I'll attach it as
> a separate patch, so you can review the main patch without waiting. There's
> a 9 hours timezone difference between us, so let's avoid unnecessary delays.

Works for me!

Comment 18

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab438f43c53b

Comment 19

[Jarda Snajdr [:jsnajdr]]

Attachment: Add a channelId attribute to nsIHttpChannel (unit test)

Added a unit test that sends two requests (one of them redirected) and checks if the channelIds are equal across processes.

Comment 20

[u408661]

Comment on attachment 8756811 [details] [diff] [review]
Add a channelId attribute to nsIHttpChannel (unit test)

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

Looks pretty good, thanks for writing it! Just one change, and one other thing to think about, and then I think we're good to go.

::: netwerk/test/unit_ipc/test_channel_id.js
@@ +24,5 @@
> +    response.setHeader("Location", "/resource", false);
> +    response.setHeader("Cache-Control", "no-cache", false);
> +  });
> +
> +  httpserv.start(12345);

Instead of this, use a dynamic port number (pass -1 as the port), get the port from the server using httpserv.identity.primaryPort, and use SendCommand to tell the child about the port number. That way we don't have to force this to run sequentially.

@@ +49,5 @@
> +  //
> +  // The second channel will be redirected and the child will never see
> +  // the response to it - let's skip it.
> +  if (notifCount == 1) {
> +    childChannelIds.push(null);

Hrm - is there really no way we can get the id of this channel on the child? I understand we don't get an onStop, but since the child knows to create a new channel for the redirect that happens on the parent, there's definitely some cross-talk. We never send a notification on the child or anything like that, that we could hook into here?

Comment 21

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5410ad411742

Comment 22

[Jarda Snajdr [:jsnajdr]]

Attachment: Add a channelId attribute to nsIHttpChannel (unit test)

Improved version of the test:
- the child process receives commands to send HTTP requests, notifies the parent about the original channel (returned from NetUtil.newChannel) and the response channel (it's different after a redirect)
- if there were multiple redirects, I could detect them all in the child by registering a nsIChannelEventSink. Fortunately, I don't need to, so I can avoid this verbose API
- the parent starts a HTTP server on a random port, then issues commands to child to send HTTP requests, and waits for the right events to arrive

Comment 23

[Carsten Book [:Tomcat]]

has problems to apply: can you take a look, thanks!

adding 1274556 to series file
renamed 1274556 -> Bug-1274556---Add-a-channelId-attribute-to-nsIHttp.patch
applying Bug-1274556---Add-a-channelId-attribute-to-nsIHttp.patch
patching file netwerk/protocol/http/nsHttpHandler.h
Hunk #1 FAILED at 22
1 out of 2 hunks FAILED -- saving rejects to file netwerk/protocol/http/nsHttpHandler.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1274556---Add-a-channelId-attribute-to-nsIHttp.patch

Comment 24

[Jarda Snajdr [:jsnajdr]]

Attachment: Add a channelId attribute to nsIHttpChannel

Rebased version of the patch for check-in.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc171b5fafc0
Add a channelId attribute to nsIHttpChannel r=hurley
https://hg.mozilla.org/integration/mozilla-inbound/rev/16042a3a145c
Add a channelId attribute to nsIHttpChannel (unit test) r=hurley

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc171b5fafc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/16042a3a145c

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc171b5fafc0
https://hg.mozilla.org/mozilla-central/rev/16042a3a145c