Bug 156639 – [WK2][iOS] Only adjust network responses' MIME type for QuickLook in the context of a main resource load

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Do not bother with QuickLook for non-main resource loads in the NetworkProcess.

[Chris Dumez]

Created attachment 276521 [details]
WIP Patch

[Chris Dumez]

Created attachment 276560 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/25765848>

[Darin Adler]

Comment on attachment 276560 [details]
Patch

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

> Source/WebCore/platform/network/ios/QuickLook.h:81
> +    static std::unique_ptr<QuickLookHandle> createIfNecessary(ResourceHandle*, NSURLConnection *, NSURLResponse *, id delegate);

Should be ResourceHandle& if you’re going to change all the call sites anyway.

> Source/WebCore/platform/network/ios/QuickLook.h:83
> +    static std::unique_ptr<QuickLookHandle> createIfNecessary(ResourceHandle*, SynchronousResourceHandleCFURLConnectionDelegate*, CFURLResponseRef);

Ditto.

> Source/WebCore/platform/network/ios/QuickLook.mm:113
> +// We must ensure that the MIME type is correct, so that QuickLook's web plugin is called when needed.
> +// We filter the basic MIME types so that we don't do unnecessary work in standard browsing situations.
> +static void adjustMIMETypeForQuickLook(CFURLResponseRef cfResponse)

All the places that use this function call shouldCreateForMIMEType just after calling it, although they differ in how they get the MIME type. But this function knows exactly how to get the MIME type. Can we combine this adjustment and checking into a single function that’s more helpful for each of these call sites? Not sure exactly what to name it, but it would both adjust the MIME type and check if it’s eligible for quick look.

> Source/WebCore/platform/network/ios/QuickLook.mm:135
> +    if (!mimeType || CFStringCompare(mimeType.get(), quickLookMIMEType.get(), kCFCompareCaseInsensitive) != kCFCompareEqualTo)

It’s a shame that the code does Unicode case folding here when we just want ASCII case folding. In code inside WebKit we fixed that, but I doubt there is a CFString function that compares for equality ignoring ASCII case.

> Source/WebCore/platform/network/ios/QuickLook.mm:439
> +std::unique_ptr<QuickLookHandle> QuickLookHandle::createIfNecessary(ResourceHandle* handle, NSURLConnection *connection, NSURLResponse *nsResponse, id delegate)

Why not just call the response "response"? I don’t think we need to name it nsResponse.

> Source/WebCore/platform/network/ios/QuickLook.mm:442
> +    bool isMainResourceLoad = handle->firstRequest().requester() != ResourceRequest::Requester::Main;

is main <- (requester != main)

Is that right? I think it’s backwards! How did you test?

> Source/WebCore/platform/network/ios/QuickLook.mm:447
> +    adjustMIMETypeForQuickLook(nsResponse._CFURLResponse);
> +    if (!shouldCreateForMIMEType(nsResponse.MIMEType))

This relies on the behavior where adjusting the MIME type on the underlying CFURLResponse will change the result of the MIMEType method. I guess we can rely on that but it seems strange. Aren’t we moving to NSURLSession? Where’s the code for that case?

> Source/WebCore/platform/network/ios/QuickLook.mm:459
> +    bool isMainResourceLoad = handle->firstRequest().requester() == ResourceRequest::Requester::Main;

And, yes, this one is the other way around. Makes me even more sure the one above is wrong. I’m not sure you need a local variable in either of these cases. Doesn’t really help me understand the code all that much.

> Source/WebCore/platform/network/ios/QuickLook.mm:485
> +std::unique_ptr<QuickLookHandle> QuickLookHandle::createIfNecessary(ResourceLoader& loader, NSURLResponse *nsURLResponse)

Why not just call the response "response"? I don’t think we need to name it nsURLResponse.

This function is a *lot* like the one above that takes an NSURLConnection and a delegate. Maybe there’s a way to share more code?

> Source/WebCore/platform/network/ios/QuickLook.mm:494
> +    adjustMIMETypeForQuickLook(nsURLResponse._CFURLResponse);
> +
> +    if (!shouldCreateForMIMEType(nsURLResponse.MIMEType))
> +        return nullptr;

Tiny formatting nit: Why a blank line after adjustMIMEType here, but not in the other two functions?

[Chris Dumez]

Comment on attachment 276560 [details]
Patch

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

>> Source/WebCore/platform/network/ios/QuickLook.h:81
>> +    static std::unique_ptr<QuickLookHandle> createIfNecessary(ResourceHandle*, NSURLConnection *, NSURLResponse *, id delegate);
> 
> Should be ResourceHandle& if you’re going to change all the call sites anyway.

