[webkit-reviews] review granted: [Bug 135359] [iOS] REGRESSION(r171526): PDF documents fail to load in WebKit1 with disk image caching enabled : [Attachment 235637] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Pratik Solanki <psolanki at apple.com>'s
request for review:
Bug 135359: [iOS] REGRESSION(r171526): PDF documents fail to load in WebKit1
with disk image caching enabled
https://bugs.webkit.org/show_bug.cgi?id=135359

Attachment 235637: Patch
https://bugs.webkit.org/attachment.cgi?id=235637&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.