251475 - Channels should ensure that originalURI is non-null

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We should document that originalURI is never null for channels (in
nsIChannel.idl) and that setting it to null will throw.  Then we should fix our
channel impls to behave so.

Comment 1

[Carl Parrish]

As soon as someone can explain to me what channels are and / or how to set
nsIChannel.idl to null I'm willing to help on this. I have a cvs build of
mozilla running but proably should grab a more recent one. 

Comment 2

[Boris Zbarsky [:bzbarsky]]

Channels are the networking layer's representation of a network request (eg
there is one channel per HTTP request).

Basically, this bug is about looking at all implementations of getOriginalURI in
the tree and making sure none of them ever return null (this involves also
changing all implementations of setOriginalURI to not accept null and possibly
fixing setOriginalURI callers as needed).

nsIChannel.idl comes in only insofar as the documentation in that file needs to
be updated.

Comment 3

[Carl Parrish]

(In reply to comment #2)
Okay I'll have some free time tomorrow and will start on this. 

Comment 4

[Daniel Kraft]

Is this bug still up-to-date? I'll look at this, although. Maybe I can help.

Comment 5

[Daniel Kraft]

I've looked through HTTP-Channel (nsHttpChannel.c) and found that it neither
checks for null in setOriginalURI(...) nor tests if mOriginalURI is null before
returning in getOriginalURI(...), so this is still to be done.
Why don't we change nsIChannel to be not just an interface, but a concrete
class, which implements the OriginalChannel-methods. They should test for null
as needed and, if the test was successful, call an implementation-specific
protected method get/setOriginalURI_Unsafe, which is equivalent to current
get/setOriginalURI?
Would this be a problem? (It's not just a simple change, but touches the
internal structure!)

Comment 6

[Daniel Kraft]

Of course, you may not want nsIChannel to be changed (nor converted to be an
ordinary C++-class), doesn't @status=FROZEN mean that?

What about a class between the interface and the implementations?
Let nsIChannelClass be : public nsIChannel, and nsHttpChannel and so on be :
public nsIChannelClass.
Then you could implement the originalURI-methods in this class safe, and let the
sub-classes implement originalURIorNull. I'd like this better than just testing
for null in every implementation. I'll try to implement this.

Comment 7

[Daniel Kraft]

I'm sorry, of coure it should be nsChannel instead of nsIChannel...

Comment 8

[Daniel Kraft]

Attachment: Patch for nsHttpChannel

Another problem: nsHttpChannel (for example) inherits from nsIChannel through
nsIHttpChannel. If I want it to inherit from nsChannel, too, which is sub of
nsIChannel, nsHttpChannel would inherit TWICE from nsIChannel.

So I had to change the system slightly:
I created a class nsChannelCommon, which implements the tests in originalURI,
and then forwards to the abstract methods originalUriUnsafe. A macro in
nsHttpChannel.cpp forwards the originalURI-calls to nsChannelCommon.
I've implemented this sofar for nsHttpChannel only, I want to hear what you
think about this solution before changing all the others.
All I know is that it compiles, and that HTTP-communication still works...

PS: I'm sorry, I didn't find out how to make a patch which creates new files.
So I tar-gzipped my patch together with the new files as attachment. I hope you
can use it... If you tell me how to make the patch correcty, of course I will
make it.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

are such originalUriUnsafe methods actually needed, or do the implementations
basically only set/get a member variable? in the latter case, you could just add
a member variable to nsChannelCommon and not have any *Unsafe methods...

as for new files, you may want to use http://viper.haque.net/~timeless/redbean/

is inheriting twice from nsIChannel a problem?

Comment 10

[Daniel Kraft]

I tried nsChannelCommon to inherit from nsIChannel, too, but the compiler told
me that "nsIChannel was an ambigous base of nsHttpChannel" (the code was
something like nsIChannel* ptr=<This is a nsHttpChannelPointer>). When removed
nsIChannel as base of nsChannelCommon, there wasn't this error anymore. (Maybe
this had another reason, but I don't know if C++ allows multiple inheriting from
one base-class).

In nsHttpChannel it is reading and setting an instance variable, but I haven't
so far looked at the other channels. If they do the same, of course we can move
this behaviour to nsChannelCommon on the whole. I'll look at this.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

ah, yes... for that, you'd need a NS_STATIC_CAST to an intermediate class
(nsIHttpChannel*, for example... or nsChannelCommon*)

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

(although, it does mean more vtable entries... hm...)

Comment 13

[Daniel Kraft]

