New issue
Advanced search Search tips

Issue 887557 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 21
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: ----



Sign in to add a comment

chromeos_unittests failing on chromium.chromiumos/linux-chromeos-rel

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Sep 20

Issue description

Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of liberato@google.com

chromeos_unittests failing on chromium.chromiumos/linux-chromeos-rel

Builders failed on: 
- linux-chromeos-rel: 
  https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel


 
Owner: nohle@chromium.org
https://chromium.googlesource.com/chromium/src/+/d0ffabb7915851ed023809b8738526b78852fd75 is in the regression range.

nohle@, can you please check if it is your change?
Hi liberato@,

Thanks for catching this! This is indeed my fault. I've identified this issue. Implementing the fix now.

Cc: -liberato@google.com
Labels: -Sheriff-Chromium
thanks for taking care of this!

removing sheriff label.
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/abe09b264ab9a1da00779263fa1f4daf78399ab4

commit abe09b264ab9a1da00779263fa1f4daf78399ab4
Author: Josh Nohle <nohle@google.com>
Date: Thu Sep 20 18:59:09 2018

[CrOS MultiDevice] DeviceReenroller: Don't store local device metadata as ref

The helper function
DeviceReenroller::GetSupportedFeaturesForLocalDevice() is currently
storing a local variable as a const ref. This is breaking
chromeos_unittests when the following `gn args` are used:

    ffmpeg_branding = "ChromeOS"
    is_component_build = false
    is_debug = false
    proprietary_codecs = true
    strip_absolute_paths_from_debug_symbols = true
    target_os = "chromeos"
    use_goma = true
    use_vaapi = true

Bug:  887557 
Change-Id: Iaa805f645b5cefc2ab93ca46e595886e5e00e6dc
Tested: chromeos_unittests with gn args listed above
Reviewed-on: https://chromium-review.googlesource.com/1236512
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592891}
[modify] https://crrev.com/abe09b264ab9a1da00779263fa1f4daf78399ab4/chromeos/services/multidevice_setup/device_reenroller.cc

Labels: Merge-Request-70 M-70
This fixes a crash in M-70.
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/074bead6f49634abfa42f9ac73e444502d3e924d

commit 074bead6f49634abfa42f9ac73e444502d3e924d
Author: Josh Nohle <nohle@google.com>
Date: Fri Sep 21 01:09:46 2018

[CrOS MultiDevice] DeviceReenroller: Remove self as observer in dtor

Because DeviceReenroller is a DeviceSyncClient::Observer, it should
remove itself as an observer in the destructor.

Bug:  887557 ,  887758 
Change-Id: I318ee3a3a9f3b7df6cef1bfb362049a6687b9436
Reviewed-on: https://chromium-review.googlesource.com/1237536
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593035}
[modify] https://crrev.com/074bead6f49634abfa42f9ac73e444502d3e924d/chromeos/services/multidevice_setup/device_reenroller.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/135c87489dd68bba667c8b4464084aa089df84c7

commit 135c87489dd68bba667c8b4464084aa089df84c7
Author: Josh Nohle <nohle@google.com>
Date: Fri Sep 21 16:37:57 2018

[CrOS MultiDevice] DeviceReenroller: Reorder unit test member variables to fix ASAN build

Because DeviceReenroller now uses the DeviceSyncClient in the destructor
to remove itself as a DeviceSyncClient::Observer, DeviceReenroller needs
to be destroyed before the DeviceSyncClient in the unit tests.

Bug:  887953 ,  887557 ,  887758 
Change-Id: I23e8f150ef08e6ad3d388f4ed86d47123077f71a
Tested: Unit tests with ASAN on
Reviewed-on: https://chromium-review.googlesource.com/1238818
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: James Hawkins <jhawkins@chromium.org>
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593220}
[modify] https://crrev.com/135c87489dd68bba667c8b4464084aa089df84c7/chromeos/services/multidevice_setup/device_reenroller_unittest.cc

Please mark which OS's this is impacting. 
Labels: OS-Chrome
Status: Fixed (was: Available)
Updated OS to ChromeOS.

Verified that the patches fix the chromeos_unittests.
- Latest build on linux-chromeos-rel is green: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel/13680
- chromeos_unittests turned green after the latest patch: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20(1)/29106

Marking as fixed.
Project Member

Comment 10 by sheriffbot@chromium.org, Sep 22

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-70 Merge-Approved-70
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 24

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cfb8e7c3a9b17068e0e45836fbdd4bf9c3d4cf6d

commit cfb8e7c3a9b17068e0e45836fbdd4bf9c3d4cf6d
Author: Kyle Horimoto <khorimoto@google.com>
Date: Mon Sep 24 21:32:11 2018

[CrOS MultiDevice] DeviceReenroller: Don't store local device metadata as ref

The helper function
DeviceReenroller::GetSupportedFeaturesForLocalDevice() is currently
storing a local variable as a const ref. This is breaking
chromeos_unittests when the following `gn args` are used:

    ffmpeg_branding = "ChromeOS"
    is_component_build = false
    is_debug = false
    proprietary_codecs = true
    strip_absolute_paths_from_debug_symbols = true
    target_os = "chromeos"
    use_goma = true
    use_vaapi = true

TBR=nohle@google.com

