Maui icon indicating copy to clipboard operation
Maui copied to clipboard

Fix IOS crash on invalid or missing file extension on Resource Media Source

Open ne0rrmatrix opened this issue 9 months ago • 5 comments

  • Bug fix

Description of Change

The primary change made to the code involves enhancing the validation of the path variable. Previously, the code checked if the path was neither null nor consisted solely of whitespace. The update extends this validation by also verifying that the path includes an extension. This adjustment is crucial for ensuring that operations such as retrieving the directory name, the filename without its extension, and the file extension itself are only executed on valid file paths. This modification aims to mitigate potential errors or unexpected outcomes when the code encounters paths that do not correspond to actual files, which would characteristically include extensions.

Summary of Changes:

  • Enhanced validation of the path variable to include a check for the presence of a file extension, in addition to the existing checks for null or whitespace content. This ensures that operations reliant on the path being a file (with an extension) are only performed on suitable paths, thereby improving the robustness of the code against invalid path inputs.

Reference to Code Changes:

  • The condition for checking the path variable has been updated to include a check for a file extension.

Linked Issues

  • Fixes #1856

PR Checklist

  • [x] Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • [ ] Has tests (if omitted, state reason in description)
  • [ ] Has samples (if omitted, state reason in description)
  • [x] Rebased on top of main at time of PR
  • [x] Changes adhere to coding standard
  • [ ] Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

Fixes issue with invalid or missing file extension for video source that caused BSOD on IOS when trying to set source without a file extension.

ne0rrmatrix avatar May 04 '24 02:05 ne0rrmatrix

How do the other platforms behave? Is the result of this PR that the setting of the resource silently fails? If so is that the desired behavior? If we are receiving an invalid source I think an appropriate exception or trace log would be a good option

bijington avatar May 07 '24 04:05 bijington

How do the other platforms behave? Is the result of this PR that the setting of the resource silently fails? If so is that the desired behavior? If we are receiving an invalid source I think an appropriate exception or trace log would be a good option

This fix only applies to IOS and should have zero impact on any other platform. The code is only in the macios branch and should not be included in any other platform. As to how other platform behave I have not tested. I'm sure someone can file a bug report if there is an issue. With this PR I am focused on the issue that was reported and anything else is outside of scope.

Edit: I added a log message on error about invalid file paths if it fails.

ne0rrmatrix avatar May 08 '24 15:05 ne0rrmatrix

This fix only applies to IOS and should have zero impact on any other platform. The code is only in the macios branch and should not be included in any other platform. As to how other platform behave I have not tested. I'm sure someone can file a bug report if there is an issue. With this PR I am focused on the issue that was reported and anything else is outside of scope.

Edit: I added a log message on error about invalid file paths if it fails.

Thanks, my question on how the other platforms behave was not to suggest we fix those also but to make sure that we are being consistent across all platforms. If Android throws an exception then I don't think iOS should silently fail.

I haven't been able to test on Android as my emulators/SDK is messed up so I might be sending us down the wrong path. I just want to make sure that we are considering the implication of the change.

I also don't see an issue with throwing an exception. Now it should be a specific exception providing a suitable message to help developers.

bijington avatar May 08 '24 16:05 bijington

This fix only applies to IOS and should have zero impact on any other platform. The code is only in the macios branch and should not be included in any other platform. As to how other platform behave I have not tested. I'm sure someone can file a bug report if there is an issue. With this PR I am focused on the issue that was reported and anything else is outside of scope. Edit: I added a log message on error about invalid file paths if it fails.

Thanks, my question on how the other platforms behave was not to suggest we fix those also but to make sure that we are being consistent across all platforms. If Android throws an exception then I don't think iOS should silently fail.

I haven't been able to test on Android as my emulators/SDK is messed up so I might be sending us down the wrong path. I just want to make sure that we are considering the implication of the change.

I also don't see an issue with throwing an exception. Now it should be a specific exception providing a suitable message to help developers.

On both windows and android the OS itself outputs an error message saying something about an invalid file. It does nothing else. So with the changes to IOS it will be the same behavior across all platforms. Except tizen which I don't have the ability to test.

ne0rrmatrix avatar May 08 '24 18:05 ne0rrmatrix

Ok great then ignore my previous comments. Thanks for the effort in checking that

bijington avatar May 08 '24 18:05 bijington