[ryanaidilp]

Summary

• Fixes [Bug]: PhonePermissionStrategy.checkServiceStatus calls completionHandler twice, causing crashes #1565
• checkServiceStatus in PhonePermissionStrategy.m called completionHandler(ServiceStatusNotApplicable) inside the if (![app canOpenURL:telURL]) branch without returning, so execution fell through and called completionHandler a second time with the enabled/disabled status.
• This double invocation of the result callback shows up as a top crash in production (via Firebase Crashlytics) on devices where canOpenURL: returns NO for tel:// (e.g. iPad).
• Adds the missing return; so completionHandler is only ever invoked once.

Test plan

• Read through every other *PermissionStrategy.m file under permission_handler_apple to confirm no other strategy has the same missing-return fallthrough pattern (all others either early-return after each handler call, or have a single-statement stub body).
• Manual verification on a device/simulator where canOpenURL: returns NO for tel:// (e.g. iPad) to confirm checkServiceStatus for Permission.phone no longer crashes.

[ryanaidilp]

@mvanbeusekom can you please review this PR? Thanks

[ibrahim-iqbal]

Confirmed the bug on master — PhonePermissionStrategy.m:22-24 invokes completionHandler(ServiceStatusNotApplicable) inside the if (![app canOpenURL:telURL]) branch and then falls straight through to the trailing completionHandler([self canDevicePlaceAPhoneCall] ? ServiceStatusEnabled : ServiceStatusDisabled) on line 25. Two invocations of the same handler for the branch where tel:// is not openable, which is the iPad case in #1565. Adding return; after the first call is the minimal fix and stops the extra canDevicePlaceAPhoneCall CoreTelephony work at the same time.

Cross-checked the neighbouring strategies to back the "no other strategy has the same fall-through" claim from the PR body:

• AppTrackingTransparencyPermissionStrategy.m — every if branch that calls the handler returns.
• AssistantPermissionStrategy.m — single-statement body.
• AudioVideoPermissionStrategy.m, BackgroundRefreshStrategy.m, BluetoothPermissionStrategy.m — same shape, each handler-invoking branch is followed by a return or is the terminal statement.

So this is genuinely the only offender.

One optional readability nudge, take or leave: if (![app canOpenURL:telURL]) { completionHandler(ServiceStatusNotApplicable); return; } else { completionHandler(...); } or an else { ... } around the trailing call would make the "these two are mutually exclusive" contract structural rather than implied by control flow. return; alone works and is the smallest possible diff, which is probably the right trade for a hot-fix on a production crash.

[ryanaidilp]

Confirmed the bug on master — PhonePermissionStrategy.m:22-24 invokes completionHandler(ServiceStatusNotApplicable) inside the if (![app canOpenURL:telURL]) branch and then falls straight through to the trailing completionHandler([self canDevicePlaceAPhoneCall] ? ServiceStatusEnabled : ServiceStatusDisabled) on line 25. Two invocations of the same handler for the branch where tel:// is not openable, which is the iPad case in #1565. Adding return; after the first call is the minimal fix and stops the extra canDevicePlaceAPhoneCall CoreTelephony work at the same time.

Cross-checked the neighbouring strategies to back the "no other strategy has the same fall-through" claim from the PR body:

• AppTrackingTransparencyPermissionStrategy.m — every if branch that calls the handler returns.
• AssistantPermissionStrategy.m — single-statement body.
• AudioVideoPermissionStrategy.m, BackgroundRefreshStrategy.m, BluetoothPermissionStrategy.m — same shape, each handler-invoking branch is followed by a return or is the terminal statement.

So this is genuinely the only offender.

One optional readability nudge, take or leave: if (![app canOpenURL:telURL]) { completionHandler(ServiceStatusNotApplicable); return; } else { completionHandler(...); } or an else { ... } around the trailing call would make the "these two are mutually exclusive" contract structural rather than implied by control flow. return; alone works and is the smallest possible diff, which is probably the right trade for a hot-fix on a production crash.

Good point on readability. I went this route to keep the code changes minimal, but I agree your approach is better. I'll wrap it in an else block and push the update. Thanks

[ibrahim-iqbal]

Confirmed the bug: before this fix, when canOpenURL: fails the method called completionHandler(ServiceStatusNotApplicable) and then fell through unconditionally into the second completionHandler(...) call — a double-invocation of a Flutter method channel result, which the channel enforces exactly-once on (crashes/asserts in debug, silently drops the second result in release). Putting the second call in the else branch is the correct, minimal fix — one-line change, matches the bug description in #1565 exactly. LGTM.

[mvanbeusekom]

Nit: I'd prefer a return statement here to prevent nesting.

[ryanaidilp]

Prefer the same thing actually, that's exactly what I've been doing previously. But as per discussionwith @ibrahim-iqbal, he ask to wrap with else for better readability and consistency across the repo. Hence, I update the implementation to wrap in else block instead of doing early return.

[mvanbeusekom]

I wanted to say, the change looks good, although my personal preference would be to reduce nesting using a return statement.

More important though, could you please update the version in the pubspec.yaml file and leave a note in the CHANGELOG.md with a summary of the bug fix?

[ryanaidilp]

I wanted to say, the change looks good, although my personal preference would be to reduce nesting using a return statement.

More important though, could you please update the version in the pubspec.yaml file and leave a note in the CHANGELOG.md with a summary of the bug fix?

Updated on my latest commit, please check @mvanbeusekom