Summary: | [iOS] REGRESSION(r171526): PDF documents fail to load in WebKit1 with disk image caching enabled | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pratik Solanki <psolanki> | ||||
Component: | Platform | Assignee: | Pratik Solanki <psolanki> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ap, joepeck, psolanki, simon.fraser, thorton | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Pratik Solanki
2014-07-28 15:19:19 PDT
Created attachment 235637 [details]
Patch
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. 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? Committed r171766: <http://trac.webkit.org/changeset/171766> |