Skip to content

Commit

Permalink
Revert "[CameraRoll]Send camera roll setting to the Android device in…
Browse files Browse the repository at this point in the history
… CrosState"

This reverts commit 3895114.

Reason for revert: likely cause of failures
Step "chromeos_components_unittests on Ubuntu-18.04" failing on builder "Linux Chromium OS ASan LSan Tests (1)"

The first run with the relevant failures:
https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20(1)/40730/overview

The following tests consistently fail starting after this first run:
CrosStateSenderTest.NotificationFeatureStateChanged
CrosStateSenderTest.PerformUpdateCrosStateRetrySequence

The first fails with this stack trace:
---
[ RUN      ] CrosStateSenderTest.NotificationFeatureStateChanged
=================================================================
==4136==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60700004b31c at pc 0x5559c16fed11 bp 0x7fffb87b9250 sp 0x7fffb87b9248
READ of size 4 at 0x60700004b31c thread T0
    #0 0x5559c16fed10 in chromeos::multidevice_setup::MultiDeviceSetupClient::GetFeatureState(chromeos::multidevice_setup::mojom::Feature) const ./../../chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.cc:51:44
    #1 0x5559ce6da0ed in chromeos::phonehub::CrosStateSender::PerformUpdateCrosState() ./../../chromeos/components/phonehub/cros_state_sender.cc:93:34
    #2 0x5559ce6d9e1d in chromeos::phonehub::CrosStateSender::AttemptUpdateCrosState() ./../../chromeos/components/phonehub/cros_state_sender.cc:85:3
    #3 0x5559c17102b7 in chromeos::secure_channel::ConnectionManager::NotifyStatusChanged() ./../../chromeos/services/secure_channel/public/cpp/client/connection_manager.cc:23:14
    #4 0x5559ba7314dc in chromeos::phonehub::CrosStateSenderTest_NotificationFeatureStateChanged_Test::TestBody() ./../../chromeos/components/phonehub/cros_state_sender_unittest.cc:146:29
    #5 0x5559baf612a1 in HandleExceptionsInMethodIfSupported<testing::Test, void> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0
    #6 0x5559baf612a1 in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2706:5
    #7 0x5559baf62ca4 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2885:11
    #8 0x5559baf647b3 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3044:30
    #9 0x5559baf87628 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5903:44
    #10 0x5559baf86d49 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0
    #11 0x5559baf86d49 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5470:10
    #12 0x5559c6e8422f in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2492:46
    #13 0x5559c6e8422f in base::TestSuite::Run() ./../../base/test/test_suite.cc:465:16
    #14 0x5559bd07db0e in base::OnceCallback<int ()>::Run() && ./../../base/callback.h:99:12
    #15 0x5559c6e8a853 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, unsigned long, bool, base::OnceCallback<void ()>) ./../../base/test/launcher/unit_test_launcher.cc:177:38
    #16 0x5559c6e8a4d5 in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>, unsigned long) ./../../base/test/launcher/unit_test_launcher.cc:247:10
    #17 0x5559ba482d45 in main ./../../chromeos/components/run_all_unittests.cc:22:10
    #18 0x7f002b751bf6 in __libc_start_main ??:0:0

