Add a channelId attribute to nsIHttpChannel

RESOLVED FIXED in Firefox 49

Status

()

Core
Networking
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jsnajdr, Assigned: jsnajdr)

Tracking

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

2 years ago
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.
nick had to do something like this for load group. his experience should guide the implementation.
Flags: needinfo?(hurley)
(Assignee)

Comment 2

2 years ago
Created attachment 8754810 [details] [diff] [review]
Part 1: add support for nsIChannel::channelId
(Assignee)

Comment 3

2 years ago
Created attachment 8754811 [details] [diff] [review]
Part 2: use channelId in RedirectChannelRegistrar
(Assignee)

Comment 4

2 years ago
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.
Assignee: nobody → jsnajdr
Whiteboard: [necko-active]
(Assignee)

Updated

2 years ago
Blocks: 1134073
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.
(Assignee)

Comment 6

2 years ago
(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 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
Flags: needinfo?(hurley)
(Assignee)

Comment 8

2 years ago
(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
(Assignee)

Updated

2 years ago
Summary: Add a channelId attribute to nsIChannel → Add a channelId attribute to nsIHttpChannel
(Assignee)

Comment 9

2 years ago
Created attachment 8755834 [details] [diff] [review]
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
Attachment #8755834 - Flags: review?(hurley)
(Assignee)

Updated

2 years ago
Attachment #8754810 - Attachment is obsolete: true
Attachment #8754811 - Attachment is obsolete: true
Can you just explain (or point me to an explanation) why the idea of exposing this on nsIParentChannel and nsIChildChannel has been abandoned?
Flags: needinfo?(jsnajdr)
(Assignee)

Comment 11

2 years ago
(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.
Flags: needinfo?(jsnajdr)
You're right, thanks.
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.
Attachment #8755834 - Flags: review?(hurley)
(Assignee)

Comment 14

2 years ago
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.

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.
Attachment #8756009 - Flags: review?(hurley)
(Assignee)

Updated

2 years ago
Attachment #8755834 - Attachment is obsolete: true
(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.
(Assignee)

Comment 16

2 years ago
(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.
(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!
Attachment #8756009 - Flags: review?(hurley) → review+
(Assignee)

Comment 18

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab438f43c53b
(Assignee)

Comment 19

2 years ago
Created attachment 8756811 [details] [diff] [review]
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.
Attachment #8756811 - Flags: review?(hurley)
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?
Attachment #8756811 - Flags: review?(hurley)
(Assignee)

Comment 21

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5410ad411742
(Assignee)

Comment 22

2 years ago
Created attachment 8757255 [details] [diff] [review]
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
Attachment #8757255 - Flags: review?(hurley)
(Assignee)

Updated

2 years ago
Attachment #8756811 - Attachment is obsolete: true
Attachment #8757255 - Flags: review?(hurley) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
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
Flags: needinfo?(jsnajdr)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 24

2 years ago
Created attachment 8758718 [details] [diff] [review]
Add a channelId attribute to nsIHttpChannel

Rebased version of the patch for check-in.
Attachment #8758718 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8756009 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Flags: needinfo?(jsnajdr)
Keywords: checkin-needed

Comment 25

2 years ago
Pushed by kwierso@gmail.com:
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
Keywords: checkin-needed

Comment 26

2 years ago
bugherderlanding
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc171b5fafc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/16042a3a145c

Comment 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dc171b5fafc0
https://hg.mozilla.org/mozilla-central/rev/16042a3a145c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.