Windows ASan browser_tests shards timing out since July 27 |
||||||
Issue descriptionExample before/after builds: https://ci.chromium.org/buildbot/chromium.clang/CrWinAsan/1040 https://ci.chromium.org/buildbot/chromium.clang/CrWinAsan/1041 Each browser_tests shard used to take around 30m, but now they time out after an hour. Many individual tests fail with timeouts as well. I got this LLVM revision blame range from those builds: good: 338127 bad: 338157 There's nothing ASan-specific in there. Other possible culprits: - Chromium change, like enabling DCHECKs, that regresses browser_tests time across the board - Swarming configuration changes - libc++ changes (always_inline changes drastically swinging performance?)
,
Jul 30
I found a use-after-scope report in there somewhere: https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.clang%2FCrWinAsan%2F1041%2F%2B%2Frecipes%2Fsteps%2Fheadless_browsertests_on_Windows-10-15063%2F0%2Flogs%2FHeadlessProtocolCompositorBrowserTest.RendererClientRedirectChainNoJs%2F0 Which relates to temporary initialization, which Richard Smith changed in r338135. So, now ASan is complaining about some use-after-scope stuff that it didn't before. Now we say this isn't kosher: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/html/parser/html_parser_idioms.cc?type=cs&q=EncodingFromMetaAttributes&sq=package:chromium&g=0&l=380 for (const auto& html_attribute : attributes) { const String& attribute_name = html_attribute.first; const String& attribute_value = AtomicString(html_attribute.second); // BUG if (ThreadSafeMatch(attribute_name, http_equivAttr)) { if (DeprecatedEqualIgnoringCase(attribute_value, "content-type")) attribute_value is a reference to a temporary, even though it's const? I'm not yet sure what's up here.
,
Jul 30
One fix in flight: https://chromium-review.googlesource.com/c/chromium/src/+/1155832 This fixes webkit_unit_tests for me, and we'll see how many browser_tests it greens up once it lands and the bots cycle.
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ea9da56a51f0b4ad7da346fb8d2f6e17d811b863 commit ea9da56a51f0b4ad7da346fb8d2f6e17d811b863 Author: Reid Kleckner <rnk@google.com> Date: Tue Jul 31 00:20:54 2018 Fix use-after-scope on reference temporary detected by ASan Consider this code: const String& attribute_value = AtomicString(html_attribute.second); It gets a String member, creates a temporaty AtomicString, and then returns a reference to its String member. The AtomicString object is not lifetime extended. However, if we change the type of the variable to 'const AtomicString&', then it will be lifetime extended, and there is no bug. This is causing the Linux and Windows ASan ToT bots to time out, so I will TBR this. TBR=csharrison@chromium.org BUG= 869067 Change-Id: Ie90c7f07038c4d394b8cba5d03fec4569476ed91 Reviewed-on: https://chromium-review.googlesource.com/1155832 Reviewed-by: Reid Kleckner <rnk@chromium.org> Commit-Queue: Reid Kleckner <rnk@chromium.org> Cr-Commit-Position: refs/heads/master@{#579244} [modify] https://crrev.com/ea9da56a51f0b4ad7da346fb8d2f6e17d811b863/third_party/blink/renderer/core/html/parser/html_parser_idioms.cc
,
Jul 31
Wow, subtle fix :-)
,
Jul 31
This seems to have also fixed browser test failures on the dll bots: https://ci.chromium.org/buildbot/chromium.clang/ToTWin(dll) Started: https://ci.chromium.org/buildbot/chromium.clang/ToTWin%28dll%29/1029, clang r338166 Stopped: https://ci.chromium.org/buildbot/chromium.clang/ToTWin%28dll%29/1042, has https://crrev.com/579244 So, this doesn't just appease ASan, LLVM actually started using this new lifetime information provided by clang to reuse stack space in some cases. > Wow, subtle fix :-) It is, and I probably would've preferred to do something like: AtomicString attribute_value(html_attribute.second); But I'm not sure what the right blink style would be, and I wanted to TBR it, so I went for the minimal change.
,
Jul 31
The roll CL revealed more ASan use-after-scope reports that need to be cleaned up before we can land this. They probably show on the regular ASan ToT bot. For example: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_chromeos_asan_rel_ng/15740 in ash_unittests [ RUN ] PanelLayoutManagerTest.UndockTest ================================================================= ==7624==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7faa8d7f0a78 at pc 0x0000066ccb61 bp 0x7ffc533eb630 sp 0x7ffc533eb628 READ of size 8 at 0x7faa8d7f0a78 thread T0 #0 0x66ccb60 in ash::PersistentWindowController::MaybeRestorePersistentWindowBounds() ash/display/persistent_window_controller.cc:101:66 #1 0x66cd123 in Run base/callback.h:99:12 #2 0x66cd123 in OnDisplayConfigurationChanged ash/display/persistent_window_controller.cc:86 #3 0x66cd123 in non-virtual thunk to ash::PersistentWindowController::OnDisplayConfigurationChanged() ash/display/persistent_window_controller.cc #4 0x66fd95b in ash::WindowTreeHostManager::PostDisplayConfigurationChange() ash/display/window_tree_host_manager.cc:763:14 #5 0xa95a607 in display::DisplayManager::UpdateDisplaysWith(std::__1::vector<display::ManagedDisplayInfo, std::__1::allocator<display::ManagedDisplayInfo> > const&) ui/display/manager/display_manager.cc:1164:16 #6 0xa9502c6 in display::DisplayManager::OnNativeDisplaysChanged(std::__1::vector<display::ManagedDisplayInfo, std::__1::allocator<display::ManagedDisplayInfo> > const&) ui/display/manager/display_manager.cc:933:3 #7 0x1a7f91c in ash::PanelLayoutManagerTest_UndockTest_Test::TestBody() ash/wm/panels/panel_layout_manager_unittest.cc:329:22 #8 0x4532a52 in testing::Test::Run() third_party/googletest/src/googletest/src/gtest.cc #9 0x45349a4 in testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2667:11 #10 0x4535d76 in testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2785:28 #11 0x455b2a6 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:5047:43 #12 0x455a4f5 in testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc #13 0x70a155a in RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2329:46 #14 0x70a155a in base::TestSuite::Run() base/test/test_suite.cc:277 #15 0x70a6ad2 in Run base/callback.h:99:12 #16 0x70a6ad2 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, bool, base::OnceCallback<void ()>) base/test/launcher/unit_test_launcher.cc:225 #17 0x70a6570 in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>) base/test/launcher/unit_test_launcher.cc:576:10 #18 0x176c064 in main ash/test/ash_unittests.cc:37:10 #19 0x7faa90d63f44 in __libc_start_main /build/eglibc-ripdx6/eglibc-2.19/csu/libc-start.c:287 Address 0x7faa8d7f0a78 is located in stack of thread T0 at offset 632 in frame #0 0x66cbedf in ash::PersistentWindowController::MaybeRestorePersistentWindowBounds() ash/display/persistent_window_controller.cc:89 This frame has 9 object(s): [32, 40) 'retval.i' [64, 72) 'ref.tmp.i' [96, 392) 'ref.tmp2.i' [464, 488) 'ref.tmp' (line 95) [528, 576) 'ref.tmp9' (line 97) [608, 656) 'ref.tmp13' (line 100) <== Memory access at offset 632 is inside this variable [688, 856) 'ref.tmp15' (line 102) [928, 944) 'persistent_window_bounds' (line 112) [960, 1256) 'ref.tmp29' (line 118)
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f560b8a25563983797327f69270468ec909bcfc0 commit f560b8a25563983797327f69270468ec909bcfc0 Author: Reid Kleckner <rnk@google.com> Date: Tue Jul 31 21:40:43 2018 Fix use-after-scope in ash window controller The temporary base::Optional<PersistentWindowInfo> object is destroyed at the end of the full expression in this statement: const auto& persistent_window_info = *window_state->persistent_window_info(); Avoid the lifetime issue by making a copy of the small struct. This bug was found by ASan after a clang change to more accurately track the lifetime of temporaries like these. TBR=afakhry@chromium.org Bug: 869067 Change-Id: Ibb4cd6761ec29fc09cd42c75b8d7bb13844f5352 Reviewed-on: https://chromium-review.googlesource.com/1157256 Reviewed-by: Reid Kleckner <rnk@chromium.org> Commit-Queue: Reid Kleckner <rnk@chromium.org> Cr-Commit-Position: refs/heads/master@{#579579} [modify] https://crrev.com/f560b8a25563983797327f69270468ec909bcfc0/ash/display/persistent_window_controller.cc
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b8054333aeb84368429ac3c85c21bf92f2a43e9b commit b8054333aeb84368429ac3c85c21bf92f2a43e9b Author: Reid Kleckner <rnk@google.com> Date: Tue Jul 31 22:03:46 2018 Fix use-after-scope in MouseCursorOverlayControllerBrowserTest In this expression, the reference would be bound to the member of a temporary object returned from GetContainerBounds(): const gfx::Size& view_size = shell()->web_contents()->GetContainerBounds().size(); Clang has become smarter about the lifetimes of temporary objects like these, so ASan now reports this as a use-afer-scope. The fix is to copy the small gfx::Size object. TBR=sergeyu@chromium.org Bug: 869067 Change-Id: Ie15d45d8698d7024c8755980725fdedaa0ccaaad Reviewed-on: https://chromium-review.googlesource.com/1157274 Commit-Queue: Reid Kleckner <rnk@chromium.org> Reviewed-by: Reid Kleckner <rnk@chromium.org> Cr-Commit-Position: refs/heads/master@{#579589} [modify] https://crrev.com/b8054333aeb84368429ac3c85c21bf92f2a43e9b/content/browser/media/capture/mouse_cursor_overlay_controller_browsertest.cc
,
Aug 1
,
Aug 2
We rolled clang and chromium.memory looks good (except for cfi?). I think all the use-after-scope bugs we uncovered are fixed. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by thakis@chromium.org
, Jul 30