Bug 173689 – REGRESSION (r218419): 3 NSMutableDictionary leaks calling -[WKProcessPool _pluginLoadClientPolicies]

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 313578 [details]
Patch v1

[Chris Dumez]

Comment on attachment 313578 [details]
Patch v1

Thank you for fixing. Me and Objective C :(

[David Kilzer (:ddkilzer)]

(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!

[WebKit Commit Bot]

Comment on attachment 313578 [details]
Patch v1

Clearing flags on attachment: 313578

Committed r218678: <http://trac.webkit.org/changeset/218678>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

I forgot; -new also returns +1 retained objects, so this isn't fixed.

[David Kilzer (:ddkilzer)]

Created attachment 313614 [details]
Patch v2

[David Kilzer (:ddkilzer)]

Created attachment 313636 [details]
Patch v3

[WebKit Commit Bot]

Comment on attachment 313636 [details]
Patch v3

Clearing flags on attachment: 313636

Committed r218704: <http://trac.webkit.org/changeset/218704>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

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.

[Chris Dumez]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 313644 [details]
Patch v4

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

Created attachment 313731 [details]
Patch v5

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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>

[Tim Horton]

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.

[David Kilzer (:ddkilzer)]

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?

[WebKit Commit Bot]

Comment on attachment 313731 [details]
Patch v5

Clearing flags on attachment: 313731

Committed r218760: <http://trac.webkit.org/changeset/218760>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.