RESOLVED FIXED 173721
REGRESSION (r218419): 7 leaks in PluginLoadClientPolicies.mm
https://bugs.webkit.org/show_bug.cgi?id=173721
Summary REGRESSION (r218419): 7 leaks in PluginLoadClientPolicies.mm
David Kilzer (:ddkilzer)
Reported 2017-06-22 09:41:02 PDT
Use autoreleased methods to create NSMutableDictionary and NSNumber objects instead of +1 retained methods. * TestWebKitAPI/Tests/WebKit2Cocoa/PluginLoadClientPolicies.mm: (TEST):
Attachments
Patch v1 (3.65 KB, patch)
2017-06-22 09:42 PDT, David Kilzer (:ddkilzer)
no flags
Patch v2 (4.14 KB, patch)
2017-06-22 11:37 PDT, David Kilzer (:ddkilzer)
joepeck: review+
David Kilzer (:ddkilzer)
Comment 1 2017-06-22 09:42:03 PDT
Created attachment 313638 [details] Patch v1
David Kilzer (:ddkilzer)
Comment 2 2017-06-22 11:37:55 PDT
Created attachment 313646 [details] Patch v2
Joseph Pecoraro
Comment 3 2017-06-22 11:56:51 PDT
Comment on attachment 313646 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=313646&action=review > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/PluginLoadClientPolicies.mm:71 > + RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]); r=me, but I find this way harder to read. How about switching to ObjC literal syntax? You'd end up with something like: RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]); [newPolicies removeObjectForKey:@"apple.com"]; newPolicies.get()[@"google.com"] = @{ @"com.macromedia.Flash Player.plugin": @{ @"26.0.0.126": @(kWKPluginLoadClientPolicyAllowAlways), }, @"com.apple.QuickTime.plugin": @{ @"1.0": @(kWKPluginLoadClientPolicyBlock), @"1.1": @(kWKPluginLoadClientPolicyAllow), }, };
David Kilzer (:ddkilzer)
Comment 4 2017-06-22 14:03:52 PDT
Comment on attachment 313646 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=313646&action=review >> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/PluginLoadClientPolicies.mm:71 >> + RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]); > > r=me, but I find this way harder to read. > > How about switching to ObjC literal syntax? You'd end up with something like: > > RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]); > [newPolicies removeObjectForKey:@"apple.com"]; > newPolicies.get()[@"google.com"] = @{ > @"com.macromedia.Flash Player.plugin": @{ > @"26.0.0.126": @(kWKPluginLoadClientPolicyAllowAlways), > }, > @"com.apple.QuickTime.plugin": @{ > @"1.0": @(kWKPluginLoadClientPolicyBlock), > @"1.1": @(kWKPluginLoadClientPolicyAllow), > }, > }; I like this better, but it has the (slight) disadvantage of putting all the objects in an autoreleasepool. I'll switch to adoptNS().get() instead, as we talked about in person.
David Kilzer (:ddkilzer)
Comment 5 2017-06-22 14:36:20 PDT
Darin Adler
Comment 6 2017-06-25 16:41:51 PDT
Comment on attachment 313646 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=313646&action=review >>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/PluginLoadClientPolicies.mm:71 >>> + RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]); >> >> r=me, but I find this way harder to read. >> >> How about switching to ObjC literal syntax? You'd end up with something like: >> >> RetainPtr<NSMutableDictionary> newPolicies = adoptNS([policies mutableCopy]); >> [newPolicies removeObjectForKey:@"apple.com"]; >> newPolicies.get()[@"google.com"] = @{ >> @"com.macromedia.Flash Player.plugin": @{ >> @"26.0.0.126": @(kWKPluginLoadClientPolicyAllowAlways), >> }, >> @"com.apple.QuickTime.plugin": @{ >> @"1.0": @(kWKPluginLoadClientPolicyBlock), >> @"1.1": @(kWKPluginLoadClientPolicyAllow), >> }, >> }; > > I like this better, but it has the (slight) disadvantage of putting all the objects in an autoreleasepool. > > I'll switch to adoptNS().get() instead, as we talked about in person. Readability seems *so* much more important than putting a few objects into the autorelease pool.
Note You need to log in before you can comment on or make changes to this bug.