(cherry picked from commit abe09b264ab9a1da00779263fa1f4daf78399ab4)

Bug:  887557 
Change-Id: Iaa805f645b5cefc2ab93ca46e595886e5e00e6dc
Tested: chromeos_unittests with gn args listed above
Reviewed-on: https://chromium-review.googlesource.com/1236512
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#592891}
Reviewed-on: https://chromium-review.googlesource.com/1241472
Cr-Commit-Position: refs/branch-heads/3538@{#606}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/cfb8e7c3a9b17068e0e45836fbdd4bf9c3d4cf6d/chromeos/services/multidevice_setup/device_reenroller.cc

Labels: Merge-Merged-70-3538
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/cfb8e7c3a9b17068e0e45836fbdd4bf9c3d4cf6d

Commit: cfb8e7c3a9b17068e0e45836fbdd4bf9c3d4cf6d
Author: khorimoto@google.com
Commiter: khorimoto@chromium.org
Date: 2018-09-24 21:32:11 +0000 UTC

[CrOS MultiDevice] DeviceReenroller: Don't store local device metadata as ref

The helper function
DeviceReenroller::GetSupportedFeaturesForLocalDevice() is currently
storing a local variable as a const ref. This is breaking
chromeos_unittests when the following `gn args` are used:

    ffmpeg_branding = "ChromeOS"
    is_component_build = false
    is_debug = false
    proprietary_codecs = true
    strip_absolute_paths_from_debug_symbols = true
    target_os = "chromeos"
    use_goma = true
    use_vaapi = true

TBR=nohle@google.com

(cherry picked from commit abe09b264ab9a1da00779263fa1f4daf78399ab4)

Bug:  887557 
Change-Id: Iaa805f645b5cefc2ab93ca46e595886e5e00e6dc
Tested: chromeos_unittests with gn args listed above
Reviewed-on: https://chromium-review.googlesource.com/1236512
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#592891}
Reviewed-on: https://chromium-review.googlesource.com/1241472
Cr-Commit-Position: refs/branch-heads/3538@{#606}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d3ddae3f6656ad3134b645c9171e96859355d46

commit 9d3ddae3f6656ad3134b645c9171e96859355d46
Author: Kyle Horimoto <khorimoto@google.com>
Date: Mon Sep 24 21:33:13 2018

[CrOS MultiDevice] DeviceReenroller: Remove self as observer in dtor

Because DeviceReenroller is a DeviceSyncClient::Observer, it should
remove itself as an observer in the destructor.

TBR=nohle@google.com

(cherry picked from commit 074bead6f49634abfa42f9ac73e444502d3e924d)

Bug:  887557 ,  887758 
Change-Id: I318ee3a3a9f3b7df6cef1bfb362049a6687b9436
Reviewed-on: https://chromium-review.googlesource.com/1237536
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#593035}
Reviewed-on: https://chromium-review.googlesource.com/1241566
Cr-Commit-Position: refs/branch-heads/3538@{#607}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/9d3ddae3f6656ad3134b645c9171e96859355d46/chromeos/services/multidevice_setup/device_reenroller.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Sep 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dd2cdd6d64c45b598e438def6c561faca7bd1f10

commit dd2cdd6d64c45b598e438def6c561faca7bd1f10
Author: Kyle Horimoto <khorimoto@google.com>
Date: Mon Sep 24 21:34:45 2018

[CrOS MultiDevice] DeviceReenroller: Reorder unit test member variables to fix ASAN build

Because DeviceReenroller now uses the DeviceSyncClient in the destructor
to remove itself as a DeviceSyncClient::Observer, DeviceReenroller needs
to be destroyed before the DeviceSyncClient in the unit tests.

TBR=nohle@google.com

(cherry picked from commit 135c87489dd68bba667c8b4464084aa089df84c7)

Bug:  887953 ,  887557 ,  887758 
Change-Id: I23e8f150ef08e6ad3d388f4ed86d47123077f71a
Tested: Unit tests with ASAN on
Reviewed-on: https://chromium-review.googlesource.com/1238818
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: James Hawkins <jhawkins@chromium.org>
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#593220}
Reviewed-on: https://chromium-review.googlesource.com/1241753
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#609}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/dd2cdd6d64c45b598e438def6c561faca7bd1f10/chromeos/services/multidevice_setup/device_reenroller_unittest.cc

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/9d3ddae3f6656ad3134b645c9171e96859355d46

Commit: 9d3ddae3f6656ad3134b645c9171e96859355d46
Author: khorimoto@google.com
Commiter: khorimoto@chromium.org
Date: 2018-09-24 21:33:13 +0000 UTC

[CrOS MultiDevice] DeviceReenroller: Remove self as observer in dtor

Because DeviceReenroller is a DeviceSyncClient::Observer, it should
remove itself as an observer in the destructor.

TBR=nohle@google.com

(cherry picked from commit 074bead6f49634abfa42f9ac73e444502d3e924d)

Bug:  887557 ,  887758 
Change-Id: I318ee3a3a9f3b7df6cef1bfb362049a6687b9436
Reviewed-on: https://chromium-review.googlesource.com/1237536
Commit-Queue: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#593035}
Reviewed-on: https://chromium-review.googlesource.com/1241566
Cr-Commit-Position: refs/branch-heads/3538@{#607}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}

Sign in to add a comment