Bug 134914 - [Cocoa] _WKRemoteObjectInterface leaks NSString ivar
Summary: [Cocoa] _WKRemoteObjectInterface leaks NSString ivar
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-14 17:24 PDT by Joseph Pecoraro
Modified: 2014-07-15 11:01 PDT (History)
3 users (show)

See Also:

Attachments
[PATCH] Proposed Fix (1.42 KB, patch)
2014-07-14 17:26 PDT, Joseph Pecoraro 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-07-14 17:24:03 PDT 
_WKRemoteObjectInterface copies _identifier in initialization, but doesn't have a -dealloc to release it.

<rdar://problem/17672075>
Comment 1 Joseph Pecoraro 2014-07-14 17:26:26 PDT 
Created attachment 234895 [details]
[PATCH] Proposed Fix

Alternatively this could be an explicit ivar with RetainPtr.
Comment 2 Anders Carlsson 2014-07-14 17:27:28 PDT 
Comment on attachment 234895 [details]
[PATCH] Proposed Fix

Please use a RetainPtr instead.
Comment 3 WebKit Commit Bot 2014-07-14 18:52:53 PDT 
Comment on attachment 234895 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 234895

Committed r171098: <http://trac.webkit.org/changeset/171098>
Comment 4 WebKit Commit Bot 2014-07-14 18:52:56 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 5 Joseph Pecoraro 2014-07-14 19:03:29 PDT 
(In reply to comment #2)
> (From update of attachment 234895 [details])
> Please use a RetainPtr instead.

Heh, you should have cq-. I'll follow-up.
Comment 6 Joseph Pecoraro 2014-07-14 19:04:35 PDT 
(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 234895 [details] [details])
> > Please use a RetainPtr instead.
> 
> Heh, you should have cq-. I'll follow-up.

Oh, I think there was a review collision and the flags got overwritten.
Comment 7 Joseph Pecoraro 2014-07-14 19:18:31 PDT 
(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 234895 [details] [details])
> > Please use a RetainPtr instead.
> 
> Heh, you should have cq-. I'll follow-up.

The identifier property is (as written) atomic, so making an explicit RetainPtr<NSString> ivar and reimplementing the getter atomically seems like unnecessary extra work considering the simplicity of the current patch. Let me know if you feel strongly about this, or perhaps the atomic is unnecessary.
Comment 8 Anders Carlsson 2014-07-15 11:01:32 PDT 
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > (From update of attachment 234895 [details] [details] [details])
> > > Please use a RetainPtr instead.
> > 
> > Heh, you should have cq-. I'll follow-up.
> 
> The identifier property is (as written) atomic, so making an explicit RetainPtr<NSString> ivar and reimplementing the getter atomically seems like unnecessary extra work considering the simplicity of the current patch. Let me know if you feel strongly about this, or perhaps the atomic is unnecessary.

Make it non-atomic instead.