Skip to content
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.

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

Avoid crash on other Linux due to ref counting issues in Directory cache #2149

Merged
merged 8 commits into from
Mar 6, 2023

Conversation

jeremypw
Copy link
Collaborator

@jeremypw jeremypw commented Mar 3, 2023

Fix #2148 (hopefully)

  • Directory cache now holds a toggle reference to the Directory
  • Move Directory creation related methods together at top of file
  • Ensure directories temporarily cached during creation are removed from cache if necessary
  • Amend test
  • Remove unnecessary null check (cache_lookup should never be called with a null parameter); Amend calling objects to comply
  • Add additional tests for directory_cache operations

A separate critical warning during Directory destruction is also fixed.

It was noticed that the creation_key property may no longer be required after ef93c74. The creation_key should be the same as the location now as it is always a folder and the location should no longer change during initialisation.

However, it was retained for now in case there is a corner case that needs it.

@bobby285271
Copy link
Member

Yeah I can no longer reproduce the crash with this branch 🥳

@jeremypw jeremypw marked this pull request as ready for review March 3, 2023 16:36
libcore/Directory.vala Outdated Show resolved Hide resolved
@jeremypw jeremypw marked this pull request as draft March 3, 2023 16:47
@jeremypw
Copy link
Collaborator Author

jeremypw commented Mar 3, 2023

Just checking that all calls to cache_lookup can be guaranteed non-null.

@jeremypw
Copy link
Collaborator Author

jeremypw commented Mar 3, 2023

Hmm maybe not. Although the property directory of Files.File is defined as non-nullable, such an object could be pointing to e.g. network:/// in which case the parent directory must be null. I'll have to put that null check in again and amend the property instead.

@jeremypw jeremypw marked this pull request as ready for review March 3, 2023 17:58
@jeremypw
Copy link
Collaborator Author

jeremypw commented Mar 3, 2023

I did manage to get a warning about the creation_key differing from the location when connected to a remote public WebDAV server but then I could not reproduce it. The code should in any case be robust in this situation.

Copy link
Member

@bobby285271 bobby285271 left a comment

Choose a reason for hiding this comment

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

Fixes the issue

@jeremypw jeremypw enabled auto-merge (squash) March 6, 2023 09:34
@jeremypw jeremypw merged commit 6a0d16e into master Mar 6, 2023
@jeremypw jeremypw deleted the fix-criticals branch March 6, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when typing path in header bar
3 participants