Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix _ASAsyncTransaction initialization method #1656

Conversation

hanton
Copy link
Contributor

@hanton hanton commented Sep 2, 2019

According to Apple, the superclass (super) initializer need to be Invoked first.

@@ -333,7 +333,7 @@ @implementation _ASAsyncTransaction

- (instancetype)initWithCompletionBlock:(void(^)(_ASAsyncTransaction *, BOOL))completionBlock
{
if ((self = [self init])) {
if ((self = [super init])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised there isn't a compiler warning enabled for this. Is it possible to enable the compiler warning as well?

Copy link
Contributor Author

@hanton hanton Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bolsinga do you mean the Violation of 'self = [super init]' Rule (CLANG_ANALYZER_OBJC_SELF_INIT) setting? It seems already enable in the Build Settings :p

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that warning catch this issue? I don't fully understand.

I like this change; my hope is just get the builds to fail if something like this bug should happen again. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a further investigation at Clang Static Analyzer's code(ObjCSelfInitChecker) and document(osx.cocoa.SelfInit), I found the build setting of Violation of 'self = [super init]' Rule is not targeted for our issue, instead, it is targeted for something like:

@interface MyObj : NSObject {
  id x;
}
- (id)init;
@end

@implementation MyObj
- (id)init {
  [super init];
  x = 0; // warn: instance variable used while 'self' is not
         // initialized
  return 0;
}
@end
@interface MyObj : NSObject
- (id)init;
@end

@implementation MyObj
- (id)init {
  [super init];
  return self; // warn: returning uninitialized 'self'
}
@end

Back to our code, I think it is OK to use self = [self init] in _ASAsyncTransaction, since the init method isn't overridden here, it's unnecessary to write self = [super init]. However, it is an unsafe way. Take an example, if someone subclass_ASAsyncTransaction, override init and call initWithCompletionBlock: from there, this could lead to endless recursion.

By searching through the codebase, this is the only place that has this bug. I suggest we use code review to prevent PR introducing new bug at the moment, until the compile has a more smart checker to handle it, then we enable it :) What's your opinion?

@rahul-malik rahul-malik merged commit 712efc4 into TextureGroup:master Sep 16, 2019
shamanskyh pushed a commit to shamanskyh/Texture that referenced this pull request Sep 16, 2019
- Invoke the superclass (super) initializer
@hanton hanton deleted the fix-_ASAsyncTransaction-initialization-method branch September 19, 2019 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants