New issue
Advanced search Search tips

Issue 869067 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocked on:
issue 867393

Blocking:
issue 866225



Sign in to add a comment

Windows ASan browser_tests shards timing out since July 27

Project Member Reported by r...@chromium.org, Jul 30

Issue description

Example 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?)
 
we don't use libc++ on windows, do we? not even in asan builds afaik.

chromium side range is good #578643 bad #578759 ... derp that's all the changes listed on https://ci.chromium.org/buildbot/chromium.clang/CrWinAsan/1041 of course. Nothing in build/config or base/ that looks interesting.
Blocking: 866225
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.
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.
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Wow, subtle fix :-)
Status: Fixed (was: Assigned)
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.
Status: Assigned (was: Fixed)
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)
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Blockedon: 867393
Status: Fixed (was: Assigned)
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