And the assertion should be enabled on Windows (looooong pending TODO(willchan)).
FYI, I've been hammering at this on-and-off, WIP @ https://chromium-review.googlesource.com/c/chromium/src/+/1283010
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b6c96e728d627f190f2c6c69bfa1e1766c9471b2 commit b6c96e728d627f190f2c6c69bfa1e1766c9471b2 Author: Gabriel Charette <gab@chromium.org> Date: Fri Nov 16 01:06:46 2018 [Tracing] Remove base::Thread usage from ETWTracingAgent post_task.h is now preferred to creating special purposed base::Threads All this code needs is a SequencedTaskRunner which is allowed to perform file I/O. Furthermore, PlatformThread::Join() and hence Thread::Stop() will be marked as a blocking operation in https://chromium-review.googlesource.com/c/chromium/src/+/1324370 and this code was preventing that (even though in this case the Join() was mostly a fast operation as there were no tasks left by the time it was invoked). R=chiniforooshan@chromium.org Bug: 707362 Change-Id: Iec2f823e762cea2d1d1e775117fd95d871baa574 Reviewed-on: https://chromium-review.googlesource.com/c/1338178 Reviewed-by: Ehsan Chiniforooshan <chiniforooshan@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#608610} [modify] https://crrev.com/b6c96e728d627f190f2c6c69bfa1e1766c9471b2/base/win/event_trace_consumer.h [modify] https://crrev.com/b6c96e728d627f190f2c6c69bfa1e1766c9471b2/content/browser/tracing/etw_tracing_agent_win.cc [modify] https://crrev.com/b6c96e728d627f190f2c6c69bfa1e1766c9471b2/content/browser/tracing/etw_tracing_agent_win.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/43847232a20919639d2f38fcbf248d777d6b8a56 commit 43847232a20919639d2f38fcbf248d777d6b8a56 Author: Gabriel Charette <gab@chromium.org> Date: Fri Nov 16 14:42:38 2018 [ScopedTaskEnvironment] Enable MTA in unit test thread pools This is a prereq for https://chromium-review.googlesource.com/c/chromium/src/+/1338221 without this change, the MTA assertions fail in unit tests even though they are correct in prod. This change matches the browser process' params @ content/browser/startup_helper.cc It has the adverse side-effect however of enabling the MTA in renderer unit tests but the downside there is not as bad as it just means some COM asserts may pass in unit tests where they wouldn't in integration tests or prod. I think that's okay because unit tests are already generally very loose on allowing I/O, waits, etc. Misuse will still be caught, but that's just not unit tests' main role. R=fdoray@chromium.org Bug: 708584, 707362 Change-Id: I68c5e41c24396885af43427d09c11e1e84ecea43 Reviewed-on: https://chromium-review.googlesource.com/c/1338305 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#608772} [modify] https://crrev.com/43847232a20919639d2f38fcbf248d777d6b8a56/base/test/scoped_task_environment.cc [modify] https://crrev.com/43847232a20919639d2f38fcbf248d777d6b8a56/base/test/scoped_task_environment_unittest.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06d99aeb0b006b51a9558c6ff90d770cc6ad7c74 commit 06d99aeb0b006b51a9558c6ff90d770cc6ad7c74 Author: Gabriel Charette <gab@chromium.org> Date: Fri Nov 16 17:25:09 2018 [TaskbarIconFinder] Replace usage of base::Thread by post_task.h post_task.h is now preferred over using dedicated base::Threads. Furthermore, this code is preventing PlatformThread::Join() called by Thread::Stop()) from being marked as a blocking operation in https://chromium-review.googlesource.com/c/chromium/src/+/1324370. This code was really only spinning up a thread to run a single task in an MTA environment. All threads in the TaskScheduler (in the browser process) are in an MTA. R=grt@chromium.org Bug: 707362 Change-Id: I5defbaeefc291f4b417c142cda60918a4cfd471d Reviewed-on: https://chromium-review.googlesource.com/c/1338221 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Greg Thompson <grt@chromium.org> Cr-Commit-Position: refs/heads/master@{#608829} [modify] https://crrev.com/06d99aeb0b006b51a9558c6ff90d770cc6ad7c74/chrome/browser/win/taskbar_icon_finder.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/48f85308d37e78509355dae303b459c6b29b2122 commit 48f85308d37e78509355dae303b459c6b29b2122 Author: Gabriel Charette <gab@chromium.org> Date: Fri Nov 16 23:04:53 2018 [PlatformThread] Mark PlatformThread::Join as blocking on Windows as well. Aligning with the POSIX impl in preparation for https://chromium-review.googlesource.com/c/chromium/src/+/1283010 Also adds two trivial ScopedAllowBaseSyncPrimitivesForTesting around existing thread joins in tests (more advanced changes were extracted to precursor CLs). Bug: 707362 Change-Id: If4c4c49b4a20554518ca4a25ae1ea16b84d3a510 Reviewed-on: https://chromium-review.googlesource.com/c/1324370 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Reviewed-by: kylechar <kylechar@chromium.org> Reviewed-by: Ryan Hamilton <rch@chromium.org> Cr-Commit-Position: refs/heads/master@{#609013} [modify] https://crrev.com/48f85308d37e78509355dae303b459c6b29b2122/base/threading/platform_thread_posix.cc [modify] https://crrev.com/48f85308d37e78509355dae303b459c6b29b2122/base/threading/platform_thread_win.cc [modify] https://crrev.com/48f85308d37e78509355dae303b459c6b29b2122/chrome/browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc [modify] https://crrev.com/48f85308d37e78509355dae303b459c6b29b2122/net/test/spawned_test_server/local_test_server_win.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/97120bd710f798ee92a93ed48816b07fd6dbed1b commit 97120bd710f798ee92a93ed48816b07fd6dbed1b Author: Gabriel Charette <gab@chromium.org> Date: Wed Dec 19 04:13:16 2018 [PlatformThread] Join() is a wait, update assertions as such. This was using ScopedBlockingCall (which was a 1:1 migration from AssertIOAllowed()) for legacy reasons. CL continued from https://codereview.chromium.org/2790473006/ Some of these allowances aren't desired but this CL merely highlights existing use cases and as such will not block on fixing them. TBR=thestig@chromium.org (trivial side-effects chrome/browser/printing/printer_query.cc) Bug: 707362 Change-Id: I01a9ae13888ab8602369ddf2d12907388bdc908b Reviewed-on: https://chromium-review.googlesource.com/c/1283010 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Reviewed-by: Julien Tinnes <jln@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Toni Baržić <tbarzic@chromium.org> Reviewed-by: Andrey Kosyakov <caseq@chromium.org> Cr-Commit-Position: refs/heads/master@{#617724} [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/base/threading/platform_thread_posix.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/base/threading/platform_thread_win.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/base/threading/thread_restrictions.h [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/chrome/browser/devtools/device/android_device_manager.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/chrome/browser/metrics/thread_watcher.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/chrome/browser/printing/printer_query.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/chrome/browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/chromeos/process_proxy/process_proxy_unittest.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/content/browser/browser_main_loop.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/content/browser/devtools/devtools_http_handler.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/content/browser/sandbox_host_linux.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/net/base/network_config_watcher_mac.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/net/proxy_resolution/multi_threaded_proxy_resolver.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/net/test/embedded_test_server/embedded_test_server.cc [modify] https://crrev.com/97120bd710f798ee92a93ed48816b07fd6dbed1b/net/test/spawned_test_server/local_test_server_win.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9455daee9e580364d5cea224318adc161b4b172 commit a9455daee9e580364d5cea224318adc161b4b172 Author: Gabriel Charette <gab@chromium.org> Date: Wed Dec 19 16:10:56 2018 Revert "[PlatformThread] Join() is a wait, update assertions as such." This reverts commit 97120bd710f798ee92a93ed48816b07fd6dbed1b. Reason for revert: Failures on an optional bot : crbug.com/916544 Original change's description: > [PlatformThread] Join() is a wait, update assertions as such. > > This was using ScopedBlockingCall (which was a 1:1 migration from > AssertIOAllowed()) for legacy reasons. > > CL continued from https://codereview.chromium.org/2790473006/ > > Some of these allowances aren't desired but this CL merely highlights > existing use cases and as such will not block on fixing them. > > TBR=thestig@chromium.org (trivial side-effects chrome/browser/printing/printer_query.cc) > > Bug: 707362 > Change-Id: I01a9ae13888ab8602369ddf2d12907388bdc908b > Reviewed-on: https://chromium-review.googlesource.com/c/1283010 > Commit-Queue: Gabriel Charette <gab@chromium.org> > Reviewed-by: Michael Wasserman <msw@chromium.org> > Reviewed-by: François Doray <fdoray@chromium.org> > Reviewed-by: Julien Tinnes <jln@chromium.org> > Reviewed-by: Matt Menke <mmenke@chromium.org> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > Reviewed-by: Toni Baržić <tbarzic@chromium.org> > Reviewed-by: Andrey Kosyakov <caseq@chromium.org> > Cr-Commit-Position: refs/heads/master@{#617724} TBR=tbarzic@chromium.org,msw@chromium.org,gab@chromium.org,jln@chromium.org,thestig@chromium.org,fdoray@chromium.org,asvitkine@chromium.org,caseq@chromium.org,mmenke@chromium.org Change-Id: I8f9dd61f7b9e2addbb6d78661d1e9ade0d0f9a85 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 707362 , 916544 Reviewed-on: https://chromium-review.googlesource.com/c/1384626 Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#617846} [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/base/threading/platform_thread_posix.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/base/threading/platform_thread_win.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/base/threading/thread_restrictions.h [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/chrome/browser/devtools/device/android_device_manager.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/chrome/browser/metrics/thread_watcher.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/chrome/browser/printing/printer_query.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/chrome/browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/chromeos/process_proxy/process_proxy_unittest.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/content/browser/browser_main_loop.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/content/browser/devtools/devtools_http_handler.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/content/browser/sandbox_host_linux.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/net/base/network_config_watcher_mac.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/net/proxy_resolution/multi_threaded_proxy_resolver.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/net/test/embedded_test_server/embedded_test_server.cc [modify] https://crrev.com/a9455daee9e580364d5cea224318adc161b4b172/net/test/spawned_test_server/local_test_server_win.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/888ce272c797ae83fca6db4b24c76eb897c80ab5 commit 888ce272c797ae83fca6db4b24c76eb897c80ab5 Author: Gabriel Charette <gab@chromium.org> Date: Thu Dec 20 04:34:49 2018 Reland "[PlatformThread] Join() is a wait, update assertions as such." This is a reland of 97120bd710f798ee92a93ed48816b07fd6dbed1b It was reverted @ https://chromium-review.googlesource.com/c/chromium/src/+/1384626 because it caused failures on an optional bot ( crbug.com/916544 ). Realizing my initial CL didn't have full coverage : in this reland I also did a code search for "ScopedAllow(Blocking|IO)\b (Stop|reset)\(\)" and migrated everything I found to ScopedAllowSyncPrimitivesOutsideBlockingScope. Original change's description: > [PlatformThread] Join() is a wait, update assertions as such. > > This was using ScopedBlockingCall (which was a 1:1 migration from > AssertIOAllowed()) for legacy reasons. > > CL continued from https://codereview.chromium.org/2790473006/ > > Some of these allowances aren't desired but this CL merely highlights > existing use cases and as such will not block on fixing them. > > TBR=thestig@chromium.org (trivial side-effects chrome/browser/printing/printer_query.cc) > > Bug: 707362 > Change-Id: I01a9ae13888ab8602369ddf2d12907388bdc908b > Reviewed-on: https://chromium-review.googlesource.com/c/1283010 > Commit-Queue: Gabriel Charette <gab@chromium.org> > Reviewed-by: Michael Wasserman <msw@chromium.org> > Reviewed-by: François Doray <fdoray@chromium.org> > Reviewed-by: Julien Tinnes <jln@chromium.org> > Reviewed-by: Matt Menke <mmenke@chromium.org> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > Reviewed-by: Toni Baržić <tbarzic@chromium.org> > Reviewed-by: Andrey Kosyakov <caseq@chromium.org> > Cr-Commit-Position: refs/heads/master@{#617724} TBR=fdoray@chromium.org (bypassing reviewers from files identical to previous CL) CQ_INCLUDE_TRYBOTS=luci.chromium.try:linux-blink-rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;luci.chromium.try:android_optional_gpu_tests_rel Bug: 707362 Change-Id: Iad5bdb5ba27c7a277dce0072905ddb8c353af881 Reviewed-on: https://chromium-review.googlesource.com/c/1384646 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Sergey Ulanov <sergeyu@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#618109} [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/base/profiler/stack_sampling_profiler.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/base/threading/platform_thread_posix.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/base/threading/platform_thread_win.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/base/threading/thread_restrictions.h [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/chrome/browser/android/vr/vr_shell.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/chrome/browser/devtools/device/android_device_manager.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/chrome/browser/media/webrtc/native_desktop_media_list.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/chrome/browser/metrics/thread_watcher.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/chrome/browser/printing/printer_query.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/chrome/browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/chromeos/process_proxy/process_proxy_unittest.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/content/browser/browser_main_loop.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/content/browser/devtools/devtools_http_handler.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/content/browser/media/capture/desktop_capture_device.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/content/browser/sandbox_host_linux.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/media/audio/audio_input_device.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/media/audio/audio_output_device.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/net/base/network_config_watcher_mac.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/net/proxy_resolution/multi_threaded_proxy_resolver.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/net/proxy_resolution/proxy_resolver_v8_tracing.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/net/test/embedded_test_server/embedded_test_server.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/net/test/spawned_test_server/local_test_server_win.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/remoting/protocol/webrtc_transport.cc [modify] https://crrev.com/888ce272c797ae83fca6db4b24c76eb897c80ab5/services/audio/public/cpp/output_device.cc
Seems to be sticking this time and no related alerts on sheriff-o-matic this morning :)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7d7d33a1a10aac558cc59b0694b423c6cf932b97 commit 7d7d33a1a10aac558cc59b0694b423c6cf932b97 Author: Gabriel Charette <gab@chromium.org> Date: Wed Jan 16 20:59:58 2019 [SandboxHostLinux] Remove destructor It was only deleted at the very end of main when Singletons are reclaimed. Reclaiming resources then is rather pointless as letting the host process die has the same effect (OS will close the socket). The destructor was problematic for https://chromium-review.googlesource.com/c/chromium/src/+/1283010 as calling PlatformThread::Join() now requires an allowance. An explicit allowance was added but perhaps just deleting the destructor is better? Bug: 707362 Change-Id: I41b6479423fff2da64931172acd69c609125104d Reviewed-on: https://chromium-review.googlesource.com/c/1378683 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Cr-Commit-Position: refs/heads/master@{#623353} [modify] https://crrev.com/7d7d33a1a10aac558cc59b0694b423c6cf932b97/content/browser/sandbox_host_linux.cc [modify] https://crrev.com/7d7d33a1a10aac558cc59b0694b423c6cf932b97/content/browser/sandbox_host_linux.h
Comment 1 by gab@chromium.org
, Aug 20Status: Assigned (was: Started)