Bug 147342

Summary: Clean up usesAsyncCallbacks handling in ResourceHandle
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Page LoadingAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, commit-queue, darin, japhet
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch
darin: review+, darin: commit-queue-
patch for landing none

Description Alexey Proskuryakov 2015-07-27 16:45:26 PDT
ResourceHandle always asks the client whether it uses async callbacks, and it has special cases for when there is no client. But we start with a non-null client and zero it out, after which it's not possible to guess what the behavior was.

The client behavior determines how we set up an NSURLConnection.

In particular, this means that we could respond to NSURLConnection delegates incorrectly at the time when a load is being shut down. At the worst (theoretical) case, we'd even leak delegate threads.
Comment 1 Alexey Proskuryakov 2015-07-28 08:52:36 PDT
Created attachment 257648 [details]
proposed patch
Comment 2 WebKit Commit Bot 2015-07-28 10:28:41 PDT
Attachment 257648 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:422:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/network/mac/ResourceHandleMac.mm:633:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2015-07-28 11:45:15 PDT
Comment on attachment 257648 [details]
proposed patch

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

> Source/WebCore/platform/network/ResourceHandle.h:255
> +#if PLATFORM(COCOA)
> +    bool usesAsyncCallbacks() const;
> +#endif

Since the BlobResourceHandle code is not inside PLATFORM(COCOA), this needs to be available cross-platform. I think that means that we need to put considerably more of this code outside conditionals.

> Source/WebCore/platform/network/ResourceHandleInternal.h:101
> +            , m_usesAsyncCallbacks(client && client->usesAsyncCallbacks())

It’s a little peculiar to use a client callback to supply a flag that is read one time when initializing. We might want to change the way this works to use an argument passed when constructing a resource handle instead of a callback to the client, if we can do that in a clean enough style.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:429
> +        return false; // Ignored by caller.
> +    } else
> +        return client && client->canAuthenticateAgainstProtectionSpace(this, protectionSpace);

Normally we don’t use an else after return.

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:640
>      } else
> -        return client() && client()->canAuthenticateAgainstProtectionSpace(this, protectionSpace);
> +        return client && client->canAuthenticateAgainstProtectionSpace(this, protectionSpace);

Normally we don’t do an else after return.
Comment 4 Alexey Proskuryakov 2015-07-28 11:55:31 PDT
Created attachment 257666 [details]
patch for landing
Comment 5 WebKit Commit Bot 2015-07-28 18:28:16 PDT
Comment on attachment 257666 [details]
patch for landing

Clearing flags on attachment: 257666

Committed r187533: <http://trac.webkit.org/changeset/187533>
Comment 6 WebKit Commit Bot 2015-07-28 18:28:20 PDT
All reviewed patches have been landed.  Closing bug.