Bug 147342 – Clean up usesAsyncCallbacks handling in ResourceHandle

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Created attachment 257648 [details]
proposed patch

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Alexey Proskuryakov]

Created attachment 257666 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 257666 [details]
patch for landing

Clearing flags on attachment: 257666

Committed r187533: <http://trac.webkit.org/changeset/187533>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.