Fix IOS crash on invalid or missing file extension on Resource Media Source #1858
Conversation
… 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.
There was a problem hiding this comment.
Thanks @ne0rrmatrix for adding this validation step. I think it won't hurt and it is pretty straightforward.
|
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 |
…tions are not met, the system logs a warning using `Logger.LogWarning`. The warning message, "Invalid file path for ResourceMediaSource," clearly communicates the nature of the issue to the developer or system administrator, indicating that the path provided cannot be used as intended for a `ResourceMediaSource`. ### List of Changes: - **Warning Logging**: Implemented logging of a warning message using `Logger.LogWarning` for cases where the path is found to be invalid based on the criteria mentioned above.
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. |
|
Ok great then ignore my previous comments. Thanks for the effort in checking that |
Description of Change
The primary change made to the code involves enhancing the validation of the
pathvariable. Previously, the code checked if thepathwas neither null nor consisted solely of whitespace. The update extends this validation by also verifying that thepathincludes 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:
pathvariable 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:
pathvariable has been updated to include a check for a file extension.Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional 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.