Use a local strong NSError for methods with nested autorelease pool

[shoumikhin]

Otherwise, the out NSError arg get assigned an autoreleasing NSError object that gets deallocated when the nested autorelease pool is drained and no error is reported:

- (BOOL)foo:(NSError **)error {
    if (error) {
        // Creates an autoreleased error object.
        *error = [NSError errorWithDomain:@"DemoDomain" code:42 userInfo:nil];
    }
    return NO;
}

- (BOOL)bar:(NSError **)error {
    @autoreleasepool {
        // foo: returns NO, sets *error to an autoreleased NSError.
        // But as soon as we leave this block, that NSError is released.
        return [self foo:error];
    }
}

- (void)run {
    NSError *error = nil;
    if (![self bar:&error]) {  // Crash here even before entering the if-statement below due to `objc_storeStrong` call on the autoreleased error out arg.
        // Even if we manage to get here by marking the error var as __autoreleasing above,
        // Error now points at freed memory, so we get another crash or garbage.
        NSLog(@"❌ Faulty: error code = %ld", (long)error.code);
    }
}

We introduce a local error var to hold onto the autoreleased NSError created by some other API like foo and use it to initialize the out NSError arg:

- (BOOL)bar:(NSError **)error {
    NSError *localError = nil;
    BOOL ok = NO;

    @autoreleasepool {
        ok = [self foo:&localError];
        // ARC retains localError here when assigning the autoreleased object.
    }

    if (!ok && error) {
        *error = localError;  // Safe - localError survived the pool drain.
    }
    return ok;
}

- (void)run {
    NSError *error = nil;
    if (![self bar:&error]) {
        if (error) {  // Don't forget to still check the error var.
          // Now this reliably logs “42”
          NSLog(@"✅ Fixed: error code = %ld", (long)error.code);
        }
    }
}

Another important observation is that generally it's required to check if the out NSError has been set despite the returned status.

And a more trickier crash happens even before the control reaches error handling. The compiler assumes the error local var in the run method is __strong, so it generates objc_storeStrong call. Whereas in fact it points to an __autoreleasing object (that gets deallocated when the pool drains as we clarified), and thus such a call on it will lead to a crash regardless, we won't even reach the code that tries to access code on it. Unless we indeed introduce a local strong var inside bar to be assigned to the out NSError arg and so make the generated objc_storeStrong call a legit one. Although, that issue can technically be resolved differently - by marking error var in run as __autoreleasing to skip the objc_storeStrong call. But that still doesn't help reporting the actual error, since it's gonna be nil after the nested autorelease pool drainage inside bar. And apparently having the nested autorelease pool is important enough, so we can't avoid it and have to use such workarounds.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10465

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit a825597 with merge base 9e59c19 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[shoumikhin]

@cymbalrush ptal
I wonder if there could be other potential issues like that?

[mergennachin]

Can you add a regression test that exercises this path? Namely, a test that will crash before this PR but will succeed with this PR.

[cymbalrush]

@cymbalrush ptal I wonder if there could be other potential issues like that?

Thanks for fixing it! I don't think there are other places where we explicitly create an autoreleasepool

[facebook-github-bot]

@shoumikhin has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[mergennachin]

@shoumikhin @larryliu0820 are there any objections on not creating a test?

[shoumikhin]

@mergennachin sorry, missed that request, adding a test in #10537