135359 2014-07-28 15:19:19 -0700 [iOS] REGRESSION(r171526): PDF documents fail to load in WebKit1 with disk image caching enabled 2014-07-29 14:42:03 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 psolanki psolanki ap joepeck psolanki simon.fraser thorton oldest_to_newest 1025256 0 psolanki 2014-07-28 15:19:19 -0700 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. 1025278 1 psolanki 2014-07-28 16:04:50 -0700 <rdar://problem/17824645> 1025297 2 235637 psolanki 2014-07-28 16:23:15 -0700 Created attachment 235637 Patch 1025403 3 235637 darin 2014-07-29 00:53:21 -0700 Comment on attachment 235637 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. 1025488 4 235637 psolanki 2014-07-29 09:16:36 -0700 Comment on attachment 235637 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? 1025560 5 psolanki 2014-07-29 14:42:03 -0700 Committed r171766: <http://trac.webkit.org/changeset/171766> 235637 2014-07-28 16:23:15 -0700 2014-07-29 09:16:36 -0700 Patch bug-135359-20140728162301.patch text/plain 3639 psolanki U3VidmVyc2lvbiBSZXZpc2lvbjogMTcxNjgwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggODM0M2M1M2E5YTUxYWNk MWYyMjg5MjRmYjRhMzM1NjBiYmFkZTAwNy4uN2EyY2U0YjNhZTMxYTJhNzc2NTM5YmU0MWIyZDM1 OTc5ZmFhMzJjMyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI3IEBACisyMDE0LTA3LTI4ICBQcmF0 aWsgU29sYW5raSAgPHBzb2xhbmtpQGFwcGxlLmNvbT4KKworICAgICAgICBbaU9TXSBSRUdSRVNT SU9OKHIxNzE1MjYpOiBQREYgZG9jdW1lbnRzIGZhaWwgdG8gbG9hZCBpbiBXZWJLaXQxIHdpdGgg ZGlzayBpbWFnZSBjYWNoaW5nIGVuYWJsZWQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTEzNTM1OQorICAgICAgICA8cmRhcjovL3Byb2JsZW0vMTc4MjQ2 NDU+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgcjE3 MTUyNiBicm9rZSB0aGUgY2FzZSB3aGVyZSB3ZSBoYXZlIGEgbWVtb3J5IG1hcHBlZCBmaWxlIGZy b20gdGhlIERpc2tJbWFnZUNhY2hlIGluIHRoZQorICAgICAgICBTaGFyZWRCdWZmZXIuIEluIHN1 Y2ggYSBjYXNlLCBtX2J1ZmZlciBpcyBlbXB0eSBhbmQgY3JlYXRlQ0ZEYXRhKCkgcmV0dXJuZWQg YW4KKyAgICAgICAgV2ViQ29yZVNoYXJlZEJ1ZmZlckRhdGEgd2l0aCBhbiBlbXB0eSBidWZmZXIu CisKKyAgICAgICAgRml4IHRoaXMgYnkgdGFraW5nIHRoZSBlYXN5IHJvdXRlIG9mIGJyaW5naW5n IGJhY2sgdGhlIG9sZCBjb2RlIGZvciB0aGUgZGlzayBpbWFnZSBjYWNoZQorICAgICAgICBmaWxl IGJhY2tlZCBjYXNlLiBJbiB0aGUgbG9uZyBydW4gd2UgcHJvYmFibHkgd2FudCB0byByZW1vdmUg dGhlIGlPUyBzcGVjaWZpYyBkaXNrIGltYWdlCisgICAgICAgIGNhY2hlIGFueXdheS4KKworICAg ICAgICBObyBuZXcgdGVzdHMgYmVjYXVzZSB0aGUgYnVnIG9ubHkgb2NjdXJzIG9uIGRldmljZSBh bmQgd2UgY2FuJ3QgcnVuIHRlc3RzIG9uIGRldmljZSB5ZXQuCisKKyAgICAgICAgKiBwbGF0Zm9y bS9tYWMvU2hhcmVkQnVmZmVyTWFjLm1tOgorICAgICAgICAoLVtXZWJDb3JlU2hhcmVkQnVmZmVy RGF0YSBpbml0V2l0aERpc2tJbWFnZVNoYXJlZEJ1ZmZlcjpdKToKKyAgICAgICAgKC1bV2ViQ29y ZVNoYXJlZEJ1ZmZlckRhdGEgbGVuZ3RoXSk6CisgICAgICAgICgtW1dlYkNvcmVTaGFyZWRCdWZm ZXJEYXRhIGJ5dGVzXSk6CisgICAgICAgIChXZWJDb3JlOjpTaGFyZWRCdWZmZXI6OmNyZWF0ZUNG RGF0YSk6CisKIDIwMTQtMDctMjggIEJyZW50IEZ1bGdoYW0gIDxiZnVsZ2hhbUBhcHBsZS5jb20+ CiAKICAgICAgICAgVW5yZXZpZXdlZCAnbWVyZ2UnIGZpeC4KZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL21hYy9TaGFyZWRCdWZmZXJNYWMubW0gYi9Tb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS9tYWMvU2hhcmVkQnVmZmVyTWFjLm1tCmluZGV4IDhjNTAxZDU5YmFiZmY1ZTg3YWZh NTM0ZjZjMWZlZjczYWE5ZGY5NDYuLjA0YmFhMmExMzhlYWM1NTNiOTNiMmQ4NjAxNjA5ZDllMmUz OTIwYTcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL21hYy9TaGFyZWRCdWZm ZXJNYWMubW0KKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbWFjL1NoYXJlZEJ1ZmZlck1h Yy5tbQpAQCAtMzcsOSArMzcsMTUgQEAgdXNpbmcgbmFtZXNwYWNlIFdlYkNvcmU7CiBAaW50ZXJm YWNlIFdlYkNvcmVTaGFyZWRCdWZmZXJEYXRhIDogTlNEYXRhCiB7CiAgICAgUmVmUHRyPFNoYXJl ZEJ1ZmZlcjo6RGF0YUJ1ZmZlcj4gYnVmZmVyOworI2lmIEVOQUJMRShESVNLX0lNQUdFX0NBQ0hF KQorICAgIFJlZlB0cjxTaGFyZWRCdWZmZXI+IHNoYXJlZEJ1ZmZlcjsKKyNlbmRpZgogfQogCiAt IChpZClpbml0V2l0aFNoYXJlZEJ1ZmZlckRhdGFCdWZmZXI6KFNoYXJlZEJ1ZmZlcjo6RGF0YUJ1 ZmZlciopZGF0YUJ1ZmZlcjsKKyNpZiBFTkFCTEUoRElTS19JTUFHRV9DQUNIRSkKKy0gKGlkKWlu aXRXaXRoRGlza0ltYWdlU2hhcmVkQnVmZmVyOihTaGFyZWRCdWZmZXIqKXNoYXJlZEJ1ZmZlcjsK KyNlbmRpZgogQGVuZAogCiBAaW1wbGVtZW50YXRpb24gV2ViQ29yZVNoYXJlZEJ1ZmZlckRhdGEK QEAgLTc2LDEzICs4MiwzOSBAQCAtIChpZClpbml0V2l0aFNoYXJlZEJ1ZmZlckRhdGFCdWZmZXI6 KFNoYXJlZEJ1ZmZlcjo6RGF0YUJ1ZmZlciopZGF0YUJ1ZmZlcgogICAgIHJldHVybiBzZWxmOwog fQogCisjaWYgRU5BQkxFKERJU0tfSU1BR0VfQ0FDSEUpCistIChpZClpbml0V2l0aERpc2tJbWFn ZVNoYXJlZEJ1ZmZlcjooU2hhcmVkQnVmZmVyKilkaXNrSW1hZ2VTaGFyZWRCdWZmZXIKK3sKKyAg ICBzZWxmID0gW3N1cGVyIGluaXRdOworCisgICAgaWYgKCFzZWxmKQorICAgICAgICByZXR1cm4g bmlsOworCisgICAgQVNTRVJUKGRpc2tJbWFnZVNoYXJlZEJ1ZmZlci0+aXNNZW1vcnlNYXBwZWQo KSk7CisgICAgc2hhcmVkQnVmZmVyID0gZGlza0ltYWdlU2hhcmVkQnVmZmVyOworICAgIHJldHVy biBzZWxmOworfQorI2VuZGlmCisKIC0gKE5TVUludGVnZXIpbGVuZ3RoCiB7CisjaWYgRU5BQkxF KERJU0tfSU1BR0VfQ0FDSEUpCisgICAgaWYgKHNoYXJlZEJ1ZmZlcikgeworICAgICAgICBBU1NF UlQoc2hhcmVkQnVmZmVyLT5pc01lbW9yeU1hcHBlZCgpKTsKKyAgICAgICAgcmV0dXJuIHNoYXJl ZEJ1ZmZlci0+c2l6ZSgpOworICAgIH0KKyNlbmRpZgogICAgIHJldHVybiBidWZmZXItPmRhdGEu c2l6ZSgpOwogfQogCiAtIChjb25zdCB2b2lkICopYnl0ZXMKIHsKKyNpZiBFTkFCTEUoRElTS19J TUFHRV9DQUNIRSkKKyAgICBpZiAoc2hhcmVkQnVmZmVyKSB7CisgICAgICAgIEFTU0VSVChzaGFy ZWRCdWZmZXItPmlzTWVtb3J5TWFwcGVkKCkpOworICAgICAgICByZXR1cm4gcmVpbnRlcnByZXRf Y2FzdDxjb25zdCB2b2lkKj4oc2hhcmVkQnVmZmVyLT5kYXRhKCkpOworICAgIH0KKyNlbmRpZgog ICAgIHJldHVybiByZWludGVycHJldF9jYXN0PGNvbnN0IHZvaWQqPihidWZmZXItPmRhdGEuZGF0 YSgpKTsKIH0KIApAQCAtMTEwLDYgKzE0MiwxMSBAQCBSZXRhaW5QdHI8Q0ZEYXRhUmVmPiBTaGFy ZWRCdWZmZXI6OmNyZWF0ZUNGRGF0YSgpCiAgICAgICAgIHJldHVybiBtX2RhdGFBcnJheS5hdCgw KTsKICNlbmRpZgogCisjaWYgRU5BQkxFKERJU0tfSU1BR0VfQ0FDSEUpCisgICAgaWYgKGlzTWVt b3J5TWFwcGVkKCkpCisgICAgICAgIHJldHVybiBhZG9wdENGKHJlaW50ZXJwcmV0X2Nhc3Q8Q0ZE YXRhUmVmPihbW1dlYkNvcmVTaGFyZWRCdWZmZXJEYXRhIGFsbG9jXSBpbml0V2l0aERpc2tJbWFn ZVNoYXJlZEJ1ZmZlcjp0aGlzXSkpOworI2VuZGlmCisKICAgICBkYXRhKCk7IC8vIEZvcmNlIGRh dGEgaW50byBtX2J1ZmZlciBmcm9tIHNlZ21lbnRzIG9yIGRhdGEgYXJyYXkuCiAgICAgaWYgKGhh c1B1cmdlYWJsZUJ1ZmZlcigpKSB7CiAgICAgICAgIFJlZlB0cjxTaGFyZWRCdWZmZXI6OkRhdGFC dWZmZXI+IGNvcGllZEJ1ZmZlciA9IGFkb3B0UmVmKG5ldyBEYXRhQnVmZmVyKTsK