0x60700004b31c is located 4 bytes to the right of 72-byte region [0x60700004b2d0,0x60700004b318)
allocated by thread T0 here:
    #0 0x5559ba47faed in operator new(unsigned long) /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cpp:95:3
    #1 0x5559c14ad498 in __libcpp_operator_new<unsigned long> ./../../buildtools/third_party/libc++/trunk/include/new:235:10
    #2 0x5559c14ad498 in __libcpp_allocate ./../../buildtools/third_party/libc++/trunk/include/new:261:10
    #3 0x5559c14ad498 in allocate ./../../buildtools/third_party/libc++/trunk/include/__memory/allocator.h:82:38
    #4 0x5559c14ad498 in allocate ./../../buildtools/third_party/libc++/trunk/include/__memory/allocator_traits.h:261:20
    #5 0x5559c14ad498 in std::__1::vector<std::__1::pair<chromeos::multidevice_setup::mojom::Feature, chromeos::multidevice_setup::mojom::FeatureState>, std::__1::allocator<std::__1::pair<chromeos::multidevice_setup::mojom::Feature, chromeos::multidevice_setup::mojom::FeatureState> > >::__vallocate(unsigned long) ./../../buildtools/third_party/libc++/trunk/include/vector:994:37
    #6 0x5559c16fdfe3 in vector<const std::__1::pair<chromeos::multidevice_setup::mojom::Feature, chromeos::multidevice_setup::mojom::FeatureState> *> ./../../buildtools/third_party/libc++/trunk/include/vector:1224:9
    #7 0x5559c16fdfe3 in flat_tree<const std::__1::pair<chromeos::multidevice_setup::mojom::Feature, chromeos::multidevice_setup::mojom::FeatureState> *> ./../../base/containers/flat_tree.h:571:20
    #8 0x5559c16fdfe3 in flat_tree ./../../base/containers/flat_tree.h:595:7
    #9 0x5559c16fdfe3 in flat_tree ./../../base/containers/flat_map.h:211:15
    #10 0x5559c16fdfe3 in chromeos::multidevice_setup::MultiDeviceSetupClient::GenerateDefaultFeatureStatesMap() ./../../chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.cc:21:10
    #11 0x5559d736b57e in chromeos::multidevice_setup::FakeMultiDeviceSetupClient::FakeMultiDeviceSetupClient() ./../../chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.cc:13:27
    #12 0x5559ba733ad7 in make_unique<chromeos::multidevice_setup::FakeMultiDeviceSetupClient> ./../../buildtools/third_party/libc++/trunk/include/__memory/unique_ptr.h:725:32
    #13 0x5559ba733ad7 in chromeos::phonehub::CrosStateSenderTest::SetUp() ./../../chromeos/components/phonehub/cros_state_sender_unittest.cc:40:9
    #14 0x5559baf6118e in HandleExceptionsInMethodIfSupported<testing::Test, void> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0
    #15 0x5559baf6118e in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2701:3
    #16 0x5559baf62ca4 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2885:11
    #17 0x5559baf647b3 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3044:30
    #18 0x5559baf87628 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5903:44
    #19 0x5559baf86d49 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0
    #20 0x5559baf86d49 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5470:10
    #21 0x5559c6e8422f in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2492:46
    #22 0x5559c6e8422f in base::TestSuite::Run() ./../../base/test/test_suite.cc:465:16
    #23 0x5559bd07db0e in base::OnceCallback<int ()>::Run() && ./../../base/callback.h:99:12
    #24 0x5559c6e8a853 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, unsigned long, bool, base::OnceCallback<void ()>) ./../../base/test/launcher/unit_test_launcher.cc:177:38
    #25 0x5559c6e8a4d5 in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>, unsigned long) ./../../base/test/launcher/unit_test_launcher.cc:247:10
    #26 0x5559ba482d45 in main ./../../chromeos/components/run_all_unittests.cc:22:10
    #27 0x7f002b751bf6 in __libc_start_main ??:0:0

SUMMARY: AddressSanitizer: heap-buffer-overflow (/b/s/w/ir/out/Release/chromeos_components_unittests+0x15f40d10)
Shadow bytes around the buggy address:
  0x0c0e80001610: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fa fa
  0x0c0e80001620: fa fa fd fd fd fd fd fd fd fd fd fd fa fa fa fa
  0x0c0e80001630: fd fd fd fd fd fd fd fd fd fd fa fa fa fa fd fd
  0x0c0e80001640: fd fd fd fd fd fd fd fd fa fa fa fa fd fd fd fd
  0x0c0e80001650: fd fd fd fd fd fd fa fa fa fa 00 00 00 00 00 00
=>0x0c0e80001660: 00 00 00[fa]fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c0e80001670: fd fa fa fa fa fa fd fd fd fd fd fd fd fd fd fd
  0x0c0e80001680: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 fa fa
  0x0c0e80001690: fa fa fd fd fd fd fd fd fd fd fd fd fa fa fa fa
  0x0c0e800016a0: fd fd fd fd fd fd fd fd fd fd fa fa fa fa 00 00
  0x0c0e800016b0: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==4136==ABORTING
---

The second fails with this one:
---
[ RUN      ] CrosStateSenderTest.PerformUpdateCrosStateRetrySequence
=================================================================
==3811==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60700005f9ec at pc 0x5579c6175d11 bp 0x7ffd5e6a0e10 sp 0x7ffd5e6a0e08
READ of size 4 at 0x60700005f9ec thread T0
    #0 0x5579c6175d10 in chromeos::multidevice_setup::MultiDeviceSetupClient::GetFeatureState(chromeos::multidevice_setup::mojom::Feature) const ./../../chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.cc:51:44
    #1 0x5579d31510ed in chromeos::phonehub::CrosStateSender::PerformUpdateCrosState() ./../../chromeos/components/phonehub/cros_state_sender.cc:93:34
    #2 0x5579d3150e1d in chromeos::phonehub::CrosStateSender::AttemptUpdateCrosState() ./../../chromeos/components/phonehub/cros_state_sender.cc:85:3
    #3 0x5579c61872b7 in chromeos::secure_channel::ConnectionManager::NotifyStatusChanged() ./../../chromeos/services/secure_channel/public/cpp/client/connection_manager.cc:23:14
    #4 0x5579bf1a3085 in chromeos::phonehub::CrosStateSenderTest_PerformUpdateCrosStateRetrySequence_Test::TestBody() ./../../chromeos/components/phonehub/cros_state_sender_unittest.cc:63:29
    #5 0x5579bf9d82a1 in HandleExceptionsInMethodIfSupported<testing::Test, void> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0
    #6 0x5579bf9d82a1 in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2706:5
    #7 0x5579bf9d9ca4 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2885:11
    #8 0x5579bf9db7b3 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3044:30
    #9 0x5579bf9fe628 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5903:44
    #10 0x5579bf9fdd49 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0
    #11 0x5579bf9fdd49 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5470:10
    #12 0x5579cb8fb22f in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2492:46
    #13 0x5579cb8fb22f in base::TestSuite::Run() ./../../base/test/test_suite.cc:465:16
    #14 0x5579c1af4b0e in base::OnceCallback<int ()>::Run() && ./../../base/callback.h:99:12
    #15 0x5579cb901853 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, unsigned long, bool, base::OnceCallback<void ()>) ./../../base/test/launcher/unit_test_launcher.cc:177:38
    #16 0x5579cb9014d5 in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>, unsigned long) ./../../base/test/launcher/unit_test_launcher.cc:247:10
    #17 0x5579beef9d45 in main ./../../chromeos/components/run_all_unittests.cc:22:10
    #18 0x7fc58b24bbf6 in __libc_start_main ??:0:0

