Bug 135359 – [iOS] REGRESSION(r171526): PDF documents fail to load in WebKit1 with disk image caching enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Pratik Solanki]

When disk image caching is enabled, m_buffer in SharedBuffer is empty. createCFData() returns a WebCoreSharedBufferData with that empty buffer data and this causes pdf to fail to load. createCFData needs to take care of disk image cache backed resources.

[Pratik Solanki]

<rdar://problem/17824645>

[Pratik Solanki]

Created attachment 235637 [details]
Patch

[Darin Adler]

Comment on attachment 235637 [details]
Patch

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

Please fix the GC problem I spotted. Also probably need to find a way to test under garbage collection.

> Source/WebCore/platform/mac/SharedBufferMac.mm:42
>      RefPtr<SharedBuffer::DataBuffer> buffer;
> +#if ENABLE(DISK_IMAGE_CACHE)
> +    RefPtr<SharedBuffer> sharedBuffer;
> +#endif

I think the instance field names here are confusing. They are basically both shared buffers, and naming one buffer and the other sharedBuffer is not going to distinguish them. Please consider names that would make the code clearer.

> Source/WebCore/platform/mac/SharedBufferMac.mm:47
> +- (id)initWithDiskImageSharedBuffer:(SharedBuffer*)sharedBuffer;

I’d think we’d want to call this initWithMemoryMappedSharedBuffer: instead.

> Source/WebCore/platform/mac/SharedBufferMac.mm:86
> +- (id)initWithDiskImageSharedBuffer:(SharedBuffer*)diskImageSharedBuffer

The function assumes this pointer is non-null, so it should take a reference rather than a pointer.

> Source/WebCore/platform/mac/SharedBufferMac.mm:93
> +    ASSERT(diskImageSharedBuffer->isMemoryMapped());

Why assert this after calling [super init]? I’d think an assertion about the validity of an argument would come first.

> Source/WebCore/platform/mac/SharedBufferMac.mm:103
> +        ASSERT(sharedBuffer->isMemoryMapped());

Do we really need to keep asserting this repeatedly? We already assert it in the init method.

> Source/WebCore/platform/mac/SharedBufferMac.mm:114
> +        ASSERT(sharedBuffer->isMemoryMapped());

Do we really need to keep asserting this repeatedly? We already assert it in the init method.

> Source/WebCore/platform/mac/SharedBufferMac.mm:115
> +        return reinterpret_cast<const void*>(sharedBuffer->data());

There’s no need for a typecast here at all. A const char* can be converted into a const void* without a cast. And if we did need a case, there’d be no reason to use reinterpret_cast instead of static_cast.

> Source/WebCore/platform/mac/SharedBufferMac.mm:118
>      return reinterpret_cast<const void*>(buffer->data.data());

No need for a typecast here either.

> Source/WebCore/platform/mac/SharedBufferMac.mm:147
> +        return adoptCF(reinterpret_cast<CFDataRef>([[WebCoreSharedBufferData alloc] initWithDiskImageSharedBuffer:this]));

This does not do the right thing for garbage collection. You can’t balance an Objective-C alloc with a CFRelease, and that’s what this code tries to do. But since we only currently turn on DISK_IMAGE_CACHE on iOS and we don’t support GC on iOS we can get away with this sloppiness. Unfortunately I see the same thing a few lines down from here, in code that is not specific to iOS, so it looks like r171526 broke WebKit running under garbage collection.

[Pratik Solanki]

Comment on attachment 235637 [details]
Patch

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

>> Source/WebCore/platform/mac/SharedBufferMac.mm:147
>> +        return adoptCF(reinterpret_cast<CFDataRef>([[WebCoreSharedBufferData alloc] initWithDiskImageSharedBuffer:this]));
> 
> This does not do the right thing for garbage collection. You can’t balance an Objective-C alloc with a CFRelease, and that’s what this code tries to do. But since we only currently turn on DISK_IMAGE_CACHE on iOS and we don’t support GC on iOS we can get away with this sloppiness. Unfortunately I see the same thing a few lines down from here, in code that is not specific to iOS, so it looks like r171526 broke WebKit running under garbage collection.

Thanks for spotting this. The original code was

    return adoptCF((CFDataRef)adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this]).leakRef());

Looks like both adoptNS and adoptCF are needed. Is this correct?

I also made createNSData() call createCFData()

-    return adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this]);
+    return adoptNS((NSData *)createCFData().leakRef());

Would that do the right thing under GC?

[Pratik Solanki]

Committed r171766: <http://trac.webkit.org/changeset/171766>