Bug 222439 – Reduce explicit usage of [objC retain] in WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Reduce explicit usage of [objC retain] in WebKit.

[Chris Dumez]

Created attachment 421580 [details]
Patch

[Darin Adler]

Comment on attachment 421580 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421580&action=review

I did not manage to spot the pattern for when you chose to use auto/retainPtr

> Source/WebCore/platform/ios/WebCoreMotionManager.mm:182
> +        if (!m_updateTimer)
> +            m_updateTimer = [NSTimer scheduledTimerWithTimeInterval:kMotionUpdateInterval target:self selector:@selector(update) userInfo:nil repeats:YES];

Looks like you fixed a leak here. Maybe land this separately?

> Source/WebCore/platform/ios/wak/WebCoreThread.mm:219
> +static void SendDelegateMessage(RetainPtr<NSInvocation> invocation)

Why not RetainPtr&&?

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 421580 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421580&action=review
> 
> I did not manage to spot the pattern for when you chose to use auto/retainPtr
> 
> > Source/WebCore/platform/ios/WebCoreMotionManager.mm:182
> > +        if (!m_updateTimer)
> > +            m_updateTimer = [NSTimer scheduledTimerWithTimeInterval:kMotionUpdateInterval target:self selector:@selector(update) userInfo:nil repeats:YES];
> 
> Looks like you fixed a leak here. Maybe land this separately?

A lot of my RetainPtr patches fixed leaks or over-releases. I usually have not split those into separate patches but I can if you feel this is better.

> 
> > Source/WebCore/platform/ios/wak/WebCoreThread.mm:219
> > +static void SendDelegateMessage(RetainPtr<NSInvocation> invocation)
> 
> Why not RetainPtr&&?

I'll update.

[Darin Adler]

Comment on attachment 421580 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421580&action=review

>>> Source/WebCore/platform/ios/WebCoreMotionManager.mm:182
>>> +            m_updateTimer = [NSTimer scheduledTimerWithTimeInterval:kMotionUpdateInterval target:self selector:@selector(update) userInfo:nil repeats:YES];
>> 
>> Looks like you fixed a leak here. Maybe land this separately?
> 
> A lot of my RetainPtr patches fixed leaks or over-releases. I usually have not split those into separate patches but I can if you feel this is better.

I don’t feel strongly.

Generally I like the idea of separating bug fixes from refactoring, even if we discover the bug fixes while refactoring.

[Chris Dumez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 421580 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421580&action=review
> 
> >>> Source/WebCore/platform/ios/WebCoreMotionManager.mm:182
> >>> +            m_updateTimer = [NSTimer scheduledTimerWithTimeInterval:kMotionUpdateInterval target:self selector:@selector(update) userInfo:nil repeats:YES];
> >> 
> >> Looks like you fixed a leak here. Maybe land this separately?
> > 
> > A lot of my RetainPtr patches fixed leaks or over-releases. I usually have not split those into separate patches but I can if you feel this is better.
> 
> I don’t feel strongly.
> 
> Generally I like the idea of separating bug fixes from refactoring, even if
> we discover the bug fixes while refactoring.

The whole point of this refactoring is to find & fix those bugs though :)

[Chris Dumez]

Created attachment 421596 [details]
Patch

[Chris Dumez]

Created attachment 421598 [details]
Patch

[Chris Dumez]

Created attachment 421599 [details]
Patch

[Darin Adler]

Comment on attachment 421599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421599&action=review

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:286
> +    static RetainPtr<NSSet> fontFamilySet = [NSSet setWithObjects:

Why not use NeverDestroyed here? I know we don’t have this project set up to fail to build if we have exit-time destructors, but such destructors still aren’t desirable. Same for all the other globals. Otherwise, this patch adds a lot of exit time code to DumpRenderTree and other tools; the code is cleaner without NeverDestroyed, but the binary is a tiny bit bigger and exit might even be a little bit slower.

The old code we are replacing did not release these objects on exit.

[Chris Dumez]

Created attachment 421714 [details]
Patch

[Chris Dumez]

(In reply to Darin Adler from comment #9)
> Comment on attachment 421599 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421599&action=review
> 
> > Tools/DumpRenderTree/mac/DumpRenderTree.mm:286
> > +    static RetainPtr<NSSet> fontFamilySet = [NSSet setWithObjects:
> 
> Why not use NeverDestroyed here? I know we don’t have this project set up to
> fail to build if we have exit-time destructors, but such destructors still
> aren’t desirable. Same for all the other globals. Otherwise, this patch adds
> a lot of exit time code to DumpRenderTree and other tools; the code is
> cleaner without NeverDestroyed, but the binary is a tiny bit bigger and exit
> might even be a little bit slower.
> 
> The old code we are replacing did not release these objects on exit.

I figured it may not be worth the trouble for testing code but I updated the patch as suggested.

[Chris Dumez]

Created attachment 421721 [details]
Patch

[EWS]

Committed r273610: <https://commits.webkit.org/r273610>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421721 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74816519>