Move Negotiate auth off main thread

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: dwmw2, Assigned: jhorak)

Tracking

(Blocks 2 bugs)

40 Branch
mozilla49
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 wontfix, firefox46 wontfix, firefox47 wontfix, firefox48 affected, firefox49 fixed, firefox-esr38 wontfix, firefox-esr45 affected, firefox50 fixed)

Details

(Whiteboard: [bugday-20131016][necko-active][ntlm])

Attachments

(1 attachment, 13 obsolete attachments)

37.80 KB, patch
jhorak
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130624104218

Steps to reproduce:

Enable Kerberos authentication.

Point browser at a host which isn't registered in Kerberos database, so Kerberos fails and we have to fall back to NTLM authentication (which is working through /usr/bin/ntlm_auth once I work around bug 554122).




Actual results:

Page takes about 90 seconds to load. During which times, the entire browser often locks up for tens of seconds at a time, failing to redraw itself or respond to any user interaction (cannot even change tabs).

If I run it in gdb and stick a breakpoint on gss_init_sec_context(), I can see that it's calling gss_init_sec_context() for the same host *ONE HUNDRED AND FORTY TIMES*. Each attempt takes between a third of a second and about five seconds. It continues to do this even after it's already given up once and started using /usr/bin/ntlm_auth.


Expected results:

Observe failure. Stop trying.

And why in $DEITY's name is it doing this blocking network operation from the main thread and not even redrawing itself while it's going on? Even if it *were* only doing it once, that would be wrong.
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
Whiteboard: [bugday-20131016]
Blocks: 520668
Version: 22 Branch → 40 Branch
Depends on: 1217383
Blocks: 1217383
No longer depends on: 1217383
Whiteboard: [bugday-20131016] → [bugday-20131016][necko-backlog][ntlm]
Duplicate of this bug: 1014061
Summary: Kerberos auth unusably slow when it fails → Move Negotiate auth off main thread
I'm trying to move negotiate authorization of the main thread. Attaching wip patch to check if I'm on the right track. Please could you check if that's viable solution? Thank you in advance.
Attachment #8733345 - Flags: feedback?(honzab.moz)
Comment on attachment 8733345 [details] [diff] [review]
wip: negotiate-off-main-thread

please:
- use 8 lines context for the patch
- add nsIAuthNegotiateCallback.idl file (and any newly introducing, if more)
- if you can, remove any commented-out stuff (e.g. in nsAuthGSSAPI.cpp)
- add some more detailed description of what the patch does to bugzilla during submit, helps with even just a feedback
Attachment #8733345 - Flags: feedback?(honzab.moz) → feedback-
Thanks for the quick reply. Sorry about the missing file and small context. I've moved from hg to git and that's the cause.

* I've added nsHttpNegotiateAuth::GenerateCredentialsAsync which is called before GenerateCredentials in nsHttpChannelAuthProvider::GenCredsAndSetEntry because GSSAPI calls in GetNextToken can sometimes take quite a long time and whole UI is blocked during it. 

* The GenerateCredentialsAsync will create an instance of GetNextTokenCompleteEvent (nsICancelableRunnable) which is returned to caller to allow cancelling request and also instance of GetNextTokenRunnable which is executed in worker thread.

* GetNextTokenRunnable::Run calls nsHttpNegotiateAuth::GenerateCredentials and when call is done it dispatches instance of GetNextTokenCompleteEvent to the main thread with results.

* The GetNextTokenCompleteEvent::Run calls mCallback->OnCredsAvailable(creds, flags). The mCallback is instance of nsIAuthNegotiateCallback currently implemented by nsHttpChannelAuthProvider.

* There's only one worker thread (named NegotiateAuth) owned by nsHttpNegotiateAuth.

* Changes in nsHttpBasicAuth.cpp are mainly because of introducing nsCOMPtr instead of passing pointer to pointer in case of mAuthContinuationState and mProxyAuthContinuationState. Relevant changes needs to be also done for sessionState.

* Also in nsHttpChannelAuthProvider::GenCredsAndSetEntry the GenerateCredentialsAsync is called and if it doesn't fail with NS_ERROR_IN_PROGRESS the GenerateCredentials is called as a fallback. Currently other auth methods like Basic, Digest and NTLM does not implement GenerateCredentialsAsync.

Please let me know if something is unclear.
Assignee: nobody → jhorak
Attachment #8733345 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8733779 - Flags: feedback?(honzab.moz)
Comment on attachment 8733779 [details] [diff] [review]
wip:negotiate-off-main-thread-2

Review of attachment 8733779 [details] [diff] [review]:
-----------------------------------------------------------------

seems more or less a correct approach, still some change are imo not needed and some changes need to be explained first.

::: extensions/auth/nsHttpNegotiateAuth.cpp
@@ +203,5 @@
> +        // Runs on main thread
> +        MOZ_ASSERT(NS_IsMainThread());
> +        mCallback->OnCredsAvailable(mCreds, mFlags);
> +        return NS_OK;
> +    }

nsCancelableRunnable doesn't implement the cancel logic for you.  you are responsible to write it yourself.  when you do it, be very careful about threading and access to the mCreds member!

I would personally prefer that you:
- assert in each method a thread you expect it to be called on
- encapsulate the SetResult and this runnable dispatch inside the class

the code style is:

void Method()
{
}

void OtherMethod()
{
}

these classes you don't expose out of this module must be inside an anonymous namespace.

@@ +228,5 @@
> +                             const char16_t *username,
> +                             const char16_t *password,
> +                             nsISupports **sessionState,
> +                             nsISupports *continuationState,
> +                             nsISupports *aCompleteEvent

why nsISupports and not directly your concrete class type?  I don't see a reason (saves static_cast)

@@ +230,5 @@
> +                             nsISupports **sessionState,
> +                             nsISupports *continuationState,
> +                             nsISupports *aCompleteEvent
> +                             ):
> +            mAuthChannel(authChannel),

the style is:

       Type aLastArg)
  : mMember(..)
  , mNextMemeber
{
}

@@ +250,5 @@
> +           nsAutoCString contractId;
> +           contractId.Assign(NS_HTTP_AUTHENTICATOR_CONTRACTID_PREFIX);
> +           contractId.Append("negotiate");
> +           nsresult rv;
> +           nsCOMPtr<nsIHttpAuthenticator> authenticator = do_GetService(contractId.get(), &rv);

hmm.. to speed this up a bit maybe let this runnable reference nsHttpNegotiateAuth at its dispatch time.

@@ +251,5 @@
> +           contractId.Assign(NS_HTTP_AUTHENTICATOR_CONTRACTID_PREFIX);
> +           contractId.Append("negotiate");
> +           nsresult rv;
> +           nsCOMPtr<nsIHttpAuthenticator> authenticator = do_GetService(contractId.get(), &rv);
> +           NS_ENSURE_SUCCESS(rv, rv);

