Comment 325 for bug 195698

(From update of attachment 356180)

This patch is complicated enough that it really needs test.

--
>+ , mAuthRetryAsync(PR_FALSE)

I'm not sure what the point of this variable is. You only check it in OnCredentialsAcquired, and when would that be called with mAuthRetryAsync == PR_FALSE?

>+ if (NS_ERROR_IN_PROGRESS == rv) {

Style in this file is generally the other way round:
  if (rv == NS_ERROR_IN_PROGRESS)

>+ mAuthRetryAsync = PR_TRUE;
>+ return Suspend();

So, this means that cancelling a channel while it's waiting for an auth prompt does not immediately cancel it. That seems unfortunate. Can you find a fix that doesn't require suspending the channel?

>+ else if (NS_ERROR_IN_PROGRESS == rv)
>+ return rv;

Same style comment as above.

>+ nsRefPtr<nsHttpHandler::PromptForIdentityLock> promptLock;

Don't really see why this has to be refcounted...

>+ /*
>+ gHttpHandler keeps a hash table of prompt tracks ('locks') for

Multiline comment style in this file is using C++ comments, i.e. a // on each line.

>+ each "realm | authType".

Why just for each realm + authType, why not also hostname?

I also note that this will do nothing to fix the master PW problem mentioned in the bug. If you have two URLs with different realms you still have the problem, right?

>+ is still open (awating user input) it is not allowed to popup

awating -> awaiting

>+ if (promptLock)
>+ promptLock->CredentialsAcquired(rv);

I know it's not strictly necessary to call this when the user cancelled the dialog, but it may still be better to preserve the original error code.

>diff --git a/netwerk/protocol/http/src/nsHttpHandler.cpp b/netwerk/protocol/http/src/nsHttpHandler.cpp
>--- a/netwerk/protocol/http/src/nsHttpHandler.cpp
>+++ b/netwerk/protocol/http/src/nsHttpHandler.cpp
>+ nsCAutoString key(aRealm);
>+ key.AppendLiteral("|");

key.Append('|');

>+ key.Append(aAuthType);

>diff --git a/netwerk/protocol/http/src/nsHttpHandler.h b/netwerk/protocol/http/src/nsHttpHandler.h
>--- a/netwerk/protocol/http/src/nsHttpHandler.h
>+++ b/netwerk/protocol/http/src/nsHttpHandler.h
>@@ -42,36 +42,40 @@
>+#include "nsHashKeys.h"
>+#include "nsBaseHashtable.h"

Shouldn't you instead use nsClassHashtable? As in:

  nsClassHashtable<nsCStringHashKey, PromptForIdentityLock>

>+ class PromptForIdentityLock : public nsISupports

I think this would be simpler if you notified the channels in CredentialsReceived instead of in the destructor.

Why did you derive this from nsISupports? I don't really see what advantages the refcounting gives you here, and you don't care about QI either.

And why the split between this and PromptForIdentityLockInternal?

Also, classes should be declared at the beginning of a public/protected/internal section, not in the middle of functions.

+ nsCOMArray<nsIHttpChannel> mChannels;

Why not use an array of nsHttpChannel instead? As in nsTArray<nsRefPtr<nsHttpChannel> >, although I don't see why this can't be nsTArray<nsHttpChannel*>.

>+ typedef nsBaseHashtable<nsCStringHashKey, PromptForIdentityLockInternal*, PromptForIdentityLockInternal*> TPromptLockHashtable;

As mentioned I think this should use nsClassHashtable.

Why the T prefix? That seems highly unsual in Mozilla code. PromptLockHashtable seems good enough.