If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Channels should ensure that originalURI is non-null

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: bz, Assigned: ian)

Tracking

({helpwanted})

Trunk
helpwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 5 obsolete attachments)

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.
Whiteboard: [good first bug]

Updated

13 years ago
Keywords: helpwanted
Target Milestone: --- → Future

Comment 1

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.

Comment 3

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

Comment 4

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

Comment 5

13 years ago
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

13 years ago
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

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

Comment 8

13 years ago
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?

Comment 10

13 years ago
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...)

Comment 13

13 years ago
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

13 years ago
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?

Comment 16

13 years ago
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

13 years ago
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?
Darin's doing a shared channel class in bug 312760 
Depends on: 312760

Comment 19

12 years ago
(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.

Updated

11 years ago
Assignee: darin → nobody
QA Contact: benc → networking
Target Milestone: Future → ---

Comment 21

11 years ago
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).

Comment 23

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

Comment 25

11 years ago
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.
Attachment #261679 - Attachment is obsolete: true
> 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?

Comment 28

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

Comment 29

9 years ago
Created attachment 342635 [details] [diff] [review]
patch that inits mOriginalURI, checks for null on Set
Attachment #342635 - Flags: review?(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!
Attachment #342635 - Flags: review?(bzbarsky) → review+
Ian, if you post a patch with the comments addressed I'll get it checked in.
(Assignee)

Comment 32

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

Comment 33

9 years ago
Created attachment 343137 [details] [diff] [review]
inits mOriginalURI, checkes for null on Set
Attachment #342635 - Attachment is obsolete: true
Attachment #343137 - Flags: review?(bzbarsky)
Attachment #343137 - Flags: superreview+
Attachment #343137 - Flags: review?(bzbarsky)
Attachment #343137 - Flags: review+
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.
Attachment #169698 - Attachment is obsolete: true
Attachment #262153 - Attachment is obsolete: true
Attachment #343137 - Attachment is obsolete: true
Ian, thank you for getting this finished!
Assignee: nobody → montague.bugzilla
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.