Ok.

>> Source/WebCore/platform/network/ios/QuickLook.h:83
>> +    static std::unique_ptr<QuickLookHandle> createIfNecessary(ResourceHandle*, SynchronousResourceHandleCFURLConnectionDelegate*, CFURLResponseRef);
> 
> Ditto.

Ok.

>> Source/WebCore/platform/network/ios/QuickLook.mm:113
>> +static void adjustMIMETypeForQuickLook(CFURLResponseRef cfResponse)
> 
> All the places that use this function call shouldCreateForMIMEType just after calling it, although they differ in how they get the MIME type. But this function knows exactly how to get the MIME type. Can we combine this adjustment and checking into a single function that’s more helpful for each of these call sites? Not sure exactly what to name it, but it would both adjust the MIME type and check if it’s eligible for quick look.

Ok, I will take a look.

>> Source/WebCore/platform/network/ios/QuickLook.mm:439
>> +std::unique_ptr<QuickLookHandle> QuickLookHandle::createIfNecessary(ResourceHandle* handle, NSURLConnection *connection, NSURLResponse *nsResponse, id delegate)
> 
> Why not just call the response "response"? I don’t think we need to name it nsResponse.

Ok

>> Source/WebCore/platform/network/ios/QuickLook.mm:442
>> +    bool isMainResourceLoad = handle->firstRequest().requester() != ResourceRequest::Requester::Main;
> 
> is main <- (requester != main)
> 
> Is that right? I think it’s backwards! How did you test?

Yes, this one is reversed. I tested the NSURLSession code path (below), which is what we now use everywhere.

>> Source/WebCore/platform/network/ios/QuickLook.mm:447
>> +    if (!shouldCreateForMIMEType(nsResponse.MIMEType))
> 
> This relies on the behavior where adjusting the MIME type on the underlying CFURLResponse will change the result of the MIMEType method. I guess we can rely on that but it seems strange. Aren’t we moving to NSURLSession? Where’s the code for that case?

Yes, it does rely on it and it is not uncommon in our networking code.
The code path used for NSURLSession / WebKit2 is the last factory function is this file.

>> Source/WebCore/platform/network/ios/QuickLook.mm:459
>> +    bool isMainResourceLoad = handle->firstRequest().requester() == ResourceRequest::Requester::Main;
> 
> And, yes, this one is the other way around. Makes me even more sure the one above is wrong. I’m not sure you need a local variable in either of these cases. Doesn’t really help me understand the code all that much.

Ok, will fold it back into the if case.

>> Source/WebCore/platform/network/ios/QuickLook.mm:485
>> +std::unique_ptr<QuickLookHandle> QuickLookHandle::createIfNecessary(ResourceLoader& loader, NSURLResponse *nsURLResponse)
> 
> Why not just call the response "response"? I don’t think we need to name it nsURLResponse.
> 
> This function is a *lot* like the one above that takes an NSURLConnection and a delegate. Maybe there’s a way to share more code?

FYI, this is the code path used for NSURLSession / WebKit2. I will look into sharing more code.

>> Source/WebCore/platform/network/ios/QuickLook.mm:494
>> +        return nullptr;
> 
> Tiny formatting nit: Why a blank line after adjustMIMEType here, but not in the other two functions?

Ok.

[Chris Dumez]

Created attachment 276572 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 276572 [details]
Patch

Clearing flags on attachment: 276572

Committed r199644: <http://trac.webkit.org/changeset/199644>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

I rolled this out in r199669 because it seems to have caused a 1-2% regression on *warm* PLT. I think this is likely due to the fact that the previous code would save the updated MIME type in the disk cache, and would need to adjust the MIME type for QuickLook when the response is a successful validation (304 status code), which is common in the context of the warm PLT.

The new code was in the WebContent process and as a result, is not aware of network validations happening in the Network Process Disk Cache. Also, since the MIME type is adjusted in WebCore, we were not able to save the updated MIME type in the disk cache.

I think the plan B for this is to keep the QuickLook MIME type adjustment code in the NetworkProcess and only run it for main resources. As a result, we will be a lot less likely to dlopen() QuickLook in the NetworkProcess (because main resources usually have the common text/html MIME type, which we don't adjust).

[Chris Dumez]

Created attachment 276646 [details]
Patch

[Chris Dumez]

take #2

[WebKit Commit Bot]

Comment on attachment 276646 [details]
Patch

Clearing flags on attachment: 276646

Committed r199682: <http://trac.webkit.org/changeset/199682>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.