0x60700005f9ec is located 4 bytes to the right of 72-byte region [0x60700005f9a0,0x60700005f9e8)
allocated by thread T0 here:
    #0 0x5579beef6aed in operator new(unsigned long) /b/s/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cpp:95:3
    #1 0x5579c5f24498 in __libcpp_operator_new<unsigned long> ./../../buildtools/third_party/libc++/trunk/include/new:235:10
    #2 0x5579c5f24498 in __libcpp_allocate ./../../buildtools/third_party/libc++/trunk/include/new:261:10
    #3 0x5579c5f24498 in allocate ./../../buildtools/third_party/libc++/trunk/include/__memory/allocator.h:82:38
    #4 0x5579c5f24498 in allocate ./../../buildtools/third_party/libc++/trunk/include/__memory/allocator_traits.h:261:20
    #5 0x5579c5f24498 in std::__1::vector<std::__1::pair<chromeos::multidevice_setup::mojom::Feature, chromeos::multidevice_setup::mojom::FeatureState>, std::__1::allocator<std::__1::pair<chromeos::multidevice_setup::mojom::Feature, chromeos::multidevice_setup::mojom::FeatureState> > >::__vallocate(unsigned long) ./../../buildtools/third_party/libc++/trunk/include/vector:994:37
    #6 0x5579c6174fe3 in vector<const std::__1::pair<chromeos::multidevice_setup::mojom::Feature, chromeos::multidevice_setup::mojom::FeatureState> *> ./../../buildtools/third_party/libc++/trunk/include/vector:1224:9
    #7 0x5579c6174fe3 in flat_tree<const std::__1::pair<chromeos::multidevice_setup::mojom::Feature, chromeos::multidevice_setup::mojom::FeatureState> *> ./../../base/containers/flat_tree.h:571:20
    #8 0x5579c6174fe3 in flat_tree ./../../base/containers/flat_tree.h:595:7
    #9 0x5579c6174fe3 in flat_tree ./../../base/containers/flat_map.h:211:15
    #10 0x5579c6174fe3 in chromeos::multidevice_setup::MultiDeviceSetupClient::GenerateDefaultFeatureStatesMap() ./../../chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.cc:21:10
    #11 0x5579dbde257e in chromeos::multidevice_setup::FakeMultiDeviceSetupClient::FakeMultiDeviceSetupClient() ./../../chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.cc:13:27
    #12 0x5579bf1aaad7 in make_unique<chromeos::multidevice_setup::FakeMultiDeviceSetupClient> ./../../buildtools/third_party/libc++/trunk/include/__memory/unique_ptr.h:725:32
    #13 0x5579bf1aaad7 in chromeos::phonehub::CrosStateSenderTest::SetUp() ./../../chromeos/components/phonehub/cros_state_sender_unittest.cc:40:9
    #14 0x5579bf9d818e in HandleExceptionsInMethodIfSupported<testing::Test, void> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0
    #15 0x5579bf9d818e in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2701:3
    #16 0x5579bf9d9ca4 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2885:11
    #17 0x5579bf9db7b3 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3044:30
    #18 0x5579bf9fe628 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5903:44
    #19 0x5579bf9fdd49 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0
    #20 0x5579bf9fdd49 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5470:10
    #21 0x5579cb8fb22f in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2492:46
    #22 0x5579cb8fb22f in base::TestSuite::Run() ./../../base/test/test_suite.cc:465:16
    #23 0x5579c1af4b0e in base::OnceCallback<int ()>::Run() && ./../../base/callback.h:99:12
    #24 0x5579cb901853 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, unsigned long, bool, base::OnceCallback<void ()>) ./../../base/test/launcher/unit_test_launcher.cc:177:38
    #25 0x5579cb9014d5 in base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>, unsigned long) ./../../base/test/launcher/unit_test_launcher.cc:247:10
    #26 0x5579beef9d45 in main ./../../chromeos/components/run_all_unittests.cc:22:10
    #27 0x7fc58b24bbf6 in __libc_start_main ??:0:0

SUMMARY: AddressSanitizer: heap-buffer-overflow (/b/s/w/ir/out/Release/chromeos_components_unittests+0x15f40d10)
Shadow bytes around the buggy address:
  0x0c0e80003ee0: fd fd fd fd fd fd fd fd fd fd fa fa fa fa fd fd
  0x0c0e80003ef0: fd fd fd fd fd fd fd fd fa fa fa fa fd fd fd fd
  0x0c0e80003f00: fd fd fd fd fd fd fa fa fa fa fd fd fd fd fd fd
  0x0c0e80003f10: fd fd fd fd fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c0e80003f20: fd fd fa fa fa fa fd fd fd fd fd fd fd fd fd fd
=>0x0c0e80003f30: fa fa fa fa 00 00 00 00 00 00 00 00 00[fa]fa fa
  0x0c0e80003f40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e80003f50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e80003f60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e80003f70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0e80003f80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==3811==ABORTING
---

