Bug 135727 – Cached file backed resources don't make it to the Web Process when NETWORK_CFDATA_ARRAY_CALLBACK is enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Pratik Solanki]

If we have a resource that is file backed in CFNetwork, then we try to send that data over to the web process so that the web process uses file backed memory. Unfortunately, this is not working on iOS because iOS uses the data array calbacks. We have this check in NetworkResourceLoader::tryGetShareableHandleFromSharedBuffer().

    if (!buffer->hasPlatformData())
        return;

For data array callbacks, hasPlatformData() will return false since SharedBuffer has a data array and not a single CFDataRef. This breaks the optimization. If I comment out the above code and fall through, the code does

    RetainPtr<CFDataRef> data = buffer->createCFData();
    if (_CFURLCacheIsResponseDataMemMapped(cache, data.get()) == kCFBooleanFalse)
        return;


createCFData() will do the right thing and the CFNetwork SPI will return true which will make our optimization work again.

The second mechanism - the disk cache notification works fine.

[Pratik Solanki]

<rdar://problem/17947880>

[Pratik Solanki]

Created attachment 236251 [details]
Patch

[Pratik Solanki]

Comment on attachment 236251 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        (WebCore::SharedBuffer::createCFData):

Is there a better name I could use for getCFDataIfAvailable? I don't like the mismatch of get and create but createCFDataIfAvailable() seemed odd.

[Alexey Proskuryakov]

Comment on attachment 236251 [details]
Patch

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

>> Source/WebCore/ChangeLog:16
>> +        (WebCore::SharedBuffer::createCFData):
> 
> Is there a better name I could use for getCFDataIfAvailable? I don't like the mismatch of get and create but createCFDataIfAvailable() seemed odd.

createCFDataIfInternallyRepresentedAsCFData()? createCFDataNoCopy()?

[Pratik Solanki]

Comment on attachment 236251 [details]
Patch

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

>>> Source/WebCore/ChangeLog:16
>>> +        (WebCore::SharedBuffer::createCFData):
>> 
>> Is there a better name I could use for getCFDataIfAvailable? I don't like the mismatch of get and create but createCFDataIfAvailable() seemed odd.
> 
> createCFDataIfInternallyRepresentedAsCFData()? createCFDataNoCopy()?

copyCFDataNoCreate() ?

[Pratik Solanki]

Created attachment 236390 [details]
Patch2

I like copyCFDataNoCreate. Patch with new name.

[Darin Adler]

Comment on attachment 236390 [details]
Patch2

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

> Source/WebCore/ChangeLog:10
> +        Add SharedBuffer::copyCFDataNoCreate() which returns CFDataRef if it has one. Refactor
> +        this code out of createCFData().

I don’t like the grammar of the phrase “copy CFData no create”. I’d prefer copyExistingCFData or copyCFDataWithoutCreating.

> Source/WebCore/platform/mac/SharedBufferMac.mm:148
> +    RetainPtr<CFDataRef> cfData = copyCFDataNoCreate();
> +    if (cfData)
> +        return cfData;

Would be nice to put the definition inside the if statement.

[Darin Adler]

Comment on attachment 236390 [details]
Patch2

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

> Source/WebKit2/ChangeLog:13
> +        so that SharedBuffer can take car of the data array case.

typo: "take car"

[Darin Adler]

Comment on attachment 236390 [details]
Patch2

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

>> Source/WebCore/ChangeLog:10
>> +        this code out of createCFData().
> 
> I don’t like the grammar of the phrase “copy CFData no create”. I’d prefer copyExistingCFData or copyCFDataWithoutCreating.

Hmm, why copy? It should just be existingCFData and should not return a RetainPtr.

[Pratik Solanki]

(In reply to comment #9)
> (From update of attachment 236390 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236390&action=review
> 
> >> Source/WebCore/ChangeLog:10
> >> +        this code out of createCFData().
> > 
> > I don’t like the grammar of the phrase “copy CFData no create”. I’d prefer copyExistingCFData or copyCFDataWithoutCreating.
> 
> Hmm, why copy? It should just be existingCFData and should not return a RetainPtr.

I like this! I'll change it to existingCFData and return a raw pointer.

[Pratik Solanki]

Committed r172502: <http://trac.webkit.org/changeset/172502>