Bug 121237

Summary: SharedBuffer::createNSData should return a RetainPtr<NSData>
Product: WebKit Reporter: Anders Carlsson <andersca>
Component: New BugsAssignee: Anders Carlsson <andersca>
Status: RESOLVED FIXED    
Severity: Normal CC: psolanki, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch darin: review+

Anders Carlsson
Reported 2013-09-12 10:45:01 PDT
SharedBuffer::createNSData should return a RetainPtr<NSData>
Attachments
Patch (20.34 KB, patch)
2013-09-12 11:21 PDT, Anders Carlsson
darin: review+
Anders Carlsson
Comment 1 2013-09-12 11:21:18 PDT
Darin Adler
Comment 2 2013-09-12 11:31:08 PDT
Comment on attachment 211451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211451&action=review You said you fixed two leaks. I saw the mention of one in the ChangeLog but didn’t spot it, nor the other one. Also, patch did not apply. > Source/WebCore/loader/ResourceBuffer.h:79 > + SharedBuffer::NSDataRetainPtr createNSData(); The name SharedBuffer::NSDataRetainPtr does not make it clear why RetainPtr<NSData> is not used; would be better if the name made that clear. And not really sure this type should be a member of SharedBuffer. It doesn’t seem to be an issue that’s specific to that class, even if we are only using it there for now. Your name suggestion NSDataRetainPtrWithoutImplicitConversionOperator is good. > Source/WebCore/loader/archive/ArchiveFactory.cpp:67 > + mimeTypes.set("application/x-webarchive", static_cast<RawDataCreationFunction*>(&archiveFactoryCreate<LegacyWebArchive>)); I don’t know why this cast is needed, nor why it’s safe. Change log doesn’t say. > Source/WebCore/platform/SharedBuffer.h:68 > + // Once both Mac and iOS builds with this change we can change the return type to be RetainPtr<NSData>. Comment should say something like "because this mistake is unlikely in new code, although a real worry during the transition". > Source/WebCore/platform/mac/SharedBufferMac.mm:101 > + return adoptNS(static_cast<NSData *>([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this])); Why is the cast to NSData * needed? > Source/WebKit/mac/WebView/WebDataSource.mm:408 > - return [mainResourceData->createNSData() autorelease]; > + return [mainResourceData->createNSData().leakRef() autorelease]; Does RetainPtr need a leakRef/autorelease function of some sort?
Darin Adler
Comment 3 2013-09-12 11:37:32 PDT
Comment on attachment 211451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211451&action=review >> Source/WebKit/mac/WebView/WebDataSource.mm:408 >> + return [mainResourceData->createNSData().leakRef() autorelease]; > > Does RetainPtr need a leakRef/autorelease function of some sort? Probably would want to name it autorelease() and then have to decide if it’s legal on CF types or not.
Anders Carlsson
Comment 4 2013-09-12 12:42:23 PDT
(In reply to comment #2) > (From update of attachment 211451 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211451&action=review > > You said you fixed two leaks. I saw the mention of one in the ChangeLog but didn’t spot it, nor the other one. Also, patch did not apply. One was in -[WebHTMLView namesOfPromisedFilesDroppedAtDestination:], the other one was PDFDocumentImage::createPDFDocument. > > > Source/WebCore/loader/archive/ArchiveFactory.cpp:67 > > + mimeTypes.set("application/x-webarchive", static_cast<RawDataCreationFunction*>(&archiveFactoryCreate<LegacyWebArchive>)); > > I don’t know why this cast is needed, nor why it’s safe. Change log doesn’t say. I’ll remove this. > > > Source/WebCore/platform/SharedBuffer.h:68 > > + // Once both Mac and iOS builds with this change we can change the return type to be RetainPtr<NSData>. > > Comment should say something like "because this mistake is unlikely in new code, although a real worry during the transition". > > > Source/WebCore/platform/mac/SharedBufferMac.mm:101 > > + return adoptNS(static_cast<NSData *>([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this])); > > Why is the cast to NSData * needed? It’s because I didn’t make the NSDataRetainPtrWithoutImplicitConversionOperator constructor work on any RetainPtr that can be converted to RetainPtr<NSData>. I’ll fix that. > > > Source/WebKit/mac/WebView/WebDataSource.mm:408 > > - return [mainResourceData->createNSData() autorelease]; > > + return [mainResourceData->createNSData().leakRef() autorelease]; > > Does RetainPtr need a leakRef/autorelease function of some sort? If this pattern is common enough, then maybe!
Anders Carlsson
Comment 5 2013-09-12 12:53:15 PDT
Tim Horton
Comment 6 2014-04-16 19:45:11 PDT
Is it time to get rid of NSDataRetainPtrWithoutImplicitConversionOperator? Getting rid of it makes https://bugs.webkit.org/show_bug.cgi?id=131782 easier.
Anders Carlsson
Comment 7 2014-04-17 11:51:12 PDT
(In reply to comment #6) > Is it time to get rid of NSDataRetainPtrWithoutImplicitConversionOperator? Getting rid of it makes https://bugs.webkit.org/show_bug.cgi?id=131782 easier. Absolutely.
Pratik Solanki
Comment 8 2014-07-23 16:30:53 PDT
(In reply to comment #7) > (In reply to comment #6) > > Is it time to get rid of NSDataRetainPtrWithoutImplicitConversionOperator? Getting rid of it makes https://bugs.webkit.org/show_bug.cgi?id=131782 easier. > > Absolutely. https://bugs.webkit.org/show_bug.cgi?id=135219
Note You need to log in before you can comment on or make changes to this bug.