fix(apple): prevent double completionHandler call in PhonePermissionStrategy

[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