Original change's description:
> [CameraRoll]Send camera roll setting to the Android device in CrosState
>
> Include camera roll setting state as part of CrosState,
> so connected mobile device would be able to get update
> when setting value is toggled.
>
> Change-Id: I04d0ed3872d5adeff5e8f8dc76c6eb6df3a50b9c
> Bug: https://crbug.com/1221297
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173740
> Commit-Queue: Jianbing Wu <jianbing@google.com>
> Auto-Submit: Jianbing Wu <jianbing@google.com>
> Reviewed-by: Jon Mann <jonmann@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#924995}

Bug: https://crbug.com/1221297
Change-Id: Ic87d96786b4244b27b1e284f801df8799911b1fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3184482
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Jon Mann <jonmann@chromium.org>
Reviewed-by: Jon Mann <jonmann@chromium.org>
Cr-Commit-Position: refs/heads/main@{#925118}
  • Loading branch information
Mark Pearson authored and Chromium LUCI CQ committed Sep 27, 2021
1 parent e597f4e commit b15bd9f
Show file tree
Hide file tree
Showing 9 changed files with 19 additions and 58 deletions.
10 changes: 2 additions & 8 deletions chromeos/components/phonehub/cros_state_sender.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,10 @@ void CrosStateSender::PerformUpdateCrosState() {
bool are_notifications_enabled =
multidevice_setup_client_->GetFeatureState(
Feature::kPhoneHubNotifications) == FeatureState::kEnabledByUser;
bool is_camera_roll_enabled =
multidevice_setup_client_->GetFeatureState(
Feature::kPhoneHubCameraRoll) == FeatureState::kEnabledByUser;

PA_LOG(INFO) << "Attempting to send cros state with notifications enabled "
<< "state as: " << are_notifications_enabled
<< " and camera roll enabled state as: "
<< is_camera_roll_enabled;
message_sender_->SendCrosState(are_notifications_enabled,
is_camera_roll_enabled);
<< "state as: " << are_notifications_enabled;
message_sender_->SendCrosState(are_notifications_enabled);

retry_timer_->Start(FROM_HERE, retry_delay_,
base::BindOnce(&CrosStateSender::OnRetryTimerFired,
Expand Down
27 changes: 7 additions & 20 deletions chromeos/components/phonehub/cros_state_sender_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,6 @@ TEST_F(CrosStateSenderTest, UpdatesOnConnected) {
// Set notification feature to be enabled.
fake_multidevice_setup_client_->SetFeatureState(
Feature::kPhoneHubNotifications, FeatureState::kEnabledByUser);
// Set camera roll feature to be enabled.
fake_multidevice_setup_client_->SetFeatureState(Feature::kPhoneHubCameraRoll,
FeatureState::kEnabledByUser);
// Expect no new messages since connection has not been established.
EXPECT_EQ(0u, fake_message_sender_->GetCrosStateCallCount());
EXPECT_FALSE(mock_timer_->IsRunning());
Expand All @@ -123,8 +120,7 @@ TEST_F(CrosStateSenderTest, UpdatesOnConnected) {
// Simulate connected state. Expect a new message to be sent.
fake_connection_manager_->SetStatus(
secure_channel::ConnectionManager::Status::kConnected);
EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().first);
EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().second);
EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount());

// Phone model is populated.
Expand All @@ -135,8 +131,7 @@ TEST_F(CrosStateSenderTest, UpdatesOnConnected) {
// Simulate disconnected state, this should not trigger a new request.
fake_connection_manager_->SetStatus(
secure_channel::ConnectionManager::Status::kDisconnected);
EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().first);
EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().second);
EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount());
EXPECT_FALSE(mock_timer_->IsRunning());
}
Expand All @@ -151,44 +146,36 @@ TEST_F(CrosStateSenderTest, NotificationFeatureStateChanged) {
EXPECT_TRUE(mock_timer_->IsRunning());

// Expect new messages to be sent when connection state is connected.
EXPECT_FALSE(fake_message_sender_->GetRecentCrosState().first);
EXPECT_FALSE(fake_message_sender_->GetRecentCrosState().second);
EXPECT_FALSE(fake_message_sender_->GetRecentCrosState());
EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount());
mock_timer_->Fire();

