Bug 186301

Summary:

[Cocoa] Update some JavaScriptCore code to be more ready for ARC

Product:

WebKit

Reporter:

Darin Adler <darin>

Component:

JavaScriptCore

Assignee:

Darin Adler <darin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, dbates, ews-watchlist, joepeck, keith_miller, mark.lam, mitz, msaboff, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

221107

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	andersca: review+

Darin Adler

Reported 2018-06-04 22:51:09 PDT

[Cocoa] Update some JavaScriptCore code to be more ready for ARC

Attachments
Patch (5.79 KB, patch) 2018-06-04 22:52 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (5.62 KB, patch) 2018-06-05 08:50 PDT, Darin Adler	no flags	Details Formatted Diff Diff
Patch (4.77 KB, patch) 2018-06-05 08:53 PDT, Darin Adler	andersca: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Darin Adler

Comment 1 2018-06-04 22:52:55 PDT

Created attachment 341955 [details] Patch

Anders Carlsson

Comment 2 2018-06-05 07:20:17 PDT

Comment on attachment 341955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341955&action=review > Source/JavaScriptCore/API/JSAPIWrapperObject.mm:95 > - m_wrappedObject = [static_cast<id>(wrappedObject) retain]; > + m_wrappedObject = (__bridge id)wrappedObject; Won't this lead to a memory leak when compiling without ARC now?

Darin Adler

Comment 3 2018-06-05 08:15:19 PDT

Comment on attachment 341955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341955&action=review >> Source/JavaScriptCore/API/JSAPIWrapperObject.mm:95 >> + m_wrappedObject = (__bridge id)wrappedObject; > > Won't this lead to a memory leak when compiling without ARC now? Yes should have omitted this change; will remove it. I’ve been doing a verification pass on these “ARC prep” patches where I search for release and retain in the patch, which I think would have caught this.

Darin Adler

Comment 4 2018-06-05 08:50:49 PDT

Created attachment 341969 [details] Patch

Darin Adler

Comment 5 2018-06-05 08:51:32 PDT

New patch omits the incorrect change Anders pointed out, and also fixes another leak elsewhere in the patch.

Darin Adler

Comment 6 2018-06-05 08:53:01 PDT

Created attachment 341970 [details] Patch

Darin Adler

Comment 7 2018-06-05 09:16:07 PDT

This quite small patch should be straightforward and correct now.