Bug 107702 – Recursion handling cancelled authentication challenges in NetworkProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Recursion handling cancelled authentication challenges in NetworkProcess.

A whole lot of WebKit::NetworkResourceLoader::receivedAuthenticationCancellation, which calls back in to itself via the WebCore authentication client.

Even worse, this uncovered the fact that the WebProcess resource loader never correctly gets told of the cancellation.

The fix will also include small refactoring to more intelligently clean up all resources for a loader anytime it completes, no matter what the nature of the completion is.

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

[Brady Eidson]

Created attachment 184257 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 184257 [details]
Patch v1

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

r=me, because this fixes a bug and is generally an improvement. I suggest addressing the below comments though.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:141
> +        m_handle->cancel();

I don't know if we want to call this from resourceHandleStopped(). I'm pretty sure that this is super confusing, but I don't know if it's necessary given how the code is structured now.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:294
> +    send(Messages::WebResourceLoader::Cancel());

Can't we just send a didFail with a cancellation error? Having a cancel message from NetworkProcess to WebProcess makes one wonder who's in charge (it's normally WebKit client code that's in charge of cancellation, not NetworkProcess).

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:120
> +    bool m_inProgress;

Can this be replaced with checks for m_handle being non-null?

> Source/WebKit2/WebProcess/Network/WebResourceLoader.messages.in:25
> +    Cancel()

As alluded to above, I'd have no idea what this message means from looking at the IDL.

[Brady Eidson]

(In reply to comment #2)
> (From update of attachment 184257 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184257&action=review
> 
> r=me, because this fixes a bug and is generally an improvement. I suggest addressing the below comments though.
> 
> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:141
> > +        m_handle->cancel();
> 
> I don't know if we want to call this from resourceHandleStopped(). I'm pretty sure that this is super confusing, but I don't know if it's necessary given how the code is structured now.

I think the cancel can be performed from the authentication-specific method, and resourceHandleStopped can just destroy the handle itself.

> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:294
> > +    send(Messages::WebResourceLoader::Cancel());
> 
> Can't we just send a didFail with a cancellation error? Having a cancel message from NetworkProcess to WebProcess makes one wonder who's in charge (it's normally WebKit client code that's in charge of cancellation, not NetworkProcess).

This is a structural problem with the authentication APIs we build on and how authentication works in WebCore and the WebProcess... this isn't actually a "did fail with an error" message.  It is a command to actually perform a cancel, which is to replicate current WebProcess behavior.

> > Source/WebKit2/WebProcess/Network/WebResourceLoader.messages.in:25
> > +    Cancel()
> 
> As alluded to above, I'd have no idea what this message means from looking at the IDL.

I agree the naming is confusing and I'll make that better.

> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.h:120
> > +    bool m_inProgress;
> 
> Can this be replaced with checks for m_handle being non-null?

Yes.

[Brady Eidson]

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