// Simulate enabling notification feature state and expect cros state to be
// enabled.
fake_multidevice_setup_client_->SetFeatureState(
Feature::kPhoneHubNotifications, FeatureState::kEnabledByUser);
EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().first);
EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
EXPECT_EQ(2u, fake_message_sender_->GetCrosStateCallCount());
mock_timer_->Fire();

// Update a different feature state and expect that it did not affect the
// cros state.
fake_multidevice_setup_client_->SetFeatureState(
Feature::kSmartLock, FeatureState::kDisabledByUser);
EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().first);
EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
EXPECT_EQ(3u, fake_message_sender_->GetCrosStateCallCount());
mock_timer_->Fire();

// Simulate disabling notification feature state and expect cros state to be
// disabled.
fake_multidevice_setup_client_->SetFeatureState(
Feature::kPhoneHubNotifications, FeatureState::kDisabledByUser);
EXPECT_FALSE(fake_message_sender_->GetRecentCrosState().first);
EXPECT_FALSE(fake_message_sender_->GetRecentCrosState());
EXPECT_EQ(4u, fake_message_sender_->GetCrosStateCallCount());

// Simulate enabling camera roll feature state and expect cros state to be
// updated.
fake_multidevice_setup_client_->SetFeatureState(Feature::kPhoneHubCameraRoll,
FeatureState::kEnabledByUser);
EXPECT_TRUE(fake_message_sender_->GetRecentCrosState().second);
EXPECT_EQ(5u, fake_message_sender_->GetCrosStateCallCount());