Of course, it is more correct if nsChannelCommon is a sub-class of nsIChannel,
but the macro FORWARD_TO_NSCHANNELCOMMON, for instance, would even then be
needed, as nsHttpChannel explicitly declares to override originalURI through
NS_DECL_NSICHANNEL (or what that macro is called), which is auto-generated (and
can't be changed, so). This is also a problem when we move even the instance
variable to nsChannelCommon.
Personally, I'd prefer doing the things "correctly" (sub-class of nsIChannel),
but only if it works (of course), and it is good C++ to inherit twice.

Comment 14

[Daniel Kraft]

Now I've looked through nsIChannel-subclasses, and found that MOST of them
define an instance variable mOriginalURI or similiar.
Special implemetations I found were for example:
 - nsAboutCacheEntry: netwerk/protocol/about/src/nsAboutCacheEntry.cpp#187ff
 - nsPartChannel: netwerk/streamconv/converters/nsMultiMixedConv.cpp#220ff
 - nsJARChannel: modules/libjar/nsJARChannel.cpp#386ff
 - nsDataChannel: netwerk/protocol/data/src/nsDataChannel.cpp#311ff
 - nsFileChannel: netwerk/protocol/file/src/nsFileChannel.cpp#219ff
 - nsFTPChannel: netwerk/protocol/ftp/src/nsFTPChannel.cpp#229ff
Some of them implement GetOriginalURI as
mOriginalURI ? mOriginalURI : <Standard-URL, Document-URL...> which is listed
above. For the whole list, just search for
::GetOriginalURI( in lxr. So I'm not sure whether we should move mOriginalURI to
nsChannelCommon. We could write classes below nsChannelCommon to implement the
standard-cases (with mOriginalURI; how about naming of those classes?) and use
one of those instead of nsChannelCommon as super-class of, let's say, nsHttpChannel?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Darin, haven't you been talking about maybe having an nsBaseChannel for some
time now?

Comment 16

[Darin Fisher]

Yes, I have been planning on refactoring the channel implementations to leverage
a common base class.  In fact, it almost seems that we could simplify the
protocol specific code to basically a set of meta data and an input stream.  I
haven't sorted out a solid plan yet, but I hope to make headway on something.

Comment 17

[Daniel Kraft]

What's about this? Should I try to change channels according to my 
nsChannelCommon (and create eventually subclasses for instance mURI-access), or 
do you want to go on with your plan?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Darin's doing a shared channel class in bug 312760 

Comment 19

[Bandan]

(In reply to comment #16)
> Yes, I have been planning on refactoring the channel implementations to
> leverage
> a common base class.  In fact, it almost seems that we could simplify the
> protocol specific code to basically a set of meta data and an input stream.  I
> haven't sorted out a solid plan yet, but I hope to make headway on something.
> 

Have you figured out any implementation details of the common base class. I would like to help on this. Do tell if you have any basic outline in your mind.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Bandan: see comment 18.

Comment 21

[Brandon Ferguson]

Attachment: Adds null checking to SetOriginalURI implementations

Here's a patch that checks for nulls on set. The check/error code is based off of Daniel's above. Everything seems to run and compile fine, but I need to run some tests still to make sure everything works as intended. Just wanted to post here to get some more eyes looking at it and make sure I'm on the right track for this bug. 

Thanks, 
Brandon

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'd use NS_ENSURE_ARG_POINTER(aURI) for this check (some of the channels already had that, btw).

Also, it looks like you're using tabs for indentation?  If so, please switch to spaces?

Last thing; you'll also want to make sure that GetOriginalURI actually never return null (so either mOriginalURI is always set or they return mURI when mOriginalURI is null or something).

Comment 23

[Brandon Ferguson]

Ah I had seen that used. Was unsure if it would have the desired effect. I'll switch what I used out and use NS_ENSURE_ARG_POINTER(aURI) from here on out.

Also you're correct I was using tabs, I'll switch to spaces, thanks for the heads up on that. 

As for the last part, do you mean something like this (as found in nsWyciwygChannel.cpp)?

  *aURI = mOriginalURI ? mOriginalURI : mURI;

It's also seen with an assertion warning if mOriginal is not set, would that be preferable? 
 

Comment 24

[Boris Zbarsky [:bzbarsky]]

I think setting the mOriginalURI at channel creation time, disallowing setting it to null,  and then just using it in GetOriginalURI() would probably be clearest.  But what nsWyciwygChannel.cpp does is fine too (though given the assertion and the checks you're adding, there's no need for the conditional there, right?).  Whichever is easier in a given channel impl is fine.

Comment 25

[Brandon Ferguson]

Attachment: Adds Null Checking and inits mOriginalURI

Excellent, thanks for the feedback. Here's a new version of the patch which: 

* Uses NS_ENSURE_ARG_POINTER 
* Initializes mOriginalURI the same as mURI (or mUrl)
* Simplifies the GetOriginalURI to simply return mOriginalURI instead of checking for null and then returning mURI if mOriginalURI is null

The only place where I had trouble is in nsExternalProtocolHandler.cpp which never seems to init mUrl or mOriginalURI. I looked for how this class is used but I couldn't find anything using it. However, I may have not been looking for the right thing.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> The only place where I had trouble is in nsExternalProtocolHandler.cpp

I'd just add members to it and set them.... this is used for handling URI schemes that we don't support, basically (by passing them to the OS).

Comment 27

[Boris Zbarsky [:bzbarsky]]

Brandon, you'll request review once you're happy with the patch, right?

Comment 28

[Brandon Ferguson]

(In reply to comment #27)
> Brandon, you'll request review once you're happy with the patch, right?


Yup, thanks for checking. Things got really busy around here for a few weeks so there hasn't been many updates. Should settle down in a week or so - then I should have a new patch for review. 

Comment 29

[ian]

Attachment: patch that inits mOriginalURI, checks for null on Set

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 342635 [details] [diff] [review]
patch that inits mOriginalURI, checks for null on Set 

Ian, you might want to add:

  [diff]
  git = 1
  showfunc = true
  unified = 8

to your ~/.hgrc to produce more readable diffs.

>+++ b/content/html/document/src/nsWyciwygChannel.cpp	Fri Oct 10 15:55:38 2008 -0400
> nsWyciwygChannel::GetOriginalURI(nsIURI* *aURI)
>-  // Let's hope this isn't called before mOriginalURI is set or we will
>-  // return the full wyciwyg URI for our originalURI  :S
>-  NS_ASSERTION(mOriginalURI, "nsWyciwygChannel::GetOriginalURI - mOriginalURI not set!\n");

We should probably leave the comment and assert that mOriginalURI != mURI instead of removing the assertion completely.

>+  *aURI = mOriginalURI;
>   NS_IF_ADDREF(*aURI);

Since we know mOriginalURI is non-null, that could be NS_ADDREF instead of NS_IF_ADDREF.  Same for a number of other GetOriginalURI methods in this patch.

>+++ b/modules/libjar/nsJARChannel.cpp	Fri Oct 10 15:55:39 >+    
>+    mOriginalURI = mJarURI;

No need to add the newline before that.  There's already a newline there in the file, right?

>+++ b/netwerk/base/src/nsBaseChannel.cpp	Fri Oct 10 15:55:39 2008 -0400
>@@ -377,16 +377,17 @@ nsBaseChannel::GetOriginalURI(nsIURI **a
>   *aURI = OriginalURI();
>   NS_IF_ADDREF(*aURI);

OriginalURI() should just returm mOriginalURI instead of what it does now, and this should be NS_ADDREF.  And SetURI should set mOriginalURI as well.

With those changes, looks good!

Comment 31

[Boris Zbarsky [:bzbarsky]]

Ian, if you post a patch with the comments addressed I'll get it checked in.

Comment 32

[ian]

Thanks for taking a look. I made the changes you asked for (to the best of my knowledge), and have posted a second version of the patch.

Comment 33

[ian]

Attachment: inits mOriginalURI, checkes for null on Set

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 343137 [details] [diff] [review]
inits mOriginalURI, checkes for null on Set

>+++ b/content/html/document/src/nsWyciwygChannel.cpp
> nsWyciwygChannel::GetOriginalURI(nsIURI* *aURI)
>   NS_ASSERTION(mOriginalURI, "nsWyciwygChannel::GetOriginalURI - mOriginalURI not set!\n");

This needs to assert that mOriginalURI != mURI.

>+++ b/uriloader/exthandler/nsExternalProtocolHandler.cpp
>nsExtProtocolChannel::GetOriginalURI(nsIURI* *aURI)
>+  NS_ADDREF(aURI);

Should be *aURI.  Sorry I missed this in the first pass....

r=bzbarsky; I'll make these two changes and the comment change to nsIChannel.idl and land this.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that was checked in

Pushed changeset 3b4a3bfc99a7.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Ian, thank you for getting this finished!