Bug 107974

Summary: HTTP Authentication should be directly between the NetworkProcess and the UIProcess
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, buildbot, rniwa
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1 ap: review+, buildbot: commit-queue-

Description Brady Eidson 2013-01-25 12:06:16 PST
HTTP Authentication should be directly between the NetworkProcess and the UIProcess

In radar as <rdar://problem/13074829>
Comment 1 Brady Eidson 2013-01-25 12:07:46 PST
*** Bug 107758 has been marked as a duplicate of this bug. ***
Comment 2 Brady Eidson 2013-01-25 12:15:08 PST
Created attachment 184793 [details]
Patch v1
Comment 3 Build Bot 2013-01-25 12:55:21 PST
Comment on attachment 184793 [details]
Patch v1

Attachment 184793 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16116508
Comment 4 Build Bot 2013-01-25 13:00:13 PST
Comment on attachment 184793 [details]
Patch v1

Attachment 184793 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16122478
Comment 5 Alexey Proskuryakov 2013-01-25 13:36:39 PST
Comment on attachment 184793 [details]
Patch v1

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

We can m_identifier from AuthenticationChallengeBase now. Will you do that in a follow-up patch, or should I?

> Source/WebKit2/ChangeLog:29
> +        Include the WebPage and WebFrame ID for the originator of this request in case it results in a challenge:

These IDs are generated by UI process, correct?

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:235
> +    uint64_t webPageID = loadParameters().webPageID();
> +    uint64_t webFrameID = loadParameters().webFrameID();

We no longer copy the whole IPC parameters structure into SchedulableLoader, this is why the build failed.

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:71
> +uint64_t AuthenticationManager::mapChallengeToIdentifier(const WebCore::AuthenticationChallenge& authenticationChallenge)

Perhaps "establishIdentifierForChallenge"?

> Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:73
> +    uint64_t challengeID = generateAuthenticationChallengeID();

That function is not thread safe. Switch to atomicIncrement?

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:83
> +    DEFINE_STATIC_LOCAL(WebProcessProxy::WebPageProxyMap, pageMap, ());
> +    return pageMap;

Maybe ASSERT(isMainThread())?

> Source/WebKit2/UIProcess/WebProcessProxy.h:82
> -    WebPageProxy* webPage(uint64_t pageID) const;
> +    static WebPageProxy* webPage(uint64_t pageID);

I don't see any issue with this, but suggest double-checking with Sam or Anders.

> Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:231
> +    NetworkResourceLoadParameters loadParameters(resourceLoadIdentifier, 0, 0, request, ResourceLoadPriorityHighest, SniffContent, storedCredentials, context->storageSession().isPrivateBrowsingSession());

This is worth a ChangeLog comment. Implementing authentication for sync requests needs to start at ResourceHandleMac level, but this wasn't immediately obvious to me.
Comment 6 Brady Eidson 2013-01-25 14:49:37 PST
The build failures are do to a refactoring patch landed in the interim, and will be easy to clear up.
Comment 7 Brady Eidson 2013-01-25 15:00:53 PST
(In reply to comment #5)
> (From update of attachment 184793 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184793&action=review
> 
> We can m_identifier from AuthenticationChallengeBase now. Will you do that in a follow-up patch, or should I?
> 
> > Source/WebKit2/ChangeLog:29
> > +        Include the WebPage and WebFrame ID for the originator of this request in case it results in a challenge:
> 
> These IDs are generated by UI process, correct?

The WebPage ID is generated by the UI process.
The WebFrame ID is generated by the WebProcess, and they are unique per WebProcess.

That is why it is necessary to shuttle both around, because once we get in to the UI Process we need to find the appropriate page based on the UI-unique page ID, then find the appropriate frame based on the WebProcess-unique frame ID.

> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:235
> > +    uint64_t webPageID = loadParameters().webPageID();
> > +    uint64_t webFrameID = loadParameters().webFrameID();
> 
> We no longer copy the whole IPC parameters structure into SchedulableLoader, this is why the build failed.

Yup.  I will adapt.
 
> > Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:71
> > +uint64_t AuthenticationManager::mapChallengeToIdentifier(const WebCore::AuthenticationChallenge& authenticationChallenge)
> 
> Perhaps "establishIdentifierForChallenge"?

I like it.

> > Source/WebKit2/Shared/Authentication/AuthenticationManager.cpp:73
> > +    uint64_t challengeID = generateAuthenticationChallengeID();
> 
> That function is not thread safe. Switch to atomicIncrement?

Nice catch.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:83
> > +    DEFINE_STATIC_LOCAL(WebProcessProxy::WebPageProxyMap, pageMap, ());
> > +    return pageMap;
> 
> Maybe ASSERT(isMainThread())?

Sure!

> > Source/WebKit2/UIProcess/WebProcessProxy.h:82
> > -    WebPageProxy* webPage(uint64_t pageID) const;
> > +    static WebPageProxy* webPage(uint64_t pageID);
> 
> I don't see any issue with this, but suggest double-checking with Sam or Anders.

I already consulted Anders and he gave me the go ahead.

> > Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:231
> > +    NetworkResourceLoadParameters loadParameters(resourceLoadIdentifier, 0, 0, request, ResourceLoadPriorityHighest, SniffContent, storedCredentials, context->storageSession().isPrivateBrowsingSession());
> 
> This is worth a ChangeLog comment. Implementing authentication for sync requests needs to start at ResourceHandleMac level, but this wasn't immediately obvious to me.

Okay!
Comment 8 Brady Eidson 2013-01-25 16:05:35 PST
http://trac.webkit.org/changeset/140874