// Firing the timer does not cause the cros state to be sent again.
mock_timer_->Fire();
EXPECT_EQ(5u, fake_message_sender_->GetCrosStateCallCount());
EXPECT_EQ(4u, fake_message_sender_->GetCrosStateCallCount());
}

} // namespace phonehub
Expand Down
8 changes: 3 additions & 5 deletions chromeos/components/phonehub/fake_message_sender.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,8 @@ namespace phonehub {
FakeMessageSender::FakeMessageSender() = default;
FakeMessageSender::~FakeMessageSender() = default;

void FakeMessageSender::SendCrosState(bool notification_enabled,
bool camera_roll_enabled) {
cros_states_.push_back(
std::make_pair(notification_enabled, camera_roll_enabled));
void FakeMessageSender::SendCrosState(bool notification_enabled) {
cros_states_.push_back(notification_enabled);
}

void FakeMessageSender::SendUpdateNotificationModeRequest(
Expand Down Expand Up @@ -79,7 +77,7 @@ size_t FakeMessageSender::GetFetchCameraRollItemsRequestCallCount() const {
return fetch_camera_roll_items_requests_.size();
}

std::pair<bool, bool> FakeMessageSender::GetRecentCrosState() const {
bool FakeMessageSender::GetRecentCrosState() const {
return cros_states_.back();
}

Expand Down
9 changes: 3 additions & 6 deletions chromeos/components/phonehub/fake_message_sender.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ class FakeMessageSender : public MessageSender {
~FakeMessageSender() override;

// MessageSender:
void SendCrosState(bool notification_enabled,
bool camera_roll_enabled) override;
void SendCrosState(bool notification_enabled) override;
void SendUpdateNotificationModeRequest(bool do_not_disturb_enabled) override;
void SendUpdateBatteryModeRequest(bool battery_saver_mode_enabled) override;
void SendDismissNotificationRequest(int64_t notification_id) override;
Expand All @@ -35,7 +34,7 @@ class FakeMessageSender : public MessageSender {
void SendFetchCameraRollItemsRequest(
const proto::FetchCameraRollItemsRequest& request) override;

std::pair<bool, bool> GetRecentCrosState() const;
bool GetRecentCrosState() const;
bool GetRecentUpdateNotificationModeRequest() const;
bool GetRecentUpdateBatteryModeRequest() const;
int64_t GetRecentDismissNotificationRequest() const;
Expand Down Expand Up @@ -64,9 +63,7 @@ class FakeMessageSender : public MessageSender {
size_t GetFetchCameraRollItemsRequestCallCount() const;

private:
std::vector<std::pair</*is_notifications_setting_enabled*/ bool,
/*is_camera_roll_setting_enabled*/ bool>>
cros_states_;
std::vector<bool> cros_states_;
std::vector<bool> update_notification_mode_requests_;
std::vector<bool> update_battery_mode_requests_;
std::vector<int64_t> dismiss_notification_requests_;
Expand Down
3 changes: 1 addition & 2 deletions chromeos/components/phonehub/message_sender.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ class MessageSender {
virtual ~MessageSender() = default;

// Sends whether the notification setting is enabled in the Chrome OS device.
virtual void SendCrosState(bool notification_setting_enabled,
bool camera_roll_setting_enabled) = 0;
virtual void SendCrosState(bool notification_setting_enabled) = 0;

// Requests that the phone enables or disables Do Not Disturb mode.
virtual void SendUpdateNotificationModeRequest(
Expand Down
7 changes: 1 addition & 6 deletions chromeos/components/phonehub/message_sender_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,13 @@ MessageSenderImpl::MessageSenderImpl(

MessageSenderImpl::~MessageSenderImpl() = default;

void MessageSenderImpl::SendCrosState(bool notification_setting_enabled,
bool camera_roll_setting_enabled) {
void MessageSenderImpl::SendCrosState(bool notification_setting_enabled) {
proto::NotificationSetting is_notification_enabled =
notification_setting_enabled
? proto::NotificationSetting::NOTIFICATIONS_ON
: proto::NotificationSetting::NOTIFICATIONS_OFF;
proto::CameraRollSetting is_camera_roll_enabled =
camera_roll_setting_enabled ? proto::CameraRollSetting::CAMERA_ROLL_ON
: proto::CameraRollSetting::CAMERA_ROLL_OFF;
proto::CrosState request;
request.set_notification_setting(is_notification_enabled);
request.set_camera_roll_setting(is_camera_roll_enabled);

SendMessage(proto::MessageType::PROVIDE_CROS_STATE, &request);
}
Expand Down
3 changes: 1 addition & 2 deletions chromeos/components/phonehub/message_sender_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ class MessageSenderImpl : public MessageSender {
~MessageSenderImpl() override;

// MessageSender:
void SendCrosState(bool notification_setting_enabled,
bool camera_roll_setting_enabled) override;
void SendCrosState(bool notification_setting_enabled) override;
void SendUpdateNotificationModeRequest(bool do_not_disturb_enabled) override;
void SendUpdateBatteryModeRequest(bool battery_saver_mode_enabled) override;
void SendDismissNotificationRequest(int64_t notification_id) override;
Expand Down
4 changes: 1 addition & 3 deletions chromeos/components/phonehub/message_sender_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,8 @@ TEST_F(MessageSenderImplTest, SendCrossState) {
proto::CrosState request;
request.set_notification_setting(
proto::NotificationSetting::NOTIFICATIONS_ON);
request.set_camera_roll_setting(proto::CameraRollSetting::CAMERA_ROLL_OFF);

message_sender_->SendCrosState(/*notification_enabled=*/true,
/*camera_roll_enabled=*/false);
message_sender_->SendCrosState(/*notification_enabled=*/true);
VerifyMessage(proto::MessageType::PROVIDE_CROS_STATE, &request,
fake_connection_manager_->sent_messages().back());
}
Expand Down
6 changes: 0 additions & 6 deletions chromeos/components/phonehub/proto/phonehub_api.proto
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,6 @@ enum NotificationSetting {
NOTIFICATIONS_ON = 1;
}

enum CameraRollSetting {
CAMERA_ROLL_OFF = 0;
CAMERA_ROLL_ON = 1;
}

enum ChargingState {
NOT_CHARGING = 0;
CHARGING_AC = 1;
Expand Down Expand Up @@ -167,7 +162,6 @@ message App {

message CrosState {
NotificationSetting notification_setting = 1;
CameraRollSetting camera_roll_setting = 2;
}

message Action {
Expand Down

0 comments on commit b15bd9f

Please sign in to comment.