[client] Add test to repro desired lockfile behavior [crashpad/crashpad : main]
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
I thought it was crashpad_is_android || crashpad_is_fuchsia, not Windows too?
Related details
- Ben Hamilton
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Hamilton (Gerrit)
Ben Hamilton added 1 comment![]( "Open in Gerrit")
I thought it was crashpad_is_android || crashpad_is_fuchsia, not Windows too?
Thanks, I misread it. FWIW, Android does support `flock()`, I'm not sure why
we disable it there.
Related details
- Justin Cohen
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 3 comments![]( "Open in Gerrit")
temp_dir_.path().Append(FILE_PATH_LITERAL("settings.__lock__"));This should be based on `settings_path`
temp_dir_.path().Append(FILE_PATH_LITERAL("settings.__lock__"));And `Settings` leaves a constant exposed for this, which you should use.
// `CRASHPAD_FLOCK_ALWAYS_SUPPORTED`, but this test exercises what happens
// when that flag is not properly defined.…it shouldn’t, because the flag should be properly defined!
Does this mean that the test WOULD exercise what WOULD happen if the flag were not properly defined?
Do you really just want something like
```
#if !(BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_FUCHSIA)) && !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
#error bad
#elif (BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_FUCHSIA)) && CRASHPAD_FLOCK_ALWAYS_SUPPORTED
#error also bad
#endif
```
?
Related details
- Ben Hamilton
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Hamilton (Gerrit)
Ben Hamilton added 3 comments![]( "Open in Gerrit")
temp_dir_.path().Append(FILE_PATH_LITERAL("settings.__lock__"));This should be based on `settings_path`
Done
temp_dir_.path().Append(FILE_PATH_LITERAL("settings.__lock__"));And `Settings` leaves a constant exposed for this, which you should use.
Done
// `CRASHPAD_FLOCK_ALWAYS_SUPPORTED`, but this test exercises what happens
// when that flag is not properly defined.…it shouldn’t, because the flag should be properly defined!
Does this mean that the test WOULD exercise what WOULD happen if the flag were not properly defined?
Do you really just want something like
```
#if !(BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_FUCHSIA)) && !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
#error bad
#elif (BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_FUCHSIA)) && CRASHPAD_FLOCK_ALWAYS_SUPPORTED
#error also bad
#endif
```?
For context, this was to reproduce the runtime error from b/430145411.
I wanted to write a test to exercise the desired behavior at runtime: on platforms that support `flock()`, the existence or not of a `.__lock__` file should not impact behavior of the settings library.
When the bug hits, this test fails, because `Initialize()` fails.
The underlying issue was indeed due to the flag not being properly defined in a subset of library source files; an `#error` in a test source file will not catch this at build time.
If you think it's not worth having a runtime test for the behavior, I'm OK with replacing this with an `#error` in the affected library source file, but that seemed a little tautological.
Related details
- Justin Cohen
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
// `CRASHPAD_FLOCK_ALWAYS_SUPPORTED`, but this test exercises what happens
// when that flag is not properly defined.Ben Hamilton…it shouldn’t, because the flag should be properly defined!
Does this mean that the test WOULD exercise what WOULD happen if the flag were not properly defined?
Do you really just want something like
```
#if !(BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_FUCHSIA)) && !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
#error bad
#elif (BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_FUCHSIA)) && CRASHPAD_FLOCK_ALWAYS_SUPPORTED
#error also bad
#endif
```?
For context, this was to reproduce the runtime error from b/430145411.
I wanted to write a test to exercise the desired behavior at runtime: on platforms that support `flock()`, the existence or not of a `.__lock__` file should not impact behavior of the settings library.
When the bug hits, this test fails, because `Initialize()` fails.
The underlying issue was indeed due to the flag not being properly defined in a subset of library source files; an `#error` in a test source file will not catch this at build time.
If you think it's not worth having a runtime test for the behavior, I'm OK with replacing this with an `#error` in the affected library source file, but that seemed a little tautological.
Can you take me through this in a little more detail? Why do platforms that don’t lock with a `.__lock__` file care about the presence of that file? Why, when platforms that were expected to use `flock` instead wound up with a `.__lock__` file, was `Settings` initialization failing?
Related details
- Ben Hamilton
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Hamilton (Gerrit)
Ben Hamilton added 1 comment![]( "Open in Gerrit")
// `CRASHPAD_FLOCK_ALWAYS_SUPPORTED`, but this test exercises what happens
// when that flag is not properly defined.Ben Hamilton…it shouldn’t, because the flag should be properly defined!
Does this mean that the test WOULD exercise what WOULD happen if the flag were not properly defined?
Do you really just want something like
```
#if !(BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_FUCHSIA)) && !CRASHPAD_FLOCK_ALWAYS_SUPPORTED
#error bad
#elif (BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_FUCHSIA)) && CRASHPAD_FLOCK_ALWAYS_SUPPORTED
#error also bad
#endif
```?
Mark MentovaiFor context, this was to reproduce the runtime error from b/430145411.
I wanted to write a test to exercise the desired behavior at runtime: on platforms that support `flock()`, the existence or not of a `.__lock__` file should not impact behavior of the settings library.
When the bug hits, this test fails, because `Initialize()` fails.
The underlying issue was indeed due to the flag not being properly defined in a subset of library source files; an `#error` in a test source file will not catch this at build time.
If you think it's not worth having a runtime test for the behavior, I'm OK with replacing this with an `#error` in the affected library source file, but that seemed a little tautological.
Can you take me through this in a little more detail? Why do platforms that don’t lock with a `.__lock__` file care about the presence of that file? Why, when platforms that were expected to use `flock` instead wound up with a `.__lock__` file, was `Settings` initialization failing?
The root cause of b/430145411 was inconsistent application of the preprocessor define `CRASHPAD_FLOCK_ALWAYS_SUPPORTED` on Apple platforms.
On these platforms, if the process was suspended or terminated while Crashpad had created the `.__lock__` file, the `.__lock__` file would never be cleaned up, and Crashpad would return errors whenever initializing settings or the crash report database.
This turned into a `DCHECK()` in debug builds, and silently dropping all crash reports in release builds.
Currently, all platforms which do not support `CRASHPAD_FLOCK_ALWAYS_SUPPORTED` happen to use `CrashReportDatabaseGeneric`, which contains code to discard lock files after 3 days:
- https://chromium.googlesource.com/crashpad/crashpad/+/refs/heads/main/handler/prune_crash_reports_thread.cc#41
- https://chromium.googlesource.com/crashpad/crashpad/+/refs/heads/main/client/crash_report_database_generic.cc#768
However, Apple platforms do not use `CrashReportDatabaseGeneric`, so they never discard stale lock files.
I wrote this test to reproduce the behavior of trying to open the settings with a `.__lock__` file (which should be ignored outside Android and Fuchsia).
Related details
- Justin Cohen
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
The root cause of b/430145411 was inconsistent application of the preprocessor define `CRASHPAD_FLOCK_ALWAYS_SUPPORTED` on Apple platforms.
Yeah, I get how a build could theoretically be misconfigured to create that situation, but…that’s a really weird thing. A really weird thing to test specifically for, too.
What happened? Crashpad would try to lock on the no-`flock` path, and then unlock on the `flock` path? Did that produce any errors?
Or: you say it was inconsistently applied. Which translation units had that macro defined properly, and which were missing it?
Related details
- Ben Hamilton
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Hamilton (Gerrit)
Ben Hamilton added 1 comment![]( "Open in Gerrit")
It is a weird thing, but unfortunately it happened.
The macro was defined for all translation units except for:
- `client/annotation.cc`
- `client/annotation_list.cc`
- `client/crash_report_database.cc`
- `client/crashpad_info.cc`
- `client/settings.cc`
Outside these files, the preprocessor macro is only checked in one other place:
https://chromium.googlesource.com/crashpad/crashpad/+/refs/heads/main/util/file/file_io_posix.cc#212
The code in `file_io_posix.cc` is only invoked from the code under `client/...`. so effectively, Apple builds were running with the non-`flock` `.__lock__` file management codepath everywhere.
Without the stale lock file cleanup logic from `CrashReportDatabaseGeneric`, whenever a process crashed while the `.__lock__` file existed, subsequent launches would permanently be stuck with a failed crash report database (it would always report that it was locked) and a failed settings file (same).
Related details
- Justin Cohen
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
It is a weird thing, but unfortunately it happened.
The macro was defined for all translation units except for:
- `client/annotation.cc`
- `client/annotation_list.cc`
- `client/crash_report_database.cc`
- `client/crashpad_info.cc`
- `client/settings.cc`
Outside these files, the preprocessor macro is only checked in one other place:
https://chromium.googlesource.com/crashpad/crashpad/+/refs/heads/main/util/file/file_io_posix.cc#212
The code in `file_io_posix.cc` is only invoked from the code under `client/...`. so effectively, Apple builds were running with the non-`flock` `.__lock__` file management codepath everywhere.
Without the stale lock file cleanup logic from `CrashReportDatabaseGeneric`, whenever a process crashed while the `.__lock__` file existed, subsequent launches would permanently be stuck with a failed crash report database (it would always report that it was locked) and a failed settings file (same).
OK, I see.
Perhaps the better test would be to make a platform-appropriate CrashReportDatabase, open its Settings and leak a lock (perhaps by crashing in a child process?), fire off whatever cleanup process we have to reclaim stale locks, and then verify that you can reopen the Settings and retake the lock. That should exercise lock reclamation.
The idea here: if you want a test to cover this, it should be a test that exercises the real-world code that incidentally was misconfigured. It shouldn’t be a contrived test that creates the specific misconfigured situation you observed.
Would a test like that be feasible?
Related details
- Ben Hamilton
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Hamilton (Gerrit)
Ben Hamilton added 1 comment![]( "Open in Gerrit")
On iOS, other than the child process part, it's pretty feasible. I'm happy to revert this change for now since the test did what I needed to to validate the fix.
Related details
- Justin Cohen
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |