HTTP Authentication should be directly between the NetworkProcess and the UIProcess

[Brady Eidson]

HTTP Authentication should be directly between the NetworkProcess and the UIProcess

In radar as <rdar://problem/13074829>

[Brady Eidson]

*** Bug 107758 has been marked as a duplicate of this bug. ***

[Brady Eidson]

Created attachment 184793 [details]
Patch v1

[Build Bot]

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

[Build Bot]

Comment on attachment 184793 [details]
Patch v1

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

[Alexey Proskuryakov]

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.

[Brady Eidson]

The build failures are do to a refactoring patch landed in the interim, and will be easy to clear up.

[Brady Eidson]

(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!

[Brady Eidson]

http://trac.webkit.org/changeset/140874