Remove TrackFile class #3761
Merged
Remove TrackFile class #3761
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
...to pepare for removing TrackFile.
|
Please push a branch to the upstream repo so signed macOS builds can be tested for regressions with sandboxing. |
Good idea. This should be the corresponding build actions: https://github.com/mixxxdj/mixxx/actions/runs/718884524 |
|
I encountered this DEBUG_ASSERT when using the Browse library feature: |
I am not able to reproduce this, neither with library tracks nor with external files. A stack trace would be helpful to analyze the circumstances. |
|
I uncollapsed the "Computer" library item, then clicked on "Quick Links", then this happens: |
|
Please check the particular file system item that causes this debug assertion (file/dir/symlink? permission issue?). I still cannot reproduce this. |
|
FYI I didn't even click on a directory, just the "Quick Links" parent item. |
|
Sure. But the browse thread seems to pick something up that is not accessible by its absolute path and returns an empty string instead. |
|
Not sure if the last commit fixes the issues. Those issues were always there, just not handled correctly. |
It does. No DEBUG_ASSERT anymore. When clicking on "Quick Links", I see this warning on the terminal: Clicking on a subitem of "Quick Links" does not cause a warning btw. |
|
Passing an empty |
|
Does someone else want to test this or should I merge? |
We really need more macOS testers (or more frequent releases) to prevent QA issues with sandboxing from piling up and going unnoticed, especially for sweeping changes to the sandbox logic that almost exclusively affect macOS (regardless of whether this did or did not introduce #12137) |
|
A sustainable solution needs to isolate and hide the sandbox handling. Otherwise this remains a moving target. The optional sandbox argument is scattered everywhere. Manual testing is not enough. I wonder why Qt doesn't provide this out-of-the-box and everyone has to reinvent the wheel when writing platform independent code. |
|
Whew, this took a lot of digging. I think I found the issue. The new implementation of 5111af7#diff-5afc6764d555de2d3fd3d5baae821a187ece9cf2d5c2526797cd7f0c7a8cc12aR71-R77 bool Sandbox::canAccess(mixxx::FileInfo* pFileInfo) {
VERIFY_OR_DEBUG_ASSERT(pFileInfo) {
return false;
}
openSecurityToken(pFileInfo, true);
return pFileInfo->isReadable();
}The problem is that if we don't assign the token to a local variable, the token will be deinitialized (and presumably invalidated) before we invoke |
|
I'll open a PR. We should probably place a |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
### TODO[ ] Check if the sandboxing commit could be split into smaller chunks. Although I don't expect that it is worth the effort.DONE
TrackFilewithmixxx::FileInfoandmixxx::FileAccess. The latter includes a sandboxing token.Sandbox: Create a sandboxing token from eithermixxx::FileInfoorQDir. I preferred not to remove the existingQDirmember functions in favor ofmixxx::FileInfo(that could represent both, i.e. files and directories).