Bug 107702 - Recursion handling cancelled authentication challenges in NetworkProcess
Summary: Recursion handling cancelled authentication challenges in NetworkProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-01-23 10:18 PST by Brady Eidson
Modified: 2013-01-23 12:23 PST (History)
2 users (show)

See Also:


Attachments
Patch v1 (9.37 KB, patch)
2013-01-23 10:24 PST, Brady Eidson
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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