Bug 107702

Summary: Recursion handling cancelled authentication challenges in NetworkProcess
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: All   
Attachments:
Description Flags
Patch v1 ap: review+

Description Brady Eidson 2013-01-23 10:18:31 PST
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>
Comment 1 Brady Eidson 2013-01-23 10:24:04 PST
Created attachment 184257 [details]
Patch v1
Comment 2 Alexey Proskuryakov 2013-01-23 10:36:49 PST
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.
Comment 3 Brady Eidson 2013-01-23 10:41:19 PST
(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.
Comment 4 Brady Eidson 2013-01-23 12:23:35 PST
http://trac.webkit.org/changeset/140572