this goes nowhere :)

you must handle these failures by sending a failure callback back to the main thread (probably null credentials as you do below?).

Move this code to its own method and let Run() be as:

Run()
{ 
  nsresult rv = Do(&creds, &flags);
  if (NS_FAILED(rv)) {
    return mCompleteEvent->DispatchError();
  } 

  return mCompleteEvent->DispatchSuccess(creds, flags);
}

@@ +320,5 @@
> +   if (!mNegotiateThread) {
> +       nsresult rv = NS_NewNamedThread("NegotiateAuth", getter_AddRefs(mNegotiateThread));
> +       NS_ENSURE_SUCCESS(rv, rv);
> +   }
> +   mNegotiateThread->Dispatch(getNextTokenRunnable, NS_DISPATCH_NORMAL);

this can fail as well

@@ +321,5 @@
> +       nsresult rv = NS_NewNamedThread("NegotiateAuth", getter_AddRefs(mNegotiateThread));
> +       NS_ENSURE_SUCCESS(rv, rv);
> +   }
> +   mNegotiateThread->Dispatch(getNextTokenRunnable, NS_DISPATCH_NORMAL);
> +   return NS_ERROR_IN_PROGRESS;

Return NS_OK here, the method made its job.  or explain the rational for this error be returned here.

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +744,2 @@
>                                   &identityInvalid);
> +    SetAuthorizationMembers(proxyAuth, continuationState);

why this change?

@@ +1261,5 @@
> +{
> +    mNegotiateAsyncCancelable = nullptr;
> +    // When channel is closed, do not proceed
> +    if (!mAuthChannel)
> +        return NS_OK;

if () {
}

strictly...

::: netwerk/protocol/http/nsIHttpAuthenticator.idl
@@ +146,5 @@
>                                 in    wstring        aDomain,
>                                 in    wstring        aUser,
>                                 in    wstring        aPassword,
>                                 inout nsISupports    aSessionState,
> +                               in    nsISupports    aContinuationState,

please explain this change.  this more seams to me like a cleanup only.  if it's the case, then I'm not a big fan of it.  if you want to do this cleanup, please do it in a separate bug.
Attachment #8733779 - Flags: feedback?(honzab.moz) → feedback+
(In reply to Honza Bambas (:mayhemer) from comment #5)
> I would personally prefer that you:
> - encapsulate the SetResult and this runnable dispatch inside the class
Could you please explain this a little more? You mean instead of SetResult use later mentioned DispatchError and DispatchSuccess and doing NS_DispatchToMainThread(this[GetNextTokenCompleteEvent]) in these methods?

> @@ +250,5 @@
> > +           nsAutoCString contractId;
> > +           contractId.Assign(NS_HTTP_AUTHENTICATOR_CONTRACTID_PREFIX);
> > +           contractId.Append("negotiate");
> > +           nsresult rv;
> > +           nsCOMPtr<nsIHttpAuthenticator> authenticator = do_GetService(contractId.get(), &rv);
> 
> hmm.. to speed this up a bit maybe let this runnable reference
> nsHttpNegotiateAuth at its dispatch time.
That would require to add GetNextTokenRunnable as friend class to nsHttpNegotiateAuth. I've tried it before (because the nsHttpNegotiateAuth is a service and has private destructor or something). I was trying to reference nsHttpNegotiateAuth in GetNextTokenRunnable as nsCOMPtr<nsIHttpAuthenticator>. Is desirable to introduce friend class?

> @@ +321,5 @@
> > +       nsresult rv = NS_NewNamedThread("NegotiateAuth", getter_AddRefs(mNegotiateThread));
> > +       NS_ENSURE_SUCCESS(rv, rv);
> > +   }
> > +   mNegotiateThread->Dispatch(getNextTokenRunnable, NS_DISPATCH_NORMAL);
> > +   return NS_ERROR_IN_PROGRESS;
> 
> Return NS_OK here, the method made its job.  or explain the rational for
> this error be returned here.
Returning NS_ERROR_IN_PROGRESS make sense later because this return value is propagated to here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#547
I'll add comment to the sources to clarify this return value.

> 
> ::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
> @@ +744,2 @@
> >                                   &identityInvalid);
> > +    SetAuthorizationMembers(proxyAuth, continuationState);
> 
> why this change?
This is quite clumsy for me too. I don't know how to correctly set continuationState (ie. content of mProxyAuthContinuationState or mAuthContinuationState - depends if proxyAuth is set, see: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#576) since I want to use nsCOMPtr<nsISupports> instead of nsISupports** for mProxyAuthContinuationState and mAuthContinuationState.

> ::: netwerk/protocol/http/nsIHttpAuthenticator.idl
> @@ +146,5 @@
> >                                 in    wstring        aDomain,
> >                                 in    wstring        aUser,
> >                                 in    wstring        aPassword,
> >                                 inout nsISupports    aSessionState,
> > +                               in    nsISupports    aContinuationState,
> 
> please explain this change.  this more seams to me like a cleanup only.  if
> it's the case, then I'm not a big fan of it.  if you want to do this
> cleanup, please do it in a separate bug.
This is more like fixing behaviour when using nsCOMPtr<nsISupports> instead of nsISupports**. When passing nsCOMPtr by using getter_AddRefs the aContinuationState is nullptr and I cannot use it in GenerateCredentials. The aContinuationState holds pointer to nsIAuthModule obtained by previous call of nsHttpNegotiateAuth::ChallengeReceived. Is there something else beside getter_AddRefs what can I use to keep current interface?
(In reply to Jan Horak from comment #6)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > I would personally prefer that you:
> > - encapsulate the SetResult and this runnable dispatch inside the class
> Could you please explain this a little more? You mean instead of SetResult
> use later mentioned DispatchError and DispatchSuccess and doing
> NS_DispatchToMainThread(this[GetNextTokenCompleteEvent]) in these methods?

Yes.

> 
> > @@ +250,5 @@
> > > +           nsAutoCString contractId;
> > > +           contractId.Assign(NS_HTTP_AUTHENTICATOR_CONTRACTID_PREFIX);
> > > +           contractId.Append("negotiate");
> > > +           nsresult rv;
> > > +           nsCOMPtr<nsIHttpAuthenticator> authenticator = do_GetService(contractId.get(), &rv);
> > 
> > hmm.. to speed this up a bit maybe let this runnable reference
> > nsHttpNegotiateAuth at its dispatch time.
> That would require to add GetNextTokenRunnable as friend class to
> nsHttpNegotiateAuth. I've tried it before (because the nsHttpNegotiateAuth
> is a service and has private destructor or something). I was trying to
> reference nsHttpNegotiateAuth in GetNextTokenRunnable as
> nsCOMPtr<nsIHttpAuthenticator>. Is desirable to introduce friend class?

