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.

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

Fix 'is_process_running' returns an incorrect 'true' when process 'pid' exists, but whose process does not belong to the current application. #75

Closed
wants to merge 3 commits into from
Closed

Fix 'is_process_running' returns an incorrect 'true' when process 'pid' exists, but whose process does not belong to the current application. #75

wants to merge 3 commits into from

Conversation

Gasparoken
Copy link
Member

Windows part not solved yet.
Linux not tested yet.

The current fix on all platforms might make aseprite/aseprite#4130 unnecessary.

@Gasparoken Gasparoken force-pushed the fix-recovery branch 6 times, most recently from 2954800 to 82e2792 Compare December 27, 2023 19:05
@Gasparoken Gasparoken force-pushed the fix-recovery branch 5 times, most recently from 0712a27 to 917917f Compare January 12, 2024 18:54
@Gasparoken
Fix 'is_process_running' returns an incorrect 'true' when process 'pi…
bb1d53e 
…d' exists, but whose process does not belong to the current application.

Effect on Aseprite: some recovery sessions do not appear in the Recovery menu.
@Gasparoken
Copy link
Member Author

Ready for review. Linux version without modifications.
This fix is needed to fix aseprite/aseprite#4130.

@Gasparoken Gasparoken mentioned this pull request Jan 14, 2024
Merged
martincapello
martincapello approved these changes Jan 16, 2024
View reviewed changes
Copy link
Member

@martincapello martincapello left a comment

Choose a reason for hiding this comment

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

LGTM! I have added the Linux part.

@Gasparoken
Copy link
Member Author

Gasparoken commented Jan 18, 2024 •
edited
Loading

I changed the approach. @martincapello could you please review it again and give me your opinion on the implementation? Could you try again on Linux?
This approach works if aseprite/aseprite#4266 is implemented.

martincapello
martincapello reviewed Jan 22, 2024
View reviewed changes
Copy link
Member

@martincapello martincapello left a comment

Choose a reason for hiding this comment

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

Looks better than before! I tested this on Linux and it works. However I'm not sure why when compiling it doesn't complain as the github actions do...I left a comment with a possible fix that should make github actions happy.

base/process.h
@@ -16,7 +17,9 @@ namespace base {

pid get_current_process_id();

bool is_process_running(pid pid);
std::string get_process_name(pid pid);
Copy link
Member

@martincapello martincapello Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think you should include <string> to avoid the errors reported by Github actions.

@Gasparoken Gasparoken force-pushed the fix-recovery branch 6 times, most recently from cc0c002 to 943c3a4 Compare January 23, 2024 14:39
@Gasparoken
Copy link
Member Author

@martincapello could you try it one more time?

@Gasparoken Gasparoken requested a review from dacap January 25, 2024 17:33
@Gasparoken Gasparoken assigned dacap and unassigned Gasparoken Jan 25, 2024
@Gasparoken
Copy link
Member Author

@dacap the PR is ready for your review.

dacap
dacap reviewed Feb 6, 2024
View reviewed changes
Copy link
Member

@dacap dacap left a comment

Choose a reason for hiding this comment

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

I've made some comments, the commits are a little messy (including git descriptions), I can take this PR from here refactoring/rebasing/editing all the commits in some way (I have 3 platforms running right now to implement this ASAP).

base/process.cpp
bool running = false;

HANDLE handle = OpenProcess(PROCESS_ALL_ACCESS, TRUE, pid);
HANDLE handle = CreateToolhelp32Snapshot(TH32CS_SNAPALL, 0);
Copy link
Member

Choose a reason for hiding this comment

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

From CreateToolhelp32Snapshot docs it looks like it's recommended to use QueryFullProcessImageNameW to get the process name. Probably we don't need this full snapshot thing (not sure what is the work this function does).

base/process.cpp
wcstombs(buf, pe.szExeFile, 64);
std::string str(buf);
for (char& c : str) {
c = tolower(c);
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't tolower the name, we should return the process name as it's reported and then post-process the name from the Aseprite side (remember that laf should be as independent from Aseprite implementation/details as possible).

base/process.cpp
}
CloseHandle(handle);
}
return "";
Copy link
Member

Choose a reason for hiding this comment

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

Always remember to return an empty string as std::string instead of "" when the return value is std::string:

Suggested change
return "";
return std::string();

base/process.cpp
Comment on lines +102 to +113
bool is_process_running(pid pid, std::string currentProcessName)
{
std::string pidProcessName = get_process_name(pid);
if (pidProcessName == "")
return false;
return pidProcessName == currentProcessName;
}

bool is_process_running(pid pid)
{
return false;
};
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we have lost functionality from the laf side here: 1) the is_process_running does nothing (it should behave exactly as it was working), and 2) the new is_process_running(pid,name) looks like something could be done from the Aseprite side now that we have get_process_name.

@dacap dacap added the wip label Feb 6, 2024
@dacap
Copy link
Member

dacap commented Feb 6, 2024

Merged in:

I'll see if I change the Windows impl later but the returned value should be the same. With this new get_process_name() function I think the original Aseprite bug could be fixed.

@dacap dacap closed this Feb 6, 2024
@dacap dacap removed the wip label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dacap dacap dacap left review comments

@martincapello martincapello martincapello approved these changes

Assignees

@dacap dacap

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Gasparoken @dacap @martincapello