224240 - nsURIChecker cleanup

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Darin Fisher]

nsURIChecker does a lot of wierd stuff.  not sure what i was smoking when i
reviewed it originally.  patch coming up...

Comment 1

[Darin Fisher]

Attachment: v0 patch -- unfinished

this is an unfinished patch; i'm just posting it here for feedback in case
anyone has interest in further modifications to the interface.	i've made the
following changes:

  1) added an init method which takes only a nsIURI
  2) replaced asyncCheckURI with asyncCheck, which only takes a 
     nsIRequestObserver and context parameter.
  3) replaced baseRequest with baseChannel.

separating these gives consumers the ability to tweak the baseChannel before
calling asyncCheck.  also, this init/asyncXXX pattern is more consistent with
other necko interfaces.

next patch will include changes to the consumers of nsIURIChecker.

Comment 2

[Darin Fisher]

i forgot to mention that this patch tries to fix some bugs (leaks and borkage)
that can occur when we hit the 404 w/ a NES 3.6 server problem.

Comment 3

[Darin Fisher]

Attachment: v1 patch

Comment 4

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

+    // If we get here, then it's an http channel, not a 100, 200 or 404.

hmm, as I read the patch this will possibly be a 404 at this place (unless the
server is NES 3.x)

Comment 5

[Darin Fisher]

yeah, i just carried that comment over... it isn't accurate :(

Comment 6

[Darin Fisher]

i've fixed the incorrect comment in my tree.

Comment 7

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

Comment on attachment 134556 [details] [diff] [review]
v1 patch

+	     rv  = lastChannel->GetOriginalURI(getter_AddRefs(uri));
+	     rv |= lastChannel->GetLoadFlags(&loadFlags);

I don't really like this, it makes it impossible to find out the real return
value... but it's your module.

+		 rv |= mChannel->SetLoadFlags(loadFlags);

what if SetLoadFlags fails but AsyncCheck succeeds - you end up not returning
from this function, and will return NS_BINDING_FAILED later. is that what you
want? it will cause an OnStopRequest immediately, with a failure code, afaict.

+    rv = ios->NewChannelFromURI(aURI, getter_AddRefs(mChannel));
hm... why not NS_NewChannel?

in AsyncCheck
+	 mStatus = NS_OK;

wasn't this done in the constructor already?

-    // break cycle if we fail to open the channel.
-    if (NS_FAILED(rv))
-	 mChannel = nsnull;
don't you still have to do this? or will all impls of AsyncOpen release their
reference to the notificationCallbacks if they failed?


doesn't the nsIRequest API require the request to add itself to the loadgroup
while pending? you don't seem  to do that...

+    NS_ASSERTION(aRequest == mChannel, "unexpected request");

hm, this will be the case even for redirects? for OnStopRequest, this is a
simple if.

+	 // break reference cycle between us and the channel (see comment in
don't you need to release the reference to mObserver and mObserverContext as
well?

Comment 8

[Darin Fisher]

>(From update of attachment 134556 [details] [diff] [review])
>+           rv  = lastChannel->GetOriginalURI(getter_AddRefs(uri));
>+           rv |= lastChannel->GetLoadFlags(&loadFlags);
>
>I don't really like this, it makes it impossible to find out the real return
>value... but it's your module.
>
>+               rv |= mChannel->SetLoadFlags(loadFlags);
>
>what if SetLoadFlags fails but AsyncCheck succeeds - you end up not returning
>from this function, and will return NS_BINDING_FAILED later. is that what you
>want? it will cause an OnStopRequest immediately, with a failure code, afaict.
 
good catch.  thx!
 
 
>+    rv = ios->NewChannelFromURI(aURI, getter_AddRefs(mChannel));
>hm... why not NS_NewChannel?
 
because that (maybe) adds bloat.  the function is inline, and it does a lot
more than we need here.  we only need a new channel, we don't need to set load
flags, etc.  now, iirc MSVC actually won't inline NS_NewChannel.  instead, it
will prefer to share the code amongst all the callers, and thereby only add the
code to necko.dll once.  in that case it would be less code.
 
 
>in AsyncCheck
>+       mStatus = NS_OK;
>
>wasn't this done in the constructor already?
 
yup... fixed.
 
 
>-    // break cycle if we fail to open the channel.
>-    if (NS_FAILED(rv))
>-       mChannel = nsnull;
>don't you still have to do this? or will all impls of AsyncOpen release their
>reference to the notificationCallbacks if they failed?
 
you are correct.
 
 
>doesn't the nsIRequest API require the request to add itself to the loadgroup
>while pending? you don't seem  to do that...
 
well, not exactly.  the only real requirement is that the request be canceled
when the load group is canceled.  in this case, the request inserts another
request (the channel), which when canceled causes the nsURIChecker to also
be canceled.  that is sufficient.
 
 
>
>+    NS_ASSERTION(aRequest == mChannel, "unexpected request");
>
>hm, this will be the case even for redirects? for OnStopRequest, this is a
>simple if.
                                                                               
                                                      
OnStopRequest is not called by the redirected channel.  that's why HTTP has a
OnRedirect, so consumers can intercept a redirect.  (i once considered making
OnStopRequest fire for the redirected channel, but i quickly decided against it
once i realized how many OnStopRequest and OnStartRequest implementations
simply didn't care for the information.)
 
 
>
>+       // break reference cycle between us and the channel (see comment in
>don't you need to release the reference to mObserver and mObserverContext as
>well?
 
see SetStatusAndCallback, which is called from OnStartRequest once we have
enough information to notify the observer.  since we must always notify the
observer, it seemed appropriate to discard our reference to the observer
immediately after notifying them.

Comment 9

[Darin Fisher]

>+           rv  = lastChannel->GetOriginalURI(getter_AddRefs(uri));
>+           rv |= lastChannel->GetLoadFlags(&loadFlags);
>
>I don't really like this, it makes it impossible to find out the real return
>value... but it's your module.

i forgot to comment on this one... the idea is that the code only cares if it
can successfully create and open the new channel.  if it cannot do so, then it
needs to return with NS_BINDING_FAILED.  the value of |rv| itself is
inconsequential.  

anyways, if you wanted to debug this code you could always single step to see
the value of returned from each function.

Comment 10

[Darin Fisher]

Attachment: v1.1 patch

revised per comments from biesi.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134679 [details] [diff] [review]
v1.1 patch

>Index: netwerk/base/src/nsURIChecker.cpp

>+ServerIsNES3x(nsIHttpChannel *httpChannel)
>+    // case sensitive string comparision is OK here.  the server string

"comparison"

>+nsURIChecker::CheckStatus()

>+                rv  = Init(uri);
>+                rv |= mChannel->SetLoadFlags(loadFlags);

If the NS_NewChannel() in Init() fails, this will crash, no?  Since
getter_AddRefs will null out the mChannel pointer...

sr=bzbarsky with those issues fixed.

Comment 12

[Darin Fisher]

patch checked in on trunk.

Comment 13

[Simon Paquet [:sipaq]]

v1.1 patch has sr.