No problem, leave it as it is in this patch.

> 
> > @@ +321,5 @@
> > > +       nsresult rv = NS_NewNamedThread("NegotiateAuth", getter_AddRefs(mNegotiateThread));
> > > +       NS_ENSURE_SUCCESS(rv, rv);
> > > +   }
> > > +   mNegotiateThread->Dispatch(getNextTokenRunnable, NS_DISPATCH_NORMAL);
> > > +   return NS_ERROR_IN_PROGRESS;
> > 
> > Return NS_OK here, the method made its job.  or explain the rational for
> > this error be returned here.
> Returning NS_ERROR_IN_PROGRESS make sense later because this return value is
> propagated to here:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpChannelAuthProvider.cpp#547
> I'll add comment to the sources to clarify this return value.

Yep, just a the comment, thanks.

> 
> > 
> > ::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
> > @@ +744,2 @@
> > >                                   &identityInvalid);
> > > +    SetAuthorizationMembers(proxyAuth, continuationState);
> > 
> > why this change?
> This is quite clumsy for me too. I don't know how to correctly set
> continuationState (ie. content of mProxyAuthContinuationState or
> mAuthContinuationState - depends if proxyAuth is set, see:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpChannelAuthProvider.cpp#576) since I want to use nsCOMPtr<nsISupports>
> instead of nsISupports** for mProxyAuthContinuationState and
> mAuthContinuationState.

This needs some clarification.  Anyway, you probably want to do something that is wrong, and adding this here is with high probability wrong too.  I need some more context from you to understand the intention and reason for this change.

> 
> > ::: netwerk/protocol/http/nsIHttpAuthenticator.idl
> > @@ +146,5 @@
> > >                                 in    wstring        aDomain,
> > >                                 in    wstring        aUser,
> > >                                 in    wstring        aPassword,
> > >                                 inout nsISupports    aSessionState,
> > > +                               in    nsISupports    aContinuationState,
> > 
> > please explain this change.  this more seams to me like a cleanup only.  if
> > it's the case, then I'm not a big fan of it.  if you want to do this
> > cleanup, please do it in a separate bug.
> This is more like fixing behaviour when using nsCOMPtr<nsISupports> instead
> of nsISupports**. When passing nsCOMPtr by using getter_AddRefs the
> aContinuationState is nullptr and I cannot use it in GenerateCredentials.
> The aContinuationState holds pointer to nsIAuthModule obtained by previous
> call of nsHttpNegotiateAuth::ChallengeReceived. Is there something else
> beside getter_AddRefs what can I use to keep current interface?

You can getter_AddRef to something else than you keep your pointer at.  Or don't use it at all.  I think the lifetime control is somewhere else and your changes shouldn't touch it.

I am so far against this change, unless you show me what you do and you cannot do it a different way.
(In reply to Honza Bambas (:mayhemer) from comment #7)
> > > ::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
> > > @@ +744,2 @@
> > > >                                   &identityInvalid);
> > > > +    SetAuthorizationMembers(proxyAuth, continuationState);
> > > 
> > > why this change?
> > This is quite clumsy for me too. I don't know how to correctly set
> > continuationState (ie. content of mProxyAuthContinuationState or
> > mAuthContinuationState - depends if proxyAuth is set, see:
> > http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> > nsHttpChannelAuthProvider.cpp#576) since I want to use nsCOMPtr<nsISupports>
> > instead of nsISupports** for mProxyAuthContinuationState and
> > mAuthContinuationState.
> 
> This needs some clarification.  Anyway, you probably want to do something
> that is wrong, and adding this here is with high probability wrong too.  I
> need some more context from you to understand the intention and reason for
> this change.

Let me try to explain how continuationState work now:
* mAuthContinuationState stores pointer to nsIAuthModule (implemented by nsAuthGSSAPI, nsAuthSASL, nsAuthSSPI)
* The mAuthContinuationState is set by nsIHttpAuthenticator::ChallengeReceived [2]
* Please note that when calling [2] the continuationState is used instead of mAuthContinuationState. The continuationState is initialized by GetAuthorizationMembers [3] to share the same code when proxy is involved by utilizing mProxyAuthContinuationState member.

The SetAuthorizationMembers is doing the reverse of GetAuthorizationMembers, if it was not called, the mAuthContinuationState would stay null because only continuationState pointer is changed.


[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#478
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#707
[3] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#576

> 
> > 
> > > ::: netwerk/protocol/http/nsIHttpAuthenticator.idl
> > > @@ +146,5 @@
> > > >                                 in    wstring        aDomain,
> > > >                                 in    wstring        aUser,
> > > >                                 in    wstring        aPassword,
> > > >                                 inout nsISupports    aSessionState,
> > > > +                               in    nsISupports    aContinuationState,
> > > 
> > > please explain this change.  this more seams to me like a cleanup only.  if
> > > it's the case, then I'm not a big fan of it.  if you want to do this
> > > cleanup, please do it in a separate bug.
> > This is more like fixing behaviour when using nsCOMPtr<nsISupports> instead
> > of nsISupports**. When passing nsCOMPtr by using getter_AddRefs the
> > aContinuationState is nullptr and I cannot use it in GenerateCredentials.
> > The aContinuationState holds pointer to nsIAuthModule obtained by previous
> > call of nsHttpNegotiateAuth::ChallengeReceived. Is there something else
> > beside getter_AddRefs what can I use to keep current interface?
> 
> You can getter_AddRef to something else than you keep your pointer at.  Or
> don't use it at all.  I think the lifetime control is somewhere else and
> your changes shouldn't touch it.
> 
> I am so far against this change, unless you show me what you do and you
> cannot do it a different way.


What I'm afraid of is that when passing continuationState (ie. mAuthContinuationState or mProxyAuthContinuationState owned by nsHttpChannelAuthProvider) to GenerateCredentialsAsync and keeping/using it in worker thread by GetNextTokenRunnable::Run() would lead to crash call on worker thread in case mAuthContinuationState is freed on main thread by nsHttpChannelAuthProvider::Cancel during worker thread execution. 

What I wanted to do is to replace raw pointer mAuthContinuationState in nsHttpChannelAuthProvider.h by nsCOMPtr to automatically raise reference count when storing it in GetNextTokenRunnable.

I agree that this probably should be changed by different patch to make my fix more clear and perhaps there is more suitable approach to this change.
Will try to respond later.
Flags: needinfo?(honzab.moz)
In the mean time: The existing code is capable of multithreaded referencing of continuation states, I think (have to double check on it, tho).   NS_IF_RELEASE on a raw ptr does the same job as nullifying a comptr.

honestly, unless you are trying to fix an actual bug, I think you don't need the changes.  If it's a prerequisite and you do need, then split the patch to two please.  May be be easier to review/land.
Hm, seems that NS_IF_RELEASE is calling nsISupport::Release() so in case there is some reference left it won't delete the instance. I think we don't have to change mAuthContinuationState to nsCOMPtr in nsHttpChannelAuthProvider while we can keep nsCOMPtr in GetNextTokenRunnable. That would simplify the patch significantly.
(In reply to Jan Horak from comment #11)
> Hm, seems that NS_IF_RELEASE is calling nsISupport::Release() so in case
> there is some reference left it won't delete the instance. I think we don't
> have to change mAuthContinuationState to nsCOMPtr in
> nsHttpChannelAuthProvider while we can keep nsCOMPtr in
> GetNextTokenRunnable. That would simplify the patch significantly.

Exactly.  Please update your patch with that, I think it will be simpler to review it/give feedback.   Thanks.
Flags: needinfo?(honzab.moz)
* Removed nsCOMPtr changes in nsHttpChannelAuthProvider.cpp
* Trying to fix issues mentioned in previous feedback.
I hope the patch is more mature now. Please have a look.
Attachment #8733779 - Attachment is obsolete: true
Attachment #8736717 - Flags: review?(honzab.moz)
Comment on attachment 8736717 [details] [diff] [review]
negotiate-off-main-thread-3.patch

Review of attachment 8736717 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  We are getting closer, still few things left to polish.

Please, next time make sure ALL my comments are addressed.

::: extensions/auth/nsHttpNegotiateAuth.cpp
@@ +184,5 @@
> +// method the nsIAuthNegotiateCallback::OnCredsAvailable is called with
> +// obtained credentials and flags when successful, otherwise credentials
> +// is a null pointer and flags are set to 0.
> +//
> +class GetNextTokenCompleteEvent final: public nsCancelableRunnable

these helper classes must be in an anonymous namespace

@@ +209,5 @@
> +        MOZ_ASSERT(!NS_IsMainThread());
> +
> +        nsresult rv;
> +        { // Locking only to set members
> +            mozilla::MutexAutoLock al(mLock);

s/al/lock/

@@ +214,5 @@
> +
> +            mCreds = aCreds;
> +            mFlags = aFlags;
> +        }
> +        nsCOMPtr<nsIRunnable> runnable = do_QueryInterface(this, &rv);

you don't need to, just pass |this| to NS_DispatchToMainThread()

@@ +233,5 @@
> +             mFlags = 0;
> +        }
> +        nsCOMPtr<nsIRunnable> runnable = do_QueryInterface(this, &rv);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        return NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL);

same set of changes here

@@ +236,5 @@
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        return NS_DispatchToMainThread(runnable, NS_DISPATCH_NORMAL);
> +    }
> +
> +    NS_IMETHODIMP Run()

override

@@ +251,5 @@
> +        }
> +        return NS_OK;
> +    }
> +
> +    NS_IMETHODIMP Cancel()

override

@@ +276,5 @@
> +//
> +// This runnable is created by GenerateCredentialsAsync and it runs
> +// in nsHttpNegotiateAuth::mNegotiateThread and calling GenerateCredentials.
> +//
> +class GetNextTokenRunnable final: public nsRunnable

final : public

@@ +335,5 @@
> +                                                     mDomain.get(),
> +                                                     mUsername.get(),
> +                                                     mPassword.get(),
> +                                                     &sessionState,
> +                                                     &continuationState,

any changes in these (any pointer changes) must be passed back up to the nsHttpChannelAuthProvider, or not?  maybe it's impl specific these never change, but for completeness sake, I think it should be there.  please double check, and if this would be an overkill, write a good comment why we don't need it.

@@ +362,5 @@
> +                                         const char16_t *password,
> +                                         nsISupports **sessionState,
> +                                         nsISupports **continuationState,
> +                                         nsICancelableRunnable **aCancelable
> +                                         )

the style is to have the ) on the same as the last arg
check also indention

@@ +370,5 @@
> +
> +   RefPtr<GetNextTokenCompleteEvent> cancelEvent = new GetNextTokenCompleteEvent(aCallback);
> +
> +
> +   nsCOMPtr<nsIRunnable> getNextTokenRunnable = new GetNextTokenRunnable(authChannel,

nit: maybe break like:

nsCOMPtr<> .. =
  new GetNext..(authChannel, 
                ...);

::: extensions/auth/nsHttpNegotiateAuth.h
@@ +38,5 @@
>                            int32_t             port,
>                            const char         *baseStart,
>                            const char         *baseEnd);
> +    // Thread for GenerateCredentialsAsync
> +    nsCOMPtr<nsIThread> mNegotiateThread;

Finaly found what I wanted to suggest to use here: please change this to LazyIdleThread (https://mxr.mozilla.org/mozilla-central/search?string=LazyIdleThread)

::: netwerk/base/nsIAuthNegotiateCallback.idl
@@ +4,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(d989cb03-e446-4086-b9e6-46842cb97bd5)]
> +interface nsIAuthNegotiateCallback : nsISupports

sorry to not mention this earlier - this is a general purpose callback and the name should not contain Negotiate in it.  simple nsIHttpAuthenticatorCallback will do better.

@@ +18,5 @@
> +   *        failed
> +   * @param wasCancelled
> +   *        This value is set to true when user has cancelled obtaining credentials
> +   */
> +  void onCredsAvailable(in string aCreds, in unsigned long aFlags, in bool wasCancelled);

I'd slightly more prefer this be called |onCredsGenerated|, the current name could be confused with methods of nsIAuthPromptCallback

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +1257,5 @@
> +    if (!aCreds) {
> +        // Failed to obtain creds
> +        if (!wasCancelled && !mRemainingChallenges.IsEmpty()) {
> +            // there are still some challenges to process, do so
> +            nsresult rv;

nit: please declare rv at the top of the method.
Attachment #8736717 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #14)
> @@ +335,5 @@
> > +                                                     mDomain.get(),
> > +                                                     mUsername.get(),
> > +                                                     mPassword.get(),
> > +                                                     &sessionState,
> > +                                                     &continuationState,
> 
> any changes in these (any pointer changes) must be passed back up to the
> nsHttpChannelAuthProvider, or not?  maybe it's impl specific these never
> change, but for completeness sake, I think it should be there.  please
> double check, and if this would be an overkill, write a good comment why we
> don't need it.
Thanks for the reviewing. Yes, I also have been thinking about it. Looking at current implementation of GenerateCredentials in all nsIHttpAuthenticator subclasses the continuationState is never changed. It is only set by ChallengeReceived in case of NTLM and Negotiate. In fact I don't know the reason the GenerateCredentials was declared like that. 

To implement that we would have to pass continuationState instance and bool proxyAuth to the OnCredsGenerated and set mProxyAuthContinuationState or mAuthContinuationState according to proxyAuth value. Is it worth the effort?
Posted patch async-negotiate2 (obsolete) — Splinter Review
I hope I've fixed everything mentioned.
About passing sessionState and continuationState back to caller:
- I'm passing continuationState back to nsHttpChannelAuthProvider (despite it is not changed in any implementation of GenerateCredentials method) for the sake of completeness
- However I'm ignoring changes to sessionState, which is currently used only by nsHttpDigestAuth::GenerateCredentials which I presume does not make a sense to implement as async. I've reflected that in comments. Passing its changes to the caller (nsHttpChannelAuthProvider) would be more complicated.

Please have a look.
Attachment #8736717 - Attachment is obsolete: true
Attachment #8740915 - Flags: review?(honzab.moz)
Posted patch async-negotiate2.patch (obsolete) — Splinter Review
Fixed indentation, reattaching.
Attachment #8740915 - Attachment is obsolete: true
Attachment #8740915 - Flags: review?(honzab.moz)
Attachment #8740919 - Flags: review?(honzab.moz)
Hmm..  why exactly you need the cancelable event?  I would understand it when it would just drop the callback and bypass it's notification immediately, but you when canceled call the callback with aCreds == nullptr (-> error).  That doesn't make sense to me.  Can you explain please?
Flags: needinfo?(jhorak)
Posted patch async-negotiate3 patch (obsolete) — Splinter Review
Ouch, that is a mistake. Of course, when user cancels loading the page, the NegotiateThread should never dispatch results to the channel auth provider.
Attachment #8740919 - Attachment is obsolete: true
Attachment #8740919 - Flags: review?(honzab.moz)
Flags: needinfo?(jhorak)
Attachment #8741816 - Flags: review?(honzab.moz)
Comment on attachment 8741816 [details] [diff] [review]
async-negotiate3 patch

Review of attachment 8741816 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/auth/nsHttpNegotiateAuth.cpp
@@ +199,5 @@
> +// method the nsIHttpAuthenticatorCallback::OnCredsAvailable is called with
> +// obtained credentials and flags when successful, otherwise credentials
> +// is a null pointer and flags are set to 0.
> +//
> +class GetNextTokenCompleteEvent final : public nsCancelableRunnable

please inherit from nsRunnable and nsICancelable and let generateCredentialsAsync return nsICancelable only.

@@ +252,5 @@
> +    {
> +        // Runs on main thread
> +        MOZ_ASSERT(NS_IsMainThread());
> +
> +        mozilla::MutexAutoLock lock(mLock);

ups, how could I miss this the last time?  please don't use lock here (it's a very very bad thing to call an unknown callback code under a lock!!!)

use Atomic<bool> mCancelled instead and get rid of the lock completely, I don't see a need for it.

then, also important, swap the callback to a local refptr before calling it, like:

nsCOMPtr<nsIHttpAuthenticatorCallback> callback;
callback.swap(mCallback); <- makes mCallback null
callback->On..();

this ensures potential reentrancy (however it's impossible here) and also correctly releases the reference, which you are not doing now while there is a potential for a cycle!

::: extensions/auth/nsHttpNegotiateAuth.h
@@ +7,5 @@
>  #define nsHttpNegotiateAuth_h__
>  
>  #include "nsIHttpAuthenticator.h"
>  #include "nsIURI.h"
> +#include "nsIThread.h"

do you still need this include?

::: netwerk/base/nsIHttpAuthenticatorCallback.idl
@@ +7,5 @@
> +[scriptable, uuid(d989cb03-e446-4086-b9e6-46842cb97bd5)]
> +interface nsIHttpAuthenticatorCallback : nsISupports
> +{
> +  /**
> +   * Authentication information is available.

please note that this is used to harvest result from nsIHttpAuthenticator.generateCredentialsAsync which generates the auth header data.  one could exchange this for a callback for user credentials prompt.

@@ +14,5 @@
> +   *        Credentials which were obtained asynchonously, null when obtaining
> +   *        credentials failed
> +   * @param aFlags
> +   *        Flags set by asynchronous call. Set to 0 when obtaining credentials
> +   *        failed

remove the "Set to 0 when obtaining credentials failed" line.  I think the error is already passed as aCreds == nullptr.  OTOH, I think the result should be passed as nsresult (sorry not to mention earlier).

::: netwerk/protocol/http/nsHttpChannelAuthProvider.h
@@ +163,5 @@
>      // A variable holding the preference settings to whether to open HTTP
>      // authentication credentials dialogs for sub-resources and cross-origin
>      // sub-resources.
>      static uint32_t                   sAuthAllowPref;
> +    nsCOMPtr<nsICancelableRunnable>   mNegotiateAsyncCancelable;

don't relate this to just "Negotiate".  should be mGenerateCredentialsCancelable
Attachment #8741816 - Flags: review?(honzab.moz) → review-
Posted patch async-negotiate4.patch (obsolete) — Splinter Review
I've simplified the nsHttpChannelAuthProvider::OnCredsGenerated to call OnAuthCancelled instead of copying its content. 

Also OnCredsGenerated now contain aResult instead of wasCanceled (since wasCanceled become invalid because was always false).

Please have a look.
Attachment #8741816 - Attachment is obsolete: true
Attachment #8743821 - Flags: review?(honzab.moz)
Posted patch async-negotiate5.patch (obsolete) — Splinter Review
Sorry about previous patch, there was some warnings during compilation and also it did not fallback to other type of auth in case negotiate auth fails.
Attachment #8743821 - Attachment is obsolete: true
Attachment #8743821 - Flags: review?(honzab.moz)
Attachment #8743833 - Flags: review?(honzab.moz)
Comment on attachment 8743833 [details] [diff] [review]
async-negotiate5.patch

Review of attachment 8743833 [details] [diff] [review]:
-----------------------------------------------------------------

closing!

::: extensions/auth/nsHttpNegotiateAuth.cpp
@@ +199,5 @@
> +// This event is fired on main thread when async call of
> +// nsHttpNegotiateAuth::GenerateCredentials is finished. During the Run()
> +// method the nsIHttpAuthenticatorCallback::OnCredsAvailable is called with
> +// obtained credentials and flags when successful, otherwise credentials
> +// is a null pointer and flags are set to 0.

please update according to the new signature (nsresult).

@@ +271,5 @@
> +    nsCOMPtr<nsIHttpAuthenticatorCallback> mCallback;
> +    char *mCreds; // This class owns it, freed in destructor
> +    uint32_t mFlags;
> +    nsresult mResult;
> +    mozilla::Atomic<bool> mCancelled;

according the code it now looks like you only work with the mCanceled flag on the main thread, right?  then it probably no longer needs to be Atomic, but just a plain bool.

@@ +354,5 @@
> +                                                     mPassword.get(),
> +                                                     &sessionState,
> +                                                     &continuationState,
> +                                                     aFlags,
> +                                                     aCreds);

I think you forgetting to pass continuationState back to the member, right?

::: netwerk/base/nsIHttpAuthenticatorCallback.idl
@@ +14,5 @@
> +   *        Credentials which were obtained asynchonously.
> +   * @param aFlags
> +   *        Flags set by asynchronous call.
> +   * @param aResult
> +   *        Set to NS_ERROR_FAILURE in case obtaning credetials fails,

just say "result status of credentials generation"

@@ +22,5 @@
> +   */
> +  void onCredsGenerated(in string aCreds,
> +                        in unsigned long aFlags,
> +                        in nsresult aResult,
> +                        in nsISupports aContinuationState);

I checked all our implementations and obviously continuationState is never changed by generateCredentials.  however, in case of nsHttpDigestAuth the session state is changed.  for saneness I think we should either return back both or none.  what do you think?

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +395,5 @@
> +                                       &ss,
> +                                       &*continuationState,
> +                                       getter_AddRefs(mGenerateCredentialsCancelable));
> +
> +    if (rv != NS_ERROR_IN_PROGRESS) {

hmm... I'd still rather make GenerateCredentialsAsync return NS_OK when it successfully posted the event and here branch like this:

rv = auth->GenerateCredentialsAsync(..)
if (NS_SUCCEEDED(rv)) {
  sessionState.swap(ss);
  return NS_ERROR_IN_PROGRESS;
}

rv = auth->GenerateCredentials(..);
sessionState.swap(ss);
if (NS_FAILED(rv)) return rv;
...

@@ +1247,5 @@
>  }
>  
>  nsresult
> +nsHttpChannelAuthProvider::OnCredsGenerated(const char *aCreds,
> +                                            uint32_t aFlags,

aFlags is unused, is there a reason to pass this?

@@ +1251,5 @@
> +                                            uint32_t aFlags,
> +                                            nsresult aResult,
> +                                            nsISupports* aContinuationState)
> +{
> +    // When channel is closed, do not proceed

please assert thread (NS_IsMainThread())

@@ +1268,5 @@
> +    // nsHttpNegotiateAuth::GenerateCredentials
> +    if (mProxyAuth) {
> +        NS_IF_RELEASE(mProxyAuthContinuationState);
> +        mProxyAuthContinuationState = aContinuationState;
> +        NS_IF_ADDREF(mProxyAuthContinuationState);

you should swap the reference, like:

nsCOMPtr<nsISupports> contState(aContinuationState);
contState.swap(m(Proxy)ContState);

this will make sure your NS_IF_RELEASE doesn't release the very last reference in case aContinuationState and mAuthContinuationState are the same object.  I know that the event references it, but that is an implementation detail opaque to this method.

::: netwerk/protocol/http/nsIHttpAuthenticator.idl
@@ +87,5 @@
> +     *        state held by the channel between consecutive calls to
> +     *        generateCredentials, assuming multiple calls are required
> +     *        to authenticate.  this state is held for at most the lifetime of
> +     *        the channel.
> +     * @pram aCompleteEvent

maybe just aCancel

@@ +99,5 @@
> +                                  in    wstring        aDomain,
> +                                  in    wstring        aUser,
> +                                  in    wstring        aPassword,
> +                                  inout nsISupports    aSessionState,
> +                                  inout nsISupports    aContinuationState,

I'm a bit curious why is these are in/out when we never change them synchronously.  we only give back contstate when changed via the callback.  these should be "in" only.
Attachment #8743833 - Flags: review?(honzab.moz) → feedback+
Posted patch async-negotiate6.patch (obsolete) — Splinter Review
Sorry for the delay, here's the updated patch. About:
> aFlags is unused, is there a reason to pass this?
I've added flags for the sake of completeness in case some other implementation of GenerateCredentialsAsync later actually set them.
Attachment #8743833 - Attachment is obsolete: true
Attachment #8750709 - Flags: review?(honzab.moz)
Thanks for working on this. Are you able to test this with Negotiate exchanges that require multiple round-trips?

Remember, Kerberos is a boring special-case here because it usually requires only *one* call to gss_init_sec_context(), which provides a token that grants you access immediately.

Other mechanisms like GSS-NTLMSSP actually require a *conversation*, with multiple 401 responses giving new tokens from the server, each of which have to be processed in the *same* GSSAPI context as the first. Even when programmers remember this, it still warrants specific testing.

I can do this testing... if someone can babysit me through making the patch build. I made it apply to 46.0.1 since I'm already building that locally anyway, but the headers don't seem to automatically get rebuilt e.g. from the new nsIHttpAuthenticatorCallback.idl.
Unfortunately I don't have environment with GSS-NTLMSSP available. Are you able to provide me this environment or how to setup some? The attached patch can be applied to the mozilla-central repository. There are some instructions how to build from mozilla-central: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
Comment on attachment 8750709 [details] [diff] [review]
async-negotiate6.patch

Review of attachment 8750709 [details] [diff] [review]:
-----------------------------------------------------------------

Getting better!  Still few nits to update, sorry.

::: extensions/auth/nsHttpNegotiateAuth.cpp
@@ +200,5 @@
> +// method the nsIHttpAuthenticatorCallback::OnCredsAvailable is called with
> +// obtained credentials, flags and NS_OK when successful, otherwise
> +// NS_ERROR_FAILURE is returned as a result of failed operation.
> +//
> +class GetNextTokenCompleteEvent final : public nsIRunnable,

it was OK to have this derived from (now) mozilla::Runnable, but this works too
you probably just needed NS_IMPL_ISUPPORTS_INHERITED below?

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +404,5 @@
> +                                       ss,
> +                                       *continuationState,
> +                                       getter_AddRefs(mGenerateCredentialsCancelable));
> +    if (NS_SUCCEEDED(rv)) {
> +        sessionState.swap(ss);

I realized you don't need to do this, the ss pointer didn't have a chance to change actually

@@ +1308,5 @@
> +        contState.swap(mAuthContinuationState);
> +        NS_IF_ADDREF(mAuthContinuationState);
> +    }
> +    ContinueOnAuthAvailable(nsDependentCString(aCreds));
> +    return NS_OK;

ups... I missed another thing.  apparently, you must also update the auth cache entry with the result and the session state that might have changed.

the best way IMO would be to carry the second part (after call to auth->GenerateCredentials()) of nsHttpChannelAuthProvider::GenCredsAndSetEntry to this callback and when we generate creds synchronously, just call it directly from that method (if convenient - I'll let you to decide).

And the callback itself would be nicer right after GenCredsAndSetEntry().

I presume authFlags == aFlags, but you will also need the (scheme, host, port, directory, realm) tuple.  I think mProxyAuth and GetAuthorizationMembers() is your friend.  And you can probably also clean the contstate assignment a bit since it returns the reference to the correct proxy/www cont sate member for you.
Attachment #8750709 - Flags: review?(honzab.moz) → review-
Setting up a test server using GSS-NTLMSSP is somewhat non-trivial. If you have an Active Directory environment, however, then testing the *client* is simple enough. Get winbind to join the domain, authenticate ('wbinfo -K $ADUSER' or just log in using pam_winbind if that's how you do it), have gssntlmssp installed correctly (the Fedora packages Just Work™).

Then 'kdestroy' to delete your Kerberos TGT, but winbind will still have your NTLM credentials. Test by authenticating to some web service: 
 curl -v  --negotiate -u : https://it.intel.com/
Watch the multiple round-trips, and the distinctive NTLMSSP-in-SPNEGO contents of the Negotiate traffic, which looks different to Kerberos:

> GET / HTTP/1.1
> Authorization: Negotiate YEAGBisGAQUFAqA2MDSgDjAMBgorBgEEAYI3AgIKoiIEIE5UTE1TU1AAAQAAABeCCKAAAAAAAAAAAAAAAAAAAAAA

< HTTP/1.1 401 Unauthorized
< WWW-Authenticate: Negotiate oYIBFzCCAROgAwoBAaEMBgorBgEEAYI3AgIKooH9BIH6TlRMTVNTUAACAAAABgAGADgAAAAVgomiXF62o5FNl8gAAAAAAAAAALwAvAA+AAAABgGxHQAAAA9BAE0AUgACAAYAQQBNAFIAAQAWAFYATQBTAFAAVwBBAEIAUwAwADEAMQAEACQAYQBtAHIALgBjAG8AcgBwAC4AaQBuAHQAZQBsAC4AYwBvAG0AAwA8AFYATQBTAFAAVwBBAEIAUwAwADEAMQAuAGEAbQByAC4AYwBvAHIAcAAuAGkAbgB0AGUAbAAuAGMAbwBtAAUAHABjAG8AcgBwAC4AaQBuAHQAZQBsAC4AYwBvAG0ABwAIAIUYEKu9q9EBAAAAAA==


> GET / HTTP/1.1
> Authorization: Negotiate oYGwMIGtoAMKAQGigaUEgaJOVExNU1NQAAMAAAAYABgAQAAAABgAGABYAAAABgAGAHAAAAAQABAAdgAAABwAHACGAAAAAAAAAKIAAAAVgomiXrioGUYy0hcAAAAAAAAAAAAAAAAAAAAAtjVaVFe07eFkJKZ8WRBe4movae419XqWRwBFAFIAZAB3AG8AbwBkAGgAbwB1AEQAVwBPAE8ARABIAE8AVQAtAEwASQBOAFUAWAA=

< HTTP/1.1 200 OK


But yes, the patch from comment 25 does appear to be working here, having installed Live HTTP Headers to confirm it really is using NTLMSSP within Negotiate and watched winbind logs to confirm it's performing authentication for me. (But hey, it wasn't going to be falling back to 'WWW-Authenticate: NTLM', was it? Bug 634334 is still open because that's broken, and has been for at least 5 years...☹)
Posted patch async-negotiate7.patch (obsolete) — Splinter Review
> it was OK to have this derived from (now) mozilla::Runnable, but this works too
> you probably just needed NS_IMPL_ISUPPORTS_INHERITED below?

I've hit some test fails when using mozilla::Runnable (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=8184a69a2764&selectedJob=20565109 ).

Isn't:
NS_IMPL_ISUPPORTS(GetNextTokenCompleteEvent, nsIRunnable, nsICancelable)
sufficient?

About storing to cache, I've decided to write nsHttpChannelAuthProvider::UpdateCache because I don't want to call ContinueOnAuthAvailable (called from OnCredsGenerated) when generating credentials synchronously.

I'll try to test with GSS-NTLMSSP when I will have the environment set. Meanwhile please have a look.
Attachment #8750709 - Attachment is obsolete: true
Attachment #8751768 - Flags: review?(honzab.moz)
(In reply to David Woodhouse from comment #30)
> But yes, the patch from comment 25 does appear to be working here, having
> installed Live HTTP Headers to confirm it really is using NTLMSSP within
> Negotiate and watched winbind logs to confirm it's performing authentication
> for me. (But hey, it wasn't going to be falling back to 'WWW-Authenticate:
> NTLM', was it? Bug 634334 is still open because that's broken, and has been
> for at least 5 years...☹)

Thanks for looking into it. Yes, the patch from bug 634334 is required at least in Fedora/Linux afaik. I've asked my colleagues to setup an environment with NTLMSSP but feel free to test again.
Simo can almost certainly help you set up something which uses $NTLM_USER_FILE on both client and server side to use NTLMSSP without actually having Windows anywhere in sight; just a client and trivial server on the same box.
(In reply to Jan Horak from comment #31)
> Created attachment 8751768 [details] [diff] [review]
> async-negotiate7.patch
> 
> > it was OK to have this derived from (now) mozilla::Runnable, but this works too
> > you probably just needed NS_IMPL_ISUPPORTS_INHERITED below?
> 
> I've hit some test fails when using mozilla::Runnable (see
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8184a69a2764&selectedJob=20565109 ).

mozilla::Runnable already implements the refcounting for you, hence the class already has mRefCnt member.  Declaring [1] is simply not needed when you derive from mozilla::Runnable.  Then you just need NS_IMPL_ISUPPORTS_INHERITED(yourClass, mozilla::Runnable, nsICancelable)

This saves some footprint by sharing the implementation.  But reimplementation as you do is OK too.  Up to you to chose which way you do it.

[1] https://hg.mozilla.org/try/diff/8184a69a2764/extensions/auth/nsHttpNegotiateAuth.cpp#l1.69
Comment on attachment 8751768 [details] [diff] [review]
async-negotiate7.patch

Review of attachment 8751768 [details] [diff] [review]:
-----------------------------------------------------------------

Few last nits and I think we are OK to go.  Feel free to land or set the checkin-needed keyword.  Thanks!

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +362,5 @@
> +nsresult
> +nsHttpChannelAuthProvider::UpdateCache(const char* aCreds,
> +                                       const char* aChallenge,
> +                                       uint32_t aGenerateFlags,
> +                                       nsISupports* aSessionState)

just a nit: it's better to put the method under GenCredsAndSetEntry where from you actually move it away from.  it will make the patch simpler to overlook and the code easier to read.

also, I'd a bit more expect this to have signature similar to GenCredsAndSetEntry, and you simply to pass all the args.  Your patch makes GetAuthorizationMembers be called twice for the (much more frequent) sync case, which copies some strings twice and is hence less efficient.

rather call GetAuthorizationMembers before you call UpdateCache from OnCredsGenerated.

@@ +365,5 @@
> +                                       uint32_t aGenerateFlags,
> +                                       nsISupports* aSessionState)
> +{
> +    nsAutoCString unused;
> +    nsresult rv;

nit: declare as first variable, make a blank like after it.

@@ +449,5 @@
>      } else {
>          continuationState = &mAuthContinuationState;
>      }
>  
>      uint32_t generateFlags;

nit: declare before first use

@@ -392,5 @@
>                                     &ss,
>                                     &*continuationState,
>                                     &generateFlags,
>                                     result);
> -

nit: leave this blank like please

@@ +1305,5 @@
>  }
>  
>  nsresult
> +nsHttpChannelAuthProvider::OnCredsGenerated(const char *aCreds,
> +                                            uint32_t aFlags,

nit: maybe call this arg aGenerateFlags, so that it's in sync with the GenCredsAndSetEntry local vars naming

@@ +1322,5 @@
> +
> +    if (NS_FAILED(aResult)) {
> +        return OnAuthCancelled(nullptr, true);
> +    }
> +    // We want to update m(Proxy)AuthContinuationState in case it was changed by

nit: blank line before this comment line please (the NS_FAILED block is then more visible)
Attachment #8751768 - Flags: review?(honzab.moz) → review+
Posted patch async-negotiate8.patch (obsolete) — Splinter Review
Fixed nits, copying review+ from previous comment. Thanks for your help!
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a5a65aa9ea5
Attachment #8751768 - Attachment is obsolete: true
Attachment #8754813 - Flags: review+
Attachment #8754813 - Flags: checkin?
backed this out since this or the other changeset caused https://treeherder.mozilla.org/logviewer.html#?job_id=28427487&repo=mozilla-inbound
Flags: needinfo?(jhorak)
(In reply to Carsten Book [:Tomcat] from comment #39)
> backed this out since this or the other changeset caused
> https://treeherder.mozilla.org/logviewer.html#?job_id=28427487&repo=mozilla-
> inbound
Hm, errors seems to be unrelated to my changes. Could you please confront with my try run? https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a5a65aa9ea5
Flags: needinfo?(jhorak)
Attachment #8754813 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/1fe1c3b03d08
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1277213
This patch needs to back out.  Can please someone do that or should I do it manually?
Status: RESOLVED → REOPENED
Flags: needinfo?(cbook)
Resolution: FIXED → ---
(In reply to Honza Bambas (:mayhemer) from comment #43)
> This patch needs to back out.  Can please someone do that or should I do it
> manually?

i will do this (filed the causing one anyway ;)
Flags: needinfo?(cbook)
(In reply to Carsten Book [:Tomcat] from comment #45)
> backed out in  https://hg.mozilla.org/mozilla-central/rev/a39da695528a

Thanks!
No longer depends on: 1277213
You never tested your patches on windows, right?  So I'm doing it now (my mistake I didn't before)

I'm getting an assertion failure:

>	xul.dll!nsAuthSSPI::AddRef() Line 181	C++
 	xul.dll!nsCOMPtr_base::assign_with_AddRef(0x0989a8d0) Line 52	C++
 	xul.dll!nsCOMPtr<nsISupports>::operator=(0x0989a8d0) Line 901	C++
 	xul.dll!`anonymous namespace'::GetNextTokenRunnable::ObtainCredentialsAndFlags(0x197ffd50, 0x197ffd54) Line 363	C++
 	xul.dll!`anonymous namespace'::GetNextTokenRunnable::Run() Line 322	C++
 	xul.dll!nsThread::ProcessNextEvent(false, 0x197ffe05) Line 1073	C++
 	xul.dll!NS_ProcessNextEvent(0x1b7aa8e0, false) Line 290	C++


on the NegotiateAuth thread, simply when trying to talk to a Negotiate authenticated page (set as trusted URI in network.negotiate-auth.trusted-uris pref).

The offensive line is:

  mContinuationState = continuationState;

The problem is that nsAuthSSPI is instantiated on the main thread, but you are trying to release/addref it on the background thread.  That is not allowed, unless the refcounter is made thread-safe and the whole class can be actually used in a multithread env.  Here it mainly means to not reuse it on more then one thread and not initiate and then destroy on two different threads.

Other problem I found is that you successfully leak nsAuthSSPI (cont state) when it fails to generate a response (when module->GetNextToken fails, later leading to mCompleteEvent->DispatchError()).

Also, GetNextTokenCompleteEvent::DispatchSuccess should accept already_AddRefed for session and cont state and caller should pass results of forget() on the smart ptrs.

I will take the liberty and provide an update to the patch here to fix these issues, or at least point directions.
Status: REOPENED → ASSIGNED
Flags: needinfo?(jhorak)
Whiteboard: [bugday-20131016][necko-backlog][ntlm] → [bugday-20131016][necko-active][ntlm]
Posted patch update v1 (-w) (obsolete) — Splinter Review
- DispatchSuccess/DispatchError now both work with already_AddRefed (not changing reference count when on the background thread)
- DispatchError also passes back both objects just to make it be released on the main thread
- The leak was caused by redundant NS_IF_ADDREF (my bad advice!) in nsHttpChannelAuthProvider::OnCredsGenerated ; the addref actually happens when the local nsCOMPtr<> is assigned
- some general/indention cleanup (hence the -w patch)

I'm leaving refcounter on nsAuthSSPI non-threadsafe to make sure the object is correctly destroyed on the main thread (where it has been Init()'ed).  The code of nsHttpNegotiateAuth ensures that sspi API is not used concurrently.  We synchronize access to the handles and contexts always only on one thread at a time.
Attachment #8765871 - Flags: review?(jhorak)
Comment on attachment 8765871 [details] [diff] [review]
update v1 (-w)

Review of attachment 8765871 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me and also works fine when testing on Linux machine. Thanks a lot looking into it on Windows!
Attachment #8765871 - Flags: review?(jhorak) → review+
(In reply to Jan Horak from comment #50)
> Comment on attachment 8765871 [details] [diff] [review]
> update v1 (-w)
> 
> Review of attachment 8765871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me and also works fine when testing on Linux machine. Thanks a
> lot looking into it on Windows!

OK, then please provide a folded (v8 + update v1) patch for landing, you have my r+ for it ;)
Attaching folded patch, copying review from previous comment.
Attachment #8754813 - Attachment is obsolete: true
Attachment #8765871 - Attachment is obsolete: true
Attachment #8765872 - Attachment is obsolete: true
Attachment #8770902 - Flags: review+
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=104a6273cdde
I guess I can set checkin-needed.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/344f289cc64d
Move Negotiate auth off main thread. r=mayhemer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/344f289cc64d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1014061
See Also: → 1394752
Duplicate of this bug: 791008
You need to log in before you can comment on or make changes to this bug.