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

LibGUI: Fix GDirectoryModel lifetime bug. #622

Merged
merged 1 commit into from Oct 3, 2019

Conversation

@xeons
Copy link
Contributor

commented Oct 2, 2019

Thumbnail generation callbacks were getting called after the class was already being disposed of causing a crash to occur.

Let me know if there's a more elegant way to solve this problem.

This should fix #619.

@@ -132,6 +134,10 @@ bool GDirectoryModel::fetch_thumbnail_for(const Entry& entry)
},

[this, path](auto thumbnail) {
// this class is disposing, we shouldn't attempt anything.
if (m_disposing)

This comment has been minimized.

Copy link
@awesomekling

awesomekling Oct 2, 2019

Member

This is still buggy, since the object the captured this points to may have been deleted entirely and so it's not safe to read from m_disposing.
Perhaps we could make the destructor of GDirectoryView proactively orphan any pending thumbnailing jobs it has scheduled. That way, when they eventually finish, they'll still go into the thumbnail cache, but we'll know not to invoke any callbacks.

This comment has been minimized.

Copy link
@xeons

xeons Oct 2, 2019

Author Contributor

I'm not sure how to go about doing that. Would I need to extend BackgroundAction in some way?

@bugaevc

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

It's not really a threading bug, it's a lifetime issue.

The way you work around this is you make GDirectoryModel weakable and then have the action capture a weak ref to it, then when you want to use it see if it still exists and exit otherwise.

@xeons

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

It's not really a threading bug, it's a lifetime issue.

The way you work around this is you make GDirectoryModel weakable and then have the action capture a weak ref to it, then when you want to use it see if it still exists and exit otherwise.

I'll try doing this when I get home, I think i see what to do.

Thumbnail generation callbacks were getting called after the class was already being destroyed causing a crash to occur.
@xeons xeons force-pushed the xeons:fix-paintbrush-open-crash branch from b28e9f9 to fc8fdc2 Oct 2, 2019
@xeons xeons changed the title LibGUI: Fix GDirectoryModel threading bug. LibGUI: Fix GDirectoryModel lifetime bug. Oct 2, 2019
@xeons

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

Making GDirectoryModel weakable and feeding that into the action worked. I also allow it to still populate the thumbnail cache.

@awesomekling

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Nice!

@awesomekling awesomekling merged commit 17597f4 into SerenityOS:master Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.