New issue
Advanced search Search tips

Issue 636375 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug

Blocking:
issue 636372



Sign in to add a comment

Fix C functions and C++ methods returning retained Cocoa objects

Project Member Reported by stkhapugin@chromium.org, Aug 10 2016

Issue description

There are C functions and C++ methods in Chromium that return retained Cocoa object pointers. 

If such a function is built with MRR, and is called from a file built with ARC, the returned object will leak. 

There are two ways to work around that: 
* mark functions as NS_RETURNS_RETAINED
* modify functions to return an autoreleased object instead. Modify callers accordingly.


 
Status: Started (was: Assigned)
This will require writing a clang-tidy plugin to detect all C function and C++ method definitions returning a Cocoa object, then manually iterate over them and annotate/modify. 

If there is a small number of such functions, I'd prefer modifying them to use autoreleased object to annotating them as NS_RETURNS_RETAINED. However, if there are too many of them, it will be hard to fix all uses by hand, and I will resort to annotations instead.

I checked assembly produced with NS_RETURNS_RETAINED and it looks fine. 
Annotator code for C functions:

void CfuncannotatorCheck::registerMatchers(MatchFinder *Finder) {
  Finder->addMatcher(functionDecl().bind("x"), this);
}

void CfuncannotatorCheck::check(const MatchFinder::MatchResult &Result) {
  // FIXME: Add callback implementation.
  const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("x");
  QualType returnType =MatchedDecl->getReturnType();
  if (!returnType->isObjCObjectPointerType())
    return;
  diag(MatchedDecl->getLocation(), "function %0 is returning a cocoa object")
      << MatchedDecl
      << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "cocoareturner_");
}
There are 678 C functions, returning ObjC object pointers. I need to see if I can automatically detect those returning a retained object. 
Cc: rohitrao@chromium.org
+rohitrao: You asked me on https://chromereviews.googleplex.com/484417013/ how did I derive the list of functions to review.
These functions should be fixed: from being annotated they should be changed to actually returning scoped_nsobjects or autoreleased objects. This will involve fixing all call sites, too. 
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 18 2016

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/ba167c719f70ff7ae9fc3247ff04161d368250d8

commit ba167c719f70ff7ae9fc3247ff04161d368250d8
Author: stkhapugin <stkhapugin@google.com>
Date: Thu Aug 18 12:40:25 2016

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f03171454f9041019b56399a5b6752e559b5429

commit 1f03171454f9041019b56399a5b6752e559b5429
Author: stkhapugin <stkhapugin@chromium.org>
Date: Fri Aug 19 12:08:22 2016

Annotates functions returning retained objects.

Annotates all C functions and C++ methods returning retained Cocoa
objects with NS_RETURNS_RETAINED annotation. This annotation is no-op
without ARC, and with ARC it ensures correct memory management.

More about this annotation:
http://clang-analyzer.llvm.org/annotations.html#attr_ns_returns_retained

More information about the process behind this cl on the bug.

BUG= 636375 

Review-Url: https://codereview.chromium.org/2239773002
Cr-Commit-Position: refs/heads/master@{#413113}

[modify] https://crrev.com/1f03171454f9041019b56399a5b6752e559b5429/components/signin/ios/browser/account_consistency_service.h
[modify] https://crrev.com/1f03171454f9041019b56399a5b6752e559b5429/ios/chrome/browser/signin/gaia_auth_fetcher_ios_private.h
[modify] https://crrev.com/1f03171454f9041019b56399a5b6752e559b5429/ios/public/provider/chrome/browser/chrome_browser_provider.h
[modify] https://crrev.com/1f03171454f9041019b56399a5b6752e559b5429/ios/public/provider/chrome/browser/geolocation_updater_provider.h
[modify] https://crrev.com/1f03171454f9041019b56399a5b6752e559b5429/ios/public/provider/chrome/browser/updatable_resource_provider.h
[modify] https://crrev.com/1f03171454f9041019b56399a5b6752e559b5429/ios/web/public/web_view_creation_util.h
[modify] https://crrev.com/1f03171454f9041019b56399a5b6752e559b5429/ios/web/test/wk_web_view_crash_utils.h
[modify] https://crrev.com/1f03171454f9041019b56399a5b6752e559b5429/ios/web/web_state/web_view_internal_creation_util.h

Components: Internals
Status: Fixed (was: Started)

Sign in to add a comment