Bug 102592

Summary: NetworkProcess Authentication
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, japhet, levin+threading, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Bug Depends on: 102593    
Bug Blocks: 98537    
Attachments:
Description Flags
Patch v1 - Take care of basic authentication ap: review+, buildbot: commit-queue-

Description Brady Eidson 2012-11-17 11:37:24 PST
Get basic authentication working in the NetworkProcess
Comment 1 Brady Eidson 2012-11-17 17:58:17 PST
Created attachment 174836 [details]
Patch v1 - Take care of basic authentication

FIXMEs mark some more advanced outliers like dealing with downloads and handling client certs.

But this gets basic "enter username/password for a subresource" authentication working.
Comment 2 Build Bot 2012-11-17 20:37:47 PST
Comment on attachment 174836 [details]
Patch v1 - Take care of basic authentication

Attachment 174836 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14875522

New failing tests:
media/media-continues-playing-after-replace-source.html
inspector-protocol/nmi-webaudio.html
Comment 3 Brady Eidson 2012-11-17 22:09:17 PST
(In reply to comment #2)
> (From update of attachment 174836 [details])
> Attachment 174836 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/14875522
> 
> New failing tests:
> media/media-continues-playing-after-replace-source.html
> inspector-protocol/nmi-webaudio.html

Can't be related to this patch.  The only changes (besides a *relaxed* ASSERT) are in NetworkProcess code.
Comment 4 Maciej Stachowiak 2012-11-17 23:38:30 PST
Comment on attachment 174836 [details]
Patch v1 - Take care of basic authentication

View in context: https://bugs.webkit.org/attachment.cgi?id=174836&action=review

> Source/WebKit2/ChangeLog:8
> +        This get's basic HTTP authentication working with the WebProcess by dispatching authentication

Did you mean "with the NetworkProcess"?
Comment 5 Brady Eidson 2012-11-18 10:11:53 PST
(In reply to comment #4)
> (From update of attachment 174836 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174836&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        This get's basic HTTP authentication working with the WebProcess by dispatching authentication
> 
> Did you mean "with the NetworkProcess"?

I did mean that, yes.  Will fix.
Comment 6 Alexey Proskuryakov 2012-11-19 10:26:23 PST
Comment on attachment 174836 [details]
Patch v1 - Take care of basic authentication

View in context: https://bugs.webkit.org/attachment.cgi?id=174836&action=review

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:317
> +    return OSAtomicIncrement64Barrier(&uniqueCanAuthenticateAgainstProtectionSpaceID);

Why not use WTF::atomicIncrement?

> Source/WebKit2/WebProcess/Network/WebResourceLoader.h:94
>      RefPtr<WebCore::ResourceLoader> m_coreLoader;
> +    WebCore::AuthenticationChallenge m_currentAuthenticationChallenge;

What is the reason to not use a pointer? Not all requests even have authentication.
Comment 7 Brady Eidson 2012-11-19 11:08:27 PST
(In reply to comment #6)
> (From update of attachment 174836 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174836&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:317
> > +    return OSAtomicIncrement64Barrier(&uniqueCanAuthenticateAgainstProtectionSpaceID);
> 
> Why not use WTF::atomicIncrement?

Great question.  I didn't know about it until now.

That said, there's no 64 bit specialization of it yet, and the Darwin specific version is already scattered about.

I'll replace them all in a single followup.

> > Source/WebKit2/WebProcess/Network/WebResourceLoader.h:94
> >      RefPtr<WebCore::ResourceLoader> m_coreLoader;
> > +    WebCore::AuthenticationChallenge m_currentAuthenticationChallenge;
> 
> What is the reason to not use a pointer? Not all requests even have authentication.

This makes sense to do.  Will change.
Comment 8 Brady Eidson 2012-11-19 11:23:32 PST
http://trac.webkit.org/changeset/135179
Comment 9 Alexey Proskuryakov 2012-11-19 12:39:24 PST
Comment on attachment 174836 [details]
Patch v1 - Take care of basic authentication

View in context: https://bugs.webkit.org/attachment.cgi?id=174836&action=review

> Source/WebKit2/WebProcess/Network/WebResourceLoader.h:34
> +#include <WebCore/AuthenticationChallenge.h>

I think that you could get rid of this include now.
Comment 10 Brady Eidson 2012-11-19 15:15:40 PST
http://trac.webkit.org/changeset/135198 as that followup