Caught by the clang static analyzer. * UIProcess/API/Cocoa/WKProcessPool.mm: (policiesHashMapToDictionary): Switch from using [[NSMutableDictionary alloc] init] which returns a +1 retained object in MRR to [NSMutableDictionary new] which returns an autoreleased object under MRR. This bug caused 3 leaks when calling -[WKProcessPool _pluginLoadClientPolicies], which should return an autoreleased object based on its signature.
Created attachment 313578 [details] Patch v1
Comment on attachment 313578 [details] Patch v1 Thank you for fixing. Me and Objective C :(
(In reply to Chris Dumez from comment #2) > Comment on attachment 313578 [details] > Patch v1 > > Thank you for fixing. Me and Objective C :( If it's any consolation, your code would have been fine had WebKit switched to ARC by now!
Comment on attachment 313578 [details] Patch v1 Clearing flags on attachment: 313578 Committed r218678: <http://trac.webkit.org/changeset/218678>
All reviewed patches have been landed. Closing bug.
Comment on attachment 313578 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=313578&action=review > Source/WebKit2/ChangeLog:3 > + REGRESSION (r218419): 3 NSMutableDiciontary leaks calling -[WKProcessPool _pluginLoadClientPolicies] Well, now this embarrassing typo is preserved forever! NSMutableDiciontary => NSMutableDictionary
I forgot; -new also returns +1 retained objects, so this isn't fixed.
Created attachment 313614 [details] Patch v2
Created attachment 313636 [details] Patch v3
Comment on attachment 313636 [details] Patch v3 Clearing flags on attachment: 313636 Committed r218704: <http://trac.webkit.org/changeset/218704>
Comment on attachment 313636 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=313636&action=review > Source/WebKit2/ChangeLog:15 > + (-[WKProcessPool _pluginLoadClientPolicies]): Remove 'copy' > + attribute from @property declaration since it is read-only. The property being read-only is not in and of itself a reason to not specify the copy attribute. For example, NSString’s uppercaseString property is declared (readonly, copy). Please fix this. > Source/WebKit2/UIProcess/API/Cocoa/WKProcessPool.mm:316 > + NSMutableDictionary *policies = [NSMutableDictionary dictionaryWithCapacity:map.size()]; We normally use RetainPtr rather than autorelease. > Source/WebKit2/UIProcess/API/Cocoa/WKProcessPool.mm:319 > + policies[host] = [NSMutableDictionary dictionaryWithCapacity:hostPair.value.size()]; Ditto. > Source/WebKit2/UIProcess/API/Cocoa/WKProcessPool.mm:322 > + policies[host][bundlerIdentifier] = [NSMutableDictionary dictionaryWithCapacity:bundleIdentifierPair.value.size()]; Ditto.
Comment on attachment 313636 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=313636&action=review >> Source/WebKit2/ChangeLog:15 >> + attribute from @property declaration since it is read-only. > > The property being read-only is not in and of itself a reason to not specify the copy attribute. For example, NSString’s uppercaseString property is declared (readonly, copy). Please fix this. Can you please clarify for people like me what 'copy' brings in this case, given that there is no setter?
(In reply to Chris Dumez from comment #13) > Comment on attachment 313636 [details] > Patch v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313636&action=review > > >> Source/WebKit2/ChangeLog:15 > >> + attribute from @property declaration since it is read-only. > > > > The property being read-only is not in and of itself a reason to not specify the copy attribute. For example, NSString’s uppercaseString property is declared (readonly, copy). Please fix this. > > Can you please clarify for people like me what 'copy' brings in this case, > given that there is no setter? In the context of a readonly property, copy is used to indicate that either the receiver has copied the incoming value (e.g. if it’s coming from an initializer), or effectively gives you a new value each time you access the property.
Comment on attachment 313636 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=313636&action=review >>>> Source/WebKit2/ChangeLog:15 >>>> + attribute from @property declaration since it is read-only. >>> >>> The property being read-only is not in and of itself a reason to not specify the copy attribute. For example, NSString’s uppercaseString property is declared (readonly, copy). Please fix this. >> >> Can you please clarify for people like me what 'copy' brings in this case, given that there is no setter? > > In the context of a readonly property, copy is used to indicate that either the receiver has copied the incoming value (e.g. if it’s coming from an initializer), or effectively gives you a new value each time you access the property. Is that a WebKit convention, or an Objective-C convention? Does it have any effect on the generated code whatsoever? (I was under the impression it had no effect, so this is basically the equivalent of a documentation change.) Will post another patch shortly. >> Source/WebKit2/UIProcess/API/Cocoa/WKProcessPool.mm:316 >> + NSMutableDictionary *policies = [NSMutableDictionary dictionaryWithCapacity:map.size()]; > > We normally use RetainPtr rather than autorelease. Would we still switch to RetainPtr<> here if WebKit were compiled with ARC? Just curious because adding RetainPtr<> only to remove it again when we switch to ARC seems like busywork. I'll make these RetainPtr<> changes in another patch.
Reopening to address comments.
(In reply to David Kilzer (:ddkilzer) from comment #15) > Comment on attachment 313636 [details] > Patch v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313636&action=review > > >>>> Source/WebKit2/ChangeLog:15 > >>>> + attribute from @property declaration since it is read-only. > >>> > >>> The property being read-only is not in and of itself a reason to not specify the copy attribute. For example, NSString’s uppercaseString property is declared (readonly, copy). Please fix this. > >> > >> Can you please clarify for people like me what 'copy' brings in this case, given that there is no setter? > > > > In the context of a readonly property, copy is used to indicate that either the receiver has copied the incoming value (e.g. if it’s coming from an initializer), or effectively gives you a new value each time you access the property. > > Is that a WebKit convention, or an Objective-C convention? It’s a Cocoa API convention that the Modern WebKit API follows. > > Does it have any effect on the generated code whatsoever? No. > (I was under the > impression it had no effect, so this is basically the equivalent of a > documentation change.) Correct. > > Will post another patch shortly. > > >> Source/WebKit2/UIProcess/API/Cocoa/WKProcessPool.mm:316 > >> + NSMutableDictionary *policies = [NSMutableDictionary dictionaryWithCapacity:map.size()]; > > > > We normally use RetainPtr rather than autorelease. > > Would we still switch to RetainPtr<> here if WebKit were compiled with ARC? RetainPtr would no-op here in ARC so we probably wouldn’t use it if we were using ARC, but ARC wouldn’t necessarily place the object in an autorelease pool.
Created attachment 313644 [details] Patch v4
Comment on attachment 313644 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=313644&action=review > Source/WebKit2/ChangeLog:10 > + * UIProcess/API/Cocoa/WKProcessPool.mm: > + (policiesHashMapToDictionary): Use RetainPtr<> to avoid > + dumping objects into autoreleasepools unnecessarily. This change seems unrelated to the leaks; I’m not sure why you are including it in this patch. > Source/WebKit2/UIProcess/API/Cocoa/WKProcessPool.mm:320 > + RetainPtr<NSMutableDictionary> hostDictionary = adoptNS([[NSMutableDictionary alloc] initWithCapacity:hostPair.value.size()]); > + policies.get()[host] = hostDictionary.get(); You don’t need to use a local variable to use RetainPtr. You can write: policies.get()[host] = adoptNS([[NSMutableDictionary alloc] initWithCapacity:hostPair.value.size()]).get(); That line of code is shorter than the first line of code here. > Source/WebKit2/UIProcess/API/Cocoa/WKProcessPool.mm:324 > + RetainPtr<NSMutableDictionary> bundleIdentifierDictionary = adoptNS([[NSMutableDictionary alloc] initWithCapacity:bundleIdentifierPair.value.size()]); > + policies.get()[host][bundlerIdentifier] = bundleIdentifierDictionary.get(); Ditto. > Source/WebKit2/UIProcess/API/Cocoa/WKProcessPool.mm:328 > + RetainPtr<NSNumber> policyNumber = adoptNS([[NSNumber alloc] initWithUnsignedInt:versionPair.value]); > + policies.get()[host][bundlerIdentifier][version] = policyNumber.get(); Ditto.
Created attachment 313731 [details] Patch v5
Comment on attachment 313644 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=313644&action=review >> Source/WebKit2/ChangeLog:10 >> + dumping objects into autoreleasepools unnecessarily. > > This change seems unrelated to the leaks; I’m not sure why you are including it in this patch. Because this object originally leaked in v1 of this patch (that landed in r218678), so this is a follow-up patch to fix feedback given by mitz. I felt it was cleaner to keep all the follow-up changes associated with one bug. >> Source/WebKit2/UIProcess/API/Cocoa/WKProcessPool.mm:320 >> + policies.get()[host] = hostDictionary.get(); > > You don’t need to use a local variable to use RetainPtr. You can write: > > policies.get()[host] = adoptNS([[NSMutableDictionary alloc] initWithCapacity:hostPair.value.size()]).get(); > > That line of code is shorter than the first line of code here. See Patch v5.
Comment on attachment 313644 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=313644&action=review >>> Source/WebKit2/ChangeLog:10 >>> + dumping objects into autoreleasepools unnecessarily. >> >> This change seems unrelated to the leaks; I’m not sure why you are including it in this patch. > > Because this object originally leaked in v1 of this patch (that landed in r218678), so this is a follow-up patch to fix feedback given by mitz. > > I felt it was cleaner to keep all the follow-up changes associated with one bug. * Because this object originally leaked _BEFORE_ v1 of this patch (that landed in r218678). <https://trac.webkit.org/r218678>
Comment on attachment 313731 [details] Patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=313731&action=review > Source/WebKit2/ChangeLog:10 > + (policiesHashMapToDictionary): Use adoptNS().get() to avoid > + dumping objects into autoreleasepools unnecessarily. Why? Is this a wider policy we should have? We use autoreleased objects all over the place. > Source/WebKit2/ChangeLog:14 > + attribute to document that we're returning a new object on each > + invocation. I shouldn't have removed it in the v2 patch. Quite.
Comment on attachment 313731 [details] Patch v5 View in context: https://bugs.webkit.org/attachment.cgi?id=313731&action=review >> Source/WebKit2/ChangeLog:10 >> + dumping objects into autoreleasepools unnecessarily. > > Why? Is this a wider policy we should have? We use autoreleased objects all over the place. This is my interpretation of mitz's previous comment (see last item in Comment #17). Perhaps I'm reading into it too much?
Comment on attachment 313731 [details] Patch v5 Clearing flags on attachment: 313731 Committed r218760: <http://trac.webkit.org/changeset/218760>