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.
13 years ago
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.
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.
(In reply to comment #2) Okay I'll have some free time tomorrow and will start on this.
Is this bug still up-to-date? I'll look at this, although. Maybe I can help.
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!)
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.
I'm sorry, of coure it should be nsChannel instead of nsIChannel...
Created attachment 169698 [details] 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.
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?
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.
ah, yes... for that, you'd need a NS_STATIC_CAST to an intermediate class (nsIHttpChannel*, for example... or nsChannelCommon*)
(although, it does mean more vtable entries... hm...)
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.
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?
Darin, haven't you been talking about maybe having an nsBaseChannel for some time now?
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.
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?
(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.
Bandan: see comment 18.
Created attachment 261679 [details] [diff] [review] 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
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).
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?
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.
Created attachment 262153 [details] [diff] [review] 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.
> 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).
Brandon, you'll request review once you're happy with the patch, right?
(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.
Created attachment 342635 [details] [diff] [review] patch that inits mOriginalURI, checks for null on Set
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!
Ian, if you post a patch with the comments addressed I'll get it checked in.
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.
Created attachment 343137 [details] [diff] [review] inits mOriginalURI, checkes for null on Set
9 years ago
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.
Created attachment 343280 [details] [diff] [review] Patch that was checked in Pushed changeset 3b4a3bfc99a7.
Ian, thank you for getting this finished!