97677 - Need API to check whether a URI exists

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Akkana Peck]

For editor link checking, we need an API to check whether a URI is valid or not
(which usually means actually trying to open the file, do an http head request,
etc as appropriate for the scheme).  Darin and bbaetz have helped me with JS
code to open URIs asynchronously and check, but it's error prone and we all
agree that this logic really belongs in netlib instead of the editor, since
netlib will be a lot smarter about interpreting the various http status codes.

Comment 1

[Akkana Peck]

Darin suggests nsILinkChecker in netwerk/base/public, implemented by
nsLinkChecker in netwerk/base/source.

I have JS code in bug 91388 which does a basic implementation of this (no proxy
handling or other advanced conditions), so I'll translate it into C++ and put it
there.

Comment 2

[Akkana Peck]

Suggestions/hints from bbaetz via IRC:
*bbaetz*
well, we should differenciate between user cancelled events
*bbaetz*
and non-user-cancelled events
*bbaetz*
and have three states: OK, not OK, and unchecked
*bbaetz*
you'll need that anyway
*»bbaetz*
User cancelled means something like we had to go through a proxy interactively
and the user chose not to?
*bbaetz*
yes
*akk*
Should "timed out" be different from "definitely not there"?
*bbaetz*
and if their net connection is down, then just flag all the links as dead, I guess
*bbaetz*
um
*bbaetz*
hmm
*akk*
(As long as I have to return something other than boolean)
*bbaetz*
thats up to you, I guess
*bbaetz*
you should disable this when the user is offline, while you;re at it
*akk*
How do I find out if we're offline?
*akk*
(I was just going to figure that the user shouldn't call this if we're offline. :-)
*bbaetz*
its an attribute on the ioservice
*bbaetz*
but you can also register as an observer, and be told when this state changes
*bbaetz*
so you can enable/disable the menu item

Comment 3

[Akkana Peck]

Oh, he also pointed out earlier in the conversation (and it's worth recording)
that it would be nice if knowledge of particular protocols were handled at the
protocol level, rather than this base class knowing all about the details of
http and whatever other protocols might have special cases.  I'm not going to
worry about that in the initial nsILinkChecker implementation (unless netlib
people think I should), but that does suggest that adding something to
nsIChannel might possibly be a better solution in the long run.

Comment 4

[Darin Fisher]

yes, and the link checker code could simply use that API in the future.

Comment 5

[Akkana Peck]

Attachment: nsIURIChecker.idl

Comment 6

[Akkana Peck]

Attachment: nsURIChecker.h

Comment 7

[Akkana Peck]

Attachment: nsURIChecker.cpp

Comment 8

[Akkana Peck]

Attachment: Diffs to existing files under netwerk

Comment 9

[Akkana Peck]

Attachment: Diffs to ComposerCommands.js to test the new classes

Comment 10

[Darin Fisher]

akkana:

1- please use NS_GENERIC_FACTORY_CONSTRUCTOR instead of nsURIChecker::Create

2- please do not override the meaning of NS_BINDING_XXX... instead, you should
invent new status codes, declared in nsIURIChecker.

Comment 11

[Darin Fisher]

actually, i'm only concerned about NS_BINDING_REDIRECTED... it doesn't apply to
URI checking.  otherwise, using NS_BINDING_SUCCEEDED, NS_BINDING_FAILED, and
NS_BINDING_ABORTED is ok.

Comment 12

[Darin Fisher]

also, if you want to generate a single diff, you can 'cvs add' your new files to
your local tree, and then use 'cvs diff -uN' to generate a diff containing the
new files.

Comment 13

[Akkana Peck]

I've attached the new files and the diffs to build and create the new class.  I
tried to follow the guidelines Darin and Bradley offered (thanks for all the
help!) -- please let me know if I misunderstood or things need changing.  If the
API looks okay, then I'm seeking review/sr.

I've also attached the patch to editor/ui/composer/content/ComposerCommands.js
which I'm using to test this.  Apply, rebuild in editor, run mozilla -editor
[url] on a url which has a few links in it (ideally, some bad, some good), then
do a Debug->Check Links and watch stdout (there's no UI as yet).  I'll be
getting review from editor folks for that code, but if one of you has time to
review the composer code as well as the netwerk code to make sure I'm not doing
something silly, the double-check would be much appreciated.

Comment 14

[Akkana Peck]

(Working on the changes Darin suggested ...)

Comment 15

[Akkana Peck]

Attachment: Netlib changes, incorporating Darin's suggestions

Comment 16

[Charles Manske]

Unfortunately, there's a new pattern for "ToUnicode" that appeared in today's 
tree update. E.g.:
name.ToNewUnicode()
should be changed to:
ToNewUnicode(name);
and you include
#include "nsReadableUtils.h"                                                    
in files that need this.
See bug 100476.

Comment 17

[Akkana Peck]

Attachment: Patch incorporating new string APIs

Comment 18

[Akkana Peck]

Attachment: Patch with new string APIs AND new license.

Comment 19

[Charles Manske]

Comment on attachment 51751 [details] [diff] [review]
Patch with new string APIs AND new license.

r=cmanske

Comment 20

[Darin Fisher]

Comment on attachment 51744 [details] [diff] [review]
Patch incorporating new string APIs

>Index: base/public/nsIIOService.idl

>+interface nsILinkChecker;
>+interface nsIObserver;

these lines can be deleted.



>Index: base/public/nsIURIChecker.idl

>+   *   NS_BINDING_IN_PROGRESS: in progress, don't know yet

nsIRequest::isPending() should be used to convey this state.

>+%{C++
>+
>+#define NS_URICHECKER_CONTRACT_ID "@mozilla.org/network/urichecker;1"
>+#define NS_URICHECKER_CID                     \
>+{ /* cf3a0e06-1dd1-11b2-a904-ac1d6da77a02 */         \
>+    0xcf3a0e06,                                      \
>+    0x1dd1,                                          \
>+    0x11b2,                                          \
>+    {0xa9, 0x04, 0xac, 0x1d, 0x6d, 0xa7, 0x7a, 0x02} \
>+}

please add this to nsNetCID.h



>+#define NS_BINDING_IN_PROGRESS NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_NETWORK, 5)

remove.. see comments above.



>Index: base/src/nsURIChecker.cpp

>+static NS_DEFINE_CID(kIOServiceCID, NS_IOSERVICE_CID);

use nsNetUtil.h:do_GetIOService to eliminate yet another static kIOServiceCID


>+//Interfaces for addref and release and queryinterface

minor nit... how about:

//Implementations for addref, release, and queryinterface



>+nsURIChecker::nsURIChecker()
>+{
>+    NS_INIT_REFCNT();
>+    mStatus = NS_BINDING_REDIRECTED;
>+    mIsPending = PR_TRUE;
>+}

you probably want to just initialize mStatus to NS_OK and mIsPending to FALSE.
mIsPending is TRUE once AsyncCheckURI has been called.


>+nsURIChecker::~nsURIChecker()
>+{
>+    Cancel(NS_BINDING_ABORTED);
>+}

calling cancel is meaningless since nsURIChecker implements nsIStreamListener.
ie., if the destructor of nsURIChecker has been called, then mChannel must
not be holding a reference back to |this|, and if it is not holding a reference
to |this|, then the channel must already have finished.


>+nsresult
>+nsURIChecker::SetStatusAndCallBack(PRUint16 aStatus)
>+{
>+    mStatus = aStatus;
>+    mIsPending = PR_FALSE;
>+
>+    mObserver->OnStartRequest((nsIRequest*)this, 0);
>+    mObserver->OnStopRequest((nsIRequest*)this, 0, mStatus);
>+
>+    return NS_OK;
>+}

why is the argument to this function not a nsresult?
also, why not make the function return void?
and, it is better style to use NS_STATIC_CAST instead of C style casts.

this function has made me realize the following:
AsyncCheckURI should take a third argument... an nsISupports context parameter
similar to the context parameter of nsIChannel::AsyncOpen... this is an opaque
parameter that should be passed to the nsIRequestObserver::OnSt{art,op}Request
methods.  please add this parameter to AsyncCheckURI.



>+/////////////////////////////////////////////////////
>+// nsIURI methods
>+//

nsIURIChecker methods, right?


>+NS_IMETHODIMP
>+nsURIChecker::AsyncCheckURI(const char* aURI, nsIRequestObserver *aObserver,
>+                            nsIRequest** aRequestRet)
>+{
>+    nsresult rv;
>+
>+    mIsPending = PR_TRUE;
>+    mStatus = NS_BINDING_REDIRECTED;

initialize mStatus to NS_OK


>+    mObserver = aObserver;
>+    if (aRequestRet)
>+        *aRequestRet = this;

you need to AddRef *aRequestRet



>+    // Get the IO Service:
>+    if (!mIOS) {
>+        mIOS = do_GetService(kIOServiceCID, &rv);
>+        if (NS_FAILED(rv)) return rv;
>+    }
>+    if (!mIOS) return NS_ERROR_UNEXPECTED;

you don't need to cache the IO service as a member variable... there is no need...
you don't use it anywhere else.  just use a local nsCOMPtr, and use do_GetIOService
instead of do_GetService (see comments above).


>+    // Make the URI
>+    rv = mIOS->NewURI(aURI, 0, getter_AddRefs(mURI));
>+    if (NS_FAILED(rv)) return rv;
>+    else if (!mURI) return NS_ERROR_UNEXPECTED;

you do not need this extra level of error checking... just check for either
NS_FAILED(rv) or !mURI... but not both.

also, i don't think that you need to keep the URI as a member variable.


>+    // Make a new channel:
>+    rv = mIOS->NewChannelFromURI(mURI, getter_AddRefs(mChannel));
>+    if (NS_FAILED(rv)) return rv;
>+    if (!mChannel) return NS_ERROR_UNEXPECTED;

again, no need for the extra error checking... just use if (NS_FAILED(rv)) ...


>+NS_IMETHODIMP
>+nsURIChecker::GetName(PRUnichar** aName)
>+{
>+    char* nameStr;
>+    nsresult rv = mURI->GetSpec(&nameStr);
>+    if (NS_FAILED(rv)) return rv;
>+
>+    nsString name;
>+    name.AssignWithConversion(nameStr);
>+    Recycle(nameStr);
>+    *aName = ToNewUnicode(name);
>+    return *aName ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
>+}

just call mChannel->GetName()




>+NS_IMETHODIMP nsURIChecker::Cancel(nsresult status)
>+{
>+    nsCOMPtr<nsIRequest> request = do_QueryInterface(mChannel);
>+    if (request)
>+        return request->Cancel(status);
>+    else return NS_ERROR_NOT_IMPLEMENTED;
>+}

this QueryInterface is not necessary... mChannel is a nsIRequest in the C++ sense.


>+NS_IMETHODIMP nsURIChecker::Suspend()
>+{
>+    nsCOMPtr<nsIRequest> request = do_QueryInterface(mChannel);
>+    if (request)
>+        return request->Suspend();
>+    else return NS_ERROR_NOT_IMPLEMENTED;
>+}

again, no QI needed.


>+NS_IMETHODIMP nsURIChecker::Resume()
>+{
>+    nsCOMPtr<nsIRequest> request = do_QueryInterface(mChannel);
>+    if (request)
>+        return request->Resume();
>+    else return NS_ERROR_NOT_IMPLEMENTED;
>+}

again, no QI needed.


>+NS_IMETHODIMP nsURIChecker::GetLoadGroup(nsILoadGroup **aLoadGroup)
>+{
>+    nsCOMPtr<nsIRequest> request = do_QueryInterface(mChannel);
>+    if (request)
>+        return request->GetLoadGroup(aLoadGroup);
>+    else return NS_ERROR_NOT_IMPLEMENTED;
>+}

again, no QI needed.


>+NS_IMETHODIMP nsURIChecker::SetLoadGroup(nsILoadGroup *aLoadGroup)
>+{
>+    nsCOMPtr<nsIRequest> request = do_QueryInterface(mChannel);
>+    if (request)
>+        return request->SetLoadGroup(aLoadGroup);
>+    else return NS_ERROR_NOT_IMPLEMENTED;
>+}

again, no QI needed.


>+NS_IMETHODIMP nsURIChecker::GetLoadFlags(nsLoadFlags *aLoadFlags)
>+{
>+    nsCOMPtr<nsIRequest> request = do_QueryInterface(mChannel);
>+    if (request)
>+        return request->GetLoadFlags(aLoadFlags);
>+    else return NS_ERROR_NOT_IMPLEMENTED;
>+}

again, no QI needed.


>+NS_IMETHODIMP nsURIChecker::SetLoadFlags(nsLoadFlags aLoadFlags)
>+{
>+    nsCOMPtr<nsIRequest> request = do_QueryInterface(mChannel);
>+    if (request)
>+        return request->SetLoadFlags(aLoadFlags);
>+    else return NS_ERROR_NOT_IMPLEMENTED;
>+}

again, no QI needed.


>+NS_IMETHODIMP
>+nsURIChecker::OnStartRequest(nsIRequest *aRequest, nsISupports *aCtxt)
>+{
>+    //printf("OnStartRequest\n");

please either use PR_LOG, wrap this line w/ #ifdef DEBUG_akkana, or delete it.


>+    // DNS errors and other obvious problems will return failure status
>+    nsresult status;
>+    nsresult rv = aRequest->GetStatus(&status);
>+    if (NS_FAILED(rv) || status != 0) {

do not test status against 0... you want NS_FAILED(status)



>+    // We don't want to read the actual data, so cancel now:
>+    aRequest->Cancel(NS_BINDING_ABORTED);

returning any error code from OnStartRequest will also cancel the request.



>+    // If status is zero, it might still be an error if it's http:
>+    // http has data even when there's an error like a 404.
>+    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(aRequest);
>+    if (!httpChannel) {
>+        return SetStatusAndCallBack(NS_BINDING_SUCCEEDED);
>+    }
>+    PRUint32 responseStatus;
>+    rv = httpChannel->GetResponseStatus(&responseStatus);
>+    if (NS_FAILED(rv)) {
>+        return SetStatusAndCallBack(NS_BINDING_FAILED);
>+    }
>+    // If it's below 100 or between 200-299, it's valid:
>+    if (responseStatus < 100 ||
>+        (responseStatus >= 200 && responseStatus < 300)) {
>+        return SetStatusAndCallBack(NS_BINDING_SUCCEEDED);
>+    }

there are no http response codes below 100... this might be a better check:

      if (responseStatus / 100 == 2) {
          // link is valid
      }


>+    // If we got a 404 (not found), we need some extra checking:
>+    // toplevel urls from Netscape Enterprise Server 3.6,
>+    // like http://www.mozilla.org, generate a 404
>+    // and will have to be retried without the head.
>+    if (responseStatus == 404)
>+    {
>+        char* server = 0;
>+        rv = httpChannel->GetResponseHeader("Server", &server);
>+        if (NS_SUCCEEDED(rv)) {
>+            if (!PL_strcmp(server, "Netscape-Enterprise/3.6")) {

would PL_strcasecmp be better?  just in case the value somehow gets lowercased.


>+                mStatus = NS_BINDING_REDIRECTED;

mStatus = NS_OK ???


>+                // Open a new channel for a real (not head) request:
>+                nsCOMPtr<nsIChannel> channel;

remember: this is an edge case.. there is no problem calling do_GetIOService
again for this edge case.  also, you can get the URI from mChannel->GetOriginalURI().


>+                rv = mIOS->NewChannelFromURI(mURI, getter_AddRefs(channel));
>+                if (NS_FAILED(rv)) return rv;
>+                if (!channel) return NS_ERROR_UNEXPECTED;

again, no need for the extra error checking.. if (NS_FAILED(rv)) should do.

>+                return channel->AsyncOpen(this, nsnull);
>+            }
>+        }
>+
>+        // Else it was a normal 404, so return as expected
>+        return SetStatusAndCallBack(NS_BINDING_FAILED);
>+    }
>+
>+    // If we get here, then it's an http channel, not a 100, 200 or 404.
>+    // Treat it as an error.
>+    mStatus = NS_BINDING_FAILED;

don't you want call SetStatusAndCallback in this case?  401 and 407 may require
special treatment since checking the link would require the user to enter their
username and password.



>+NS_IMETHODIMP
>+nsURIChecker::OnStopRequest(nsIRequest *request, nsISupports *ctxt,
>+                             nsresult statusCode)
>+{
>+    //printf("OnStopRequest\n");

please clean up this line as above.




>+
>+// OnDataAvailable shouldn't generally be called,
>+// since we use head requests whenever possible,
>+// but in practice we're seeing it every time.
>+NS_IMETHODIMP
>+nsURIChecker::OnDataAvailable(nsIRequest *aRequest, nsISupports *aCtxt,
>+                               nsIInputStream *aInput, PRUint32 aOffset,
>+                               PRUint32 aCount)
>+{
>+#ifdef DEBUG_akkana
>+    PRUnichar* uri;
>+    GetName(&uri);
>+    nsString uristr(uri);
>+    char* cstr = ToNewCString(uristr);
>+    printf("OnDataAvailable: %s\n", cstr);
>+    Recycle(cstr);
>+#endif
>+    // If we've gotten here, something went wrong with the previous cancel,
>+    // so try again:
>+    aRequest->Cancel(NS_BINDING_ABORTED);
>+    return NS_OK;
>+}

returning a failure code also cancels the request.



>Index: base/src/nsURIChecker.h

>+ * Contributor(s): 
       akkana ...


>Index: build/nsNetModule.cpp

looks good

Comment 21

[Akkana Peck]

Attachment: Patch revised per Darin's suggestions

Comment 22

[Darin Fisher]

Comment on attachment 51789 [details] [diff] [review]
Patch revised per Darin's suggestions

sr=darin w/ the 2 changes i mentioned over IRC

1- fix comment to remove less than 100

2- remove changes to nsIIOservice.idl

Comment 23

[Akkana Peck]

Fixed.