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

Error dialog causes process to hang #16849

Open
dktapps opened this issue Nov 18, 2024 · 3 comments · May be fixed by #16850
Open

Error dialog causes process to hang #16849

dktapps opened this issue Nov 18, 2024 · 3 comments · May be fixed by #16850
Assignees
@cmb69
Labels
Bug Category: Engine OS: Windows Status: Verified

Comments

@dktapps
Copy link
Contributor

dktapps commented Nov 18, 2024 •
edited
Loading

Description

php-src/main/main.c

Lines 2121 to 2122 in dd4fc9a

/* Disable the message box for assertions.*/
_CrtSetReportMode(_CRT_ASSERT, 0);

This doesn't appear to produce the desired effect in latest versions of PHP. Dialog asserts are still generated during tests.

From my brief research it doesn't appear that 0 is a valid input for this function.

The better approach would probably be to redirect the output to stderr, as is done here when PHP_WIN32_HEAP_DEBUG is set:

php-src/sapi/cli/php_cli.c

Lines 1181 to 1186 in dd4fc9a

_CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE);
_CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDERR);
_CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE);
_CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDERR);
_CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE);
_CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);

However, setting PHP_WIN32_HEAP_DEBUG enables a bunch of other stuff that isn't necessarily wanted.

This bug is problematic because it causes tests on Windows CIs to get stuck when asserts are triggered in debug builds.

PHP Version

8.1, 8.2, 8.3

Operating System

Windows
@dktapps dktapps mentioned this issue Nov 18, 2024
Closed
@cmb69 cmb69 self-assigned this Nov 18, 2024
@cmb69
Copy link
Member

cmb69 commented Nov 18, 2024

From my brief research it doesn't appear that 0 is a valid input for this function.

Indeed, it is undocumented. If I pass e.g. 16 instead, I get:

Expression: fMode == _CRTDBG_REPORT_MODE || (fMode & ~(_CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG | _CRTDBG_MODE_WNDW)) == 0

Now one might think this is an errenous assertion, but there seems to be special support; from what I can tell it works _CRTDBG_MODE_FILE and _CRTDBG_FILE_STDERR have been set. If I only set _CRTDBG_MODE_FILE, the output goes nowhere since the report file defaults to INVALID_HANDLE_VALUE.

Dialog asserts are still generated during tests.

Not really. The dialogs are about the inevitable error after an assertion failure where abort() will be called.

I think it's probably best to choose a minimal solution as bug fix (i.e. add _CrtSetReportMode(_CRT_ERROR, 0);), and possibly be more fancy as new feature (the dialogs are nifty during testing if a post-mortem debugger is attached).

@cmb69
Copy link
Member

cmb69 commented Nov 18, 2024

However, setting PHP_WIN32_HEAP_DEBUG enables a bunch of other stuff that isn't necessarily wanted.

Well, it's likely the best memory leak detector available for MSVC builds (might find a couple of other memory related issues, but ASan is far superior). Set USE_ZEND_ALLOC=0, USE_TRACKED_ALLOC=1 and PHP_WIN32_DEBUG_HEAP=1 to find leaks when running php.exe (not implemented for other SAPIs).

@cmb69 cmb69 changed the title Windows VC++ Runtime assert dialogs are not actually disabled Error dialog causes process to hang Nov 18, 2024
cmb69 added a commit to cmb69/php-src that referenced this issue Nov 18, 2024
@cmb69
Fix phpGH-16849: Error dialog causes process to hang
6070923 
If `_DEBUG` is set, assertion failures and errors are directed to a
debug message window by default[1].  That causes a process to hang,
since these dialogs are modal.  While we already cater to assertion
failures, errors have apparently been overlooked.

We choose a minimal fix for BC reasons; although passing `0` as
`reportMode` is undocumented, it obviously works fine for a long time.
We may consider to improve on this for the `master` branch.

[1] <https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/crtsetreportmode>
@cmb69 cmb69 linked a pull request Nov 18, 2024 that will close this issue
Fix GH-16849: Error dialog causes process to hang #16850
Open
@dktapps
Copy link
Contributor Author

dktapps commented Nov 18, 2024

Dialog asserts are still generated during tests.

Not really. The dialogs are about the inevitable error after an assertion failure where abort() will be called.

Ah, I hadn't realized there was a difference.

@dktapps dktapps mentioned this issue Nov 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cmb69 cmb69

Labels
Bug Category: Engine OS: Windows Status: Verified
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix GH-16849: Error dialog causes process to hang cmb69/php-src
2 participants
@cmb69 @dktapps