[task] Replace SequenceManagerForTest in blink fuzzer ThreadManager [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 4 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
4 comments
LGTM
ThreadManager(SequenceManagerFuzzerProcessor* processor);
Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
single-argument constructors must be marked ...
check: google-explicit-constructor
single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel` or `mac-clang-tidy-rel`)
// Note that this doesn't guarantee advancing the thread's clock to |time|.
// The clock is advanced to the minimum desired time of all the owned threads.Outdated comment?
void ThreadPoolManager::AdvanceClockSynchronouslyImpl() {FYI: this synchronization has historically caused a bunch of ephemeral hangs, e.g. crbug.com/398067546, crbug.com/328676397, crbug.com/396459484, etc. I investigated once (https://issues.chromium.org/issues/328676397#comment4) and found a thread created during a synchronization cycle was causing a deadlock. This change looks so much cleaner (!), and I _think_ should resolve that too.
---
Separately, I'm not sure how much value this fuzzer provides has vs. other testing. Of all the bugs that have been filed in the last few years, the only valid ones have been problems with the fuzzer. But it probably doesn't hurt to keep it around, especially if the synchronization issues are fixed..
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 4 comments![]( "Open in Gerrit")
ThreadManager(SequenceManagerFuzzerProcessor* processor);
Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
single-argument constructors must be marked ...
check: google-explicit-constructor
single-argument constructors must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel` or `mac-clang-tidy-rel`)
Done
// Note that this doesn't guarantee advancing the thread's clock to |time|.
// The clock is advanced to the minimum desired time of all the owned threads.Etienne Pierre-DorayOutdated comment?
Fixed.
void ThreadPoolManager::AdvanceClockSynchronouslyImpl() {FYI: this synchronization has historically caused a bunch of ephemeral hangs, e.g. crbug.com/398067546, crbug.com/328676397, crbug.com/396459484, etc. I investigated once (https://issues.chromium.org/issues/328676397#comment4) and found a thread created during a synchronization cycle was causing a deadlock. This change looks so much cleaner (!), and I _think_ should resolve that too.
---
Separately, I'm not sure how much value this fuzzer provides has vs. other testing. Of all the bugs that have been filed in the last few years, the only valid ones have been problems with the fuzzer. But it probably doesn't hurt to keep it around, especially if the synchronization issues are fixed..
I wouldn't be surprised if the former 3 step barrier was the culprit 😊
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/platform/scheduler/test/fuzzer/thread_manager.h
Insertions: 1, Deletions: 1.
@@ -30,7 +30,7 @@
USING_FAST_MALLOC(ThreadManager);
public:
- ThreadManager(SequenceManagerFuzzerProcessor* processor);
+ explicit ThreadManager(SequenceManagerFuzzerProcessor* processor);
~ThreadManager();
```
```
The name of the file: third_party/blink/renderer/platform/scheduler/test/fuzzer/thread_pool_manager.h
Insertions: 2, Deletions: 3.
@@ -45,9 +45,8 @@
bool quit_when_idle_requested) override;
const char* GetName() const override { return "MockTimeDomain"; }
- // Advances the mock tick clock of all the threads synchronously.
- // Note that this doesn't guarantee advancing the thread's clock to |time|.
- // The clock is advanced to the minimum desired time of all the owned threads.
+ // Advances the mock tick clock of all the threads synchronously, until it
+ // reaches `time`.
void AdvanceClockSynchronouslyToTime(base::TimeTicks time);
// Used by a thread to notify the thread manager that it is done executing the
```
Change information
[task] Replace SequenceManagerForTest in blink fuzzer ThreadManager
This CL replaces SequenceManagerForTest::Create() that runs on
top of a mock task runner by a SequenceManager running directly
on the thread's message pump. This removes implicit usage of
deprecated ThreadControllerImpl.
This uses TimeDomain to drive mock time in ThreadPoolManager
(with a lot of simplification).
- M third_party/blink/renderer/platform/scheduler/test/fuzzer/sequence_manager_fuzzer_processor.cc
- M third_party/blink/renderer/platform/scheduler/test/fuzzer/simple_thread_impl.cc
- M third_party/blink/renderer/platform/scheduler/test/fuzzer/simple_thread_impl.h
- M third_party/blink/renderer/platform/scheduler/test/fuzzer/thread_manager.cc
- M third_party/blink/renderer/platform/scheduler/test/fuzzer/thread_manager.h
- M third_party/blink/renderer/platform/scheduler/test/fuzzer/thread_pool_manager.cc
- M third_party/blink/renderer/platform/scheduler/test/fuzzer/thread_pool_manager.h
Code-Review: +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |