Skip to content

Fix NSFileHandleOperationException crash in logger, #3590 #3938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 7, 2022
Merged

Fix NSFileHandleOperationException crash in logger, #3590 #3938

merged 2 commits into from
Oct 7, 2022

Conversation

low-batt
Copy link
Contributor

@low-batt low-batt commented Sep 9, 2022

The commit in the pull request will:

  • Add a new Lock class
  • Change Logger to use a lock to coordinate closing of the log file
  • Change Logger to not use Utilities methods that call back into the logger
  • Change Logger to catch and handle exceptions thrown by file operations
  • Remove logDirURL from Utilities
  • Change PrefAdvancedViewController to use Logger.logDirectory instead of Utilities.logDirURL

These changes address multiple ways in which the logger can cause IINA to crash.

Description:
A new proposed fix for this issue that directly addresses the logger crash.

@low-batt
Fix NSFileHandleOperationException crash in logger, iina#3590
464e0f2 
The commit in the pull request will:
- Add a new Lock class
- Change Logger to use a lock to coordinate closing of the log file
- Change Logger to not use `Utilities` methods that call back into
  the logger
- Change Logger to catch and handle exceptions thrown by file
  operations
- Remove logDirURL from Utilities
- Change PrefAdvancedViewController to use Logger.logDirectory
  instead of Utilities.logDirURL

These changes address multiple ways in which the logger can cause
IINA to crash.
This was referenced Sep 9, 2022
Closed
Closed
@low-batt
Copy link
Contributor Author

low-batt commented Sep 9, 2022

The new Lock class in the commit is the one I posted a preview of in the plugin-system PR. There were two comments on that:

Looks reasonable, except I'd make two changes: make the underlying lock type a typealias rather than a generic type, and using deinitialize with no parameters.

I corrected the lock property declaration to use os_unfair_lock_t. I was unable to remove the parameter in the call to deinitialize as that resulted in the compilation error shown below. That version of deinitialize was removed in Swift 5.

/Users/low-batt/Documents/builds/iina/iina/iina/Lock.swift:63:10: error: 'deinitialize()' is unavailable: the default argument to deinitialize(count:) has been removed, please specify the count explicitly
    lock.deinitialize()
         ^~~~~~~~~~~~
Swift.UnsafeMutablePointer:6:17: note: 'deinitialize()' was obsoleted in Swift 5.0

@low-batt low-batt mentioned this pull request Sep 14, 2022
Merged
@low-batt
Copy link
Contributor Author

The new plugin system is triggering issue #3590, so it is desirable to get this PR reviewed and merged. Another reason to get this merged early is that the new Lock class is needed for other fixes.

The fix attempts to insure that the logger will never crash IINA.

The problems with IINA's termination sequence the original PR attempted to address will be fixed for issue #3939. I'm nearly completion on a fix for that. For the moment I'm turning my attention to other issues now that the plugin system has been merged, but will come back to fixing the termination sequence soon.

saagarjha
saagarjha approved these changes Sep 18, 2022
View reviewed changes
iina/Lock.swift Outdated
@available(macOS 10.12, *)
private class OSUnfairLockImpl: LockImpl {

// Use a pointer to insure the lock, which is a struct, is not copied.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Use a pointer to insure the lock, which is a struct, is not copied.
// Use a pointer to ensure the lock, which is a struct, is not copied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed typo.

iina/Lock.swift Outdated
}
}

private class NSLockImpl: LockImpl {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private class NSLockImpl: LockImpl {
private struct NSLockImpl: LockImpl {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed.

@low-batt
Fix NSFileHandleOperationException crash in logger, iina#3590
bd0cf36 
This commit will:
- Change to a struct instead of a class for NSLockImpl
- Cleanup code that locates the user's library directory
- Correct typo in comment

This addresses issues found during review.
@low-batt low-batt mentioned this pull request Sep 27, 2022
Closed
2 tasks
@lhc70000 lhc70000 merged commit 16acefc into iina:develop Oct 7, 2022
@low-batt low-batt linked an issue Oct 17, 2022 that may be closed by this pull request
NSFileHandleOperationException crash in logger during termination #3590
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saagarjha saagarjha saagarjha approved these changes

@uiryuu uiryuu Awaiting requested review from uiryuu

@lhc70000 lhc70000 Awaiting requested review from lhc70000

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NSFileHandleOperationException crash in logger during termination
3 participants
@low-batt @saagarjha @lhc70000