New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 761532 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

InitializerImpl async shutdown pattern does not scale

Project Member Reported by khorimoto@chromium.org, Sep 1 2017

Issue description

Currently, we delete some objects during async shutdown starts and delete others when it has finished. We manually delete each individual object, so each time a new object is added to InitializerImpl, we need to remember to add that to the correct portion of the shutdown flow.

This isn't scalable, and it is error-prone since someone adding a new object could forget to add it to the right shutdown area.

Instead, when we create each object, we should associate it with a certain shutdown group and delete the entire group at once.

 

Comment 1 Deleted

Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 13 2017

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

commit 556c2bcd395e267ebd6e51df10073e080c5f2b5c
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 13 14:47:11 2017

[CrOS Tether] Rename Initializer to TetherComponent.

This is the first step in refactoring the Tether component's shutdown
flow, which currently is not scalable as written. Because this class has
evolved and now encompasses more than just initializing the component,
it's renamed to TetherComponent.

The rest of the refactor will be part of a subsequent CL.

Bug:  761532 , 672263
Change-Id: Ic7d8d51a1b96e2572948757ded6fced3b15d75a0
Reviewed-on: https://chromium-review.googlesource.com/716673
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508698}
[modify] https://crrev.com/556c2bcd395e267ebd6e51df10073e080c5f2b5c/chrome/browser/chromeos/tether/tether_service.cc
[modify] https://crrev.com/556c2bcd395e267ebd6e51df10073e080c5f2b5c/chrome/browser/chromeos/tether/tether_service.h
[modify] https://crrev.com/556c2bcd395e267ebd6e51df10073e080c5f2b5c/chrome/browser/chromeos/tether/tether_service_unittest.cc
[modify] https://crrev.com/556c2bcd395e267ebd6e51df10073e080c5f2b5c/chromeos/components/tether/BUILD.gn
[delete] https://crrev.com/ac97fd5c4a120812ddb3673f32357afb2f48098b/chromeos/components/tether/fake_initializer.cc
[delete] https://crrev.com/ac97fd5c4a120812ddb3673f32357afb2f48098b/chromeos/components/tether/fake_initializer.h
[add] https://crrev.com/556c2bcd395e267ebd6e51df10073e080c5f2b5c/chromeos/components/tether/fake_tether_component.cc
[add] https://crrev.com/556c2bcd395e267ebd6e51df10073e080c5f2b5c/chromeos/components/tether/fake_tether_component.h
[delete] https://crrev.com/ac97fd5c4a120812ddb3673f32357afb2f48098b/chromeos/components/tether/initializer.h
[rename] https://crrev.com/556c2bcd395e267ebd6e51df10073e080c5f2b5c/chromeos/components/tether/tether_component.cc
[add] https://crrev.com/556c2bcd395e267ebd6e51df10073e080c5f2b5c/chromeos/components/tether/tether_component.h
[rename] https://crrev.com/556c2bcd395e267ebd6e51df10073e080c5f2b5c/chromeos/components/tether/tether_component_impl.cc
[rename] https://crrev.com/556c2bcd395e267ebd6e51df10073e080c5f2b5c/chromeos/components/tether/tether_component_impl.h
[rename] https://crrev.com/556c2bcd395e267ebd6e51df10073e080c5f2b5c/chromeos/components/tether/tether_component_impl_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 17 2017

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

commit cb6c60f786ffdaffedc49a7d61cc28a3f9c5d085
Author: Kyle Horimoto <khorimoto@google.com>
Date: Tue Oct 17 22:12:58 2017

[CrOS Tether] Add a factory to CrashRecoveryManager.

This will be used to aid testing in a follow-up CL.

Bug:  761532 , 672263
Change-Id: Icd54139447b13f6a8ec1feb6f4345701ff4e115d
Reviewed-on: https://chromium-review.googlesource.com/724080
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509565}
[modify] https://crrev.com/cb6c60f786ffdaffedc49a7d61cc28a3f9c5d085/chromeos/components/tether/crash_recovery_manager.cc
[modify] https://crrev.com/cb6c60f786ffdaffedc49a7d61cc28a3f9c5d085/chromeos/components/tether/crash_recovery_manager.h

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 17 2017

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

commit 7a7f3d91f1169b381a8dfb85df349efa589b2afe
Author: Kyle Horimoto <khorimoto@google.com>
Date: Tue Oct 17 23:47:54 2017

[CrOS Tether] Add a factory to DisconnectTetheringRequestSender.

This will be used to aid testing in a follow-up CL.

Bug:  761532 , 672263
Change-Id: Ifb7b81ab745126aed13abe92e46fe2c77c0bada0
Reviewed-on: https://chromium-review.googlesource.com/724228
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509605}
[modify] https://crrev.com/7a7f3d91f1169b381a8dfb85df349efa589b2afe/chromeos/components/tether/disconnect_tethering_request_sender_impl.cc
[modify] https://crrev.com/7a7f3d91f1169b381a8dfb85df349efa589b2afe/chromeos/components/tether/disconnect_tethering_request_sender_impl.h

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 18 2017

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

commit 25979bd506b19307211c702b69c112b215a74eef
Author: Kyle Horimoto <khorimoto@google.com>
Date: Wed Oct 18 01:59:28 2017

[CrOS Tether] Refactor BleScanner.

Now, BleScanner is a base class with a BleScannerImpl implementation and
a FakeBleScanner test double; additionally, BleScannerImpl has a
factory.

This will be used in a follow-up CL to improve the robustness of the
initialization flow.

Note that some other files were edited slightly due to compiler errors
which resulted from the change.

Bug:  761532 , 672263
Change-Id: I95bf3686dd1074275db670ef2791a38cb75c7805
Reviewed-on: https://chromium-review.googlesource.com/724269
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509652}
[modify] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/BUILD.gn
[modify] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/ble_connection_manager.h
[modify] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/ble_connection_manager_unittest.cc
[modify] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/ble_scanner.cc
[modify] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/ble_scanner.h
[add] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/ble_scanner_impl.cc
[add] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/ble_scanner_impl.h
[rename] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/ble_scanner_impl_unittest.cc
[modify] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/fake_ble_connection_manager.cc
[add] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/fake_ble_scanner.cc
[add] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/fake_ble_scanner.h
[modify] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/host_scanner_operation.cc
[modify] https://crrev.com/25979bd506b19307211c702b69c112b215a74eef/chromeos/components/tether/tether_component_impl.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 18 2017

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

commit 87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5
Author: Kyle Horimoto <khorimoto@google.com>
Date: Wed Oct 18 18:07:33 2017

[CrOS Tether] Refactor BleAdvertiser.

Now, BleAdvertiser is a base class with a BleAdvertiserImpl
implementation and a FakeBleAdvertiser test double; additionally,
BleAdvertiserImpl has a factory.

This will be used in a follow-up CL to improve the robustness of the
initialization flow.

Bug:  761532 , 672263
Change-Id: I99f9dacfb5d6e62fa4580d40126e84d3e6040612
Reviewed-on: https://chromium-review.googlesource.com/724423
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509811}
[modify] https://crrev.com/87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5/chromeos/components/tether/BUILD.gn
[modify] https://crrev.com/87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5/chromeos/components/tether/ble_advertiser.cc
[modify] https://crrev.com/87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5/chromeos/components/tether/ble_advertiser.h
[add] https://crrev.com/87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5/chromeos/components/tether/ble_advertiser_impl.cc
[add] https://crrev.com/87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5/chromeos/components/tether/ble_advertiser_impl.h
[rename] https://crrev.com/87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5/chromeos/components/tether/ble_advertiser_impl_unittest.cc
[modify] https://crrev.com/87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5/chromeos/components/tether/ble_connection_manager_unittest.cc
[add] https://crrev.com/87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5/chromeos/components/tether/fake_ble_advertiser.cc
[add] https://crrev.com/87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5/chromeos/components/tether/fake_ble_advertiser.h
[modify] https://crrev.com/87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5/chromeos/components/tether/tether_component_impl.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 19 2017

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

commit 2c09b844a2147e68d714c488cb8deca2100b9ccb
Author: Kyle Horimoto <khorimoto@google.com>
Date: Thu Oct 19 20:51:21 2017

[CrOS Tether] Refactor CrashRecoveryManager.

Now, CrashRecoveryManager is a pure virtual base class with a
CrashRecoveryManagerImpl implementation and a FakeCrashRecoveryManager
test double.

This will be used in a follow-up CL to improve the robustness of the
initialization flow.

Bug:  761532 , 672263
Change-Id: I104a24e8c580bf7e5b767b7ab0af1aa7cd8f3f3d
Reviewed-on: https://chromium-review.googlesource.com/727184
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510201}
[modify] https://crrev.com/2c09b844a2147e68d714c488cb8deca2100b9ccb/chromeos/components/tether/BUILD.gn
[modify] https://crrev.com/2c09b844a2147e68d714c488cb8deca2100b9ccb/chromeos/components/tether/active_host.h
[modify] https://crrev.com/2c09b844a2147e68d714c488cb8deca2100b9ccb/chromeos/components/tether/crash_recovery_manager.h
[rename] https://crrev.com/2c09b844a2147e68d714c488cb8deca2100b9ccb/chromeos/components/tether/crash_recovery_manager_impl.cc
[add] https://crrev.com/2c09b844a2147e68d714c488cb8deca2100b9ccb/chromeos/components/tether/crash_recovery_manager_impl.h
[rename] https://crrev.com/2c09b844a2147e68d714c488cb8deca2100b9ccb/chromeos/components/tether/crash_recovery_manager_impl_unittest.cc
[add] https://crrev.com/2c09b844a2147e68d714c488cb8deca2100b9ccb/chromeos/components/tether/fake_crash_recovery_manager.cc
[add] https://crrev.com/2c09b844a2147e68d714c488cb8deca2100b9ccb/chromeos/components/tether/fake_crash_recovery_manager.h
[modify] https://crrev.com/2c09b844a2147e68d714c488cb8deca2100b9ccb/chromeos/components/tether/tether_component_impl.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 20 2017

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

commit 4b9ab834cbdc1e6d4ec03a1f855dbba53f0a46f0
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 20 15:55:40 2017

[CrOS Tether] Refactor HostScanScheduler.

Now, HostScanScheduler is a pure virtual base class with a
HostScanSchedulerImpl implementation and a FakeHostScanScheduler
test double.

This will be used in a follow-up CL to improve the robustness of the
initialization flow.

Bug:  761532 , 672263
Change-Id: I75b8d81e56a0a82a9f2c7e29d1aab2d5c38ca4c4
Reviewed-on: https://chromium-review.googlesource.com/727267
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510450}
[modify] https://crrev.com/4b9ab834cbdc1e6d4ec03a1f855dbba53f0a46f0/chromeos/components/tether/BUILD.gn
[add] https://crrev.com/4b9ab834cbdc1e6d4ec03a1f855dbba53f0a46f0/chromeos/components/tether/fake_host_scan_scheduler.cc
[add] https://crrev.com/4b9ab834cbdc1e6d4ec03a1f855dbba53f0a46f0/chromeos/components/tether/fake_host_scan_scheduler.h
[modify] https://crrev.com/4b9ab834cbdc1e6d4ec03a1f855dbba53f0a46f0/chromeos/components/tether/host_scan_scheduler.h
[rename] https://crrev.com/4b9ab834cbdc1e6d4ec03a1f855dbba53f0a46f0/chromeos/components/tether/host_scan_scheduler_impl.cc
[add] https://crrev.com/4b9ab834cbdc1e6d4ec03a1f855dbba53f0a46f0/chromeos/components/tether/host_scan_scheduler_impl.h
[rename] https://crrev.com/4b9ab834cbdc1e6d4ec03a1f855dbba53f0a46f0/chromeos/components/tether/host_scan_scheduler_impl_unittest.cc
[modify] https://crrev.com/4b9ab834cbdc1e6d4ec03a1f855dbba53f0a46f0/chromeos/components/tether/tether_component_impl.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 20 2017

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

commit 61e2b127cd21425344f7841cc75d4e15a6de7658
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 20 18:04:27 2017

[CrOS Tether] Refactor the asynchronous shutdown flow.

Previously, this flow had two major flaws:
(1) It required that each time a top-level object was created, the
    developer had to remember explicitly to delete the object at the
    correct time. This was an easy source of bugs which caused several
    crashes (e.g.,  crbug.com/762318 ).
(2) The implementation was written in such a way that it was impossible
    to unit-test. There was a lot of core code in the initialization and
    shutdown flows which was completely untested, which made it
    difficult to make any edits to the relevant files with any certainty
    that the flow would not break.

This refactor creates two new classes which hold the core objects for
the Tether component: AsynchronousShutdownObjectContainer and
SynchronousShutdownObjectContainer. When the Tether component is to be
shut down, it immediately deletes the synchronous shutdown objects and
asks the asynchronous shutdown objects to begin the shutdown flow; once
that is complete, the entire component is considered shut down.

Bug:  761532 , 672263
Change-Id: I1b000a4147cf7076c1ef7bb1d6781c72a1dc64f9
Reviewed-on: https://chromium-review.googlesource.com/727026
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510489}
[modify] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/BUILD.gn
[add] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/asynchronous_shutdown_object_container.h
[add] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/asynchronous_shutdown_object_container_impl.cc
[add] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/asynchronous_shutdown_object_container_impl.h
[add] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/asynchronous_shutdown_object_container_impl_unittest.cc
[modify] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/crash_recovery_manager_impl.cc
[modify] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/crash_recovery_manager_impl.h
[add] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/fake_asynchronous_shutdown_object_container.cc
[add] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/fake_asynchronous_shutdown_object_container.h
[add] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/fake_synchronous_shutdown_object_container.cc
[add] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/fake_synchronous_shutdown_object_container.h
[add] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/synchronous_shutdown_object_container.h
[add] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/synchronous_shutdown_object_container_impl.cc
[add] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/synchronous_shutdown_object_container_impl.h
[modify] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/tether_component_impl.cc
[modify] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/tether_component_impl.h
[modify] https://crrev.com/61e2b127cd21425344f7841cc75d4e15a6de7658/chromeos/components/tether/tether_component_impl_unittest.cc

Labels: Merge-Request-63
Requesting merge to M-63. This fix must be merged in order to merge the fix for  issue 776241 , which is based on top of this change.
Please add appropriate OSs.
Labels: OS-Chrome
Labels: -Merge-Request-63 Merge-Approved-63
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 20 2017

Labels: -merge-approved-63 merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ffe4ba63c7f0da9cd261d33ef206e346864d42b

commit 8ffe4ba63c7f0da9cd261d33ef206e346864d42b
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 20 21:53:19 2017

[CrOS Tether] Rename Initializer to TetherComponent.

This is the first step in refactoring the Tether component's shutdown
flow, which currently is not scalable as written. Because this class has
evolved and now encompasses more than just initializing the component,
it's renamed to TetherComponent.

The rest of the refactor will be part of a subsequent CL.

TBR=khorimoto@google.com

(cherry picked from commit 556c2bcd395e267ebd6e51df10073e080c5f2b5c)

Bug:  761532 , 672263
Change-Id: Ic7d8d51a1b96e2572948757ded6fced3b15d75a0
Reviewed-on: https://chromium-review.googlesource.com/716673
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#508698}
Reviewed-on: https://chromium-review.googlesource.com/731577
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#116}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/8ffe4ba63c7f0da9cd261d33ef206e346864d42b/chrome/browser/chromeos/tether/tether_service.cc
[modify] https://crrev.com/8ffe4ba63c7f0da9cd261d33ef206e346864d42b/chrome/browser/chromeos/tether/tether_service.h
[modify] https://crrev.com/8ffe4ba63c7f0da9cd261d33ef206e346864d42b/chrome/browser/chromeos/tether/tether_service_unittest.cc
[modify] https://crrev.com/8ffe4ba63c7f0da9cd261d33ef206e346864d42b/chromeos/components/tether/BUILD.gn
[delete] https://crrev.com/fc2f1955dfec5c6a6f42657284c562d79db09ac3/chromeos/components/tether/fake_initializer.cc
[delete] https://crrev.com/fc2f1955dfec5c6a6f42657284c562d79db09ac3/chromeos/components/tether/fake_initializer.h
[add] https://crrev.com/8ffe4ba63c7f0da9cd261d33ef206e346864d42b/chromeos/components/tether/fake_tether_component.cc
[add] https://crrev.com/8ffe4ba63c7f0da9cd261d33ef206e346864d42b/chromeos/components/tether/fake_tether_component.h
[delete] https://crrev.com/fc2f1955dfec5c6a6f42657284c562d79db09ac3/chromeos/components/tether/initializer.h
[rename] https://crrev.com/8ffe4ba63c7f0da9cd261d33ef206e346864d42b/chromeos/components/tether/tether_component.cc
[add] https://crrev.com/8ffe4ba63c7f0da9cd261d33ef206e346864d42b/chromeos/components/tether/tether_component.h
[rename] https://crrev.com/8ffe4ba63c7f0da9cd261d33ef206e346864d42b/chromeos/components/tether/tether_component_impl.cc
[rename] https://crrev.com/8ffe4ba63c7f0da9cd261d33ef206e346864d42b/chromeos/components/tether/tether_component_impl.h
[rename] https://crrev.com/8ffe4ba63c7f0da9cd261d33ef206e346864d42b/chromeos/components/tether/tether_component_impl_unittest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Oct 20 2017

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

commit cd1a4090579fa3521891c360b63d07f02bd3eb96
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 20 21:57:57 2017

[CrOS Tether] Add a factory to CrashRecoveryManager.

This will be used to aid testing in a follow-up CL.

TBR=khorimoto@google.com

(cherry picked from commit cb6c60f786ffdaffedc49a7d61cc28a3f9c5d085)

Bug:  761532 , 672263
Change-Id: Icd54139447b13f6a8ec1feb6f4345701ff4e115d
Reviewed-on: https://chromium-review.googlesource.com/724080
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#509565}
Reviewed-on: https://chromium-review.googlesource.com/731582
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#117}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/cd1a4090579fa3521891c360b63d07f02bd3eb96/chromeos/components/tether/crash_recovery_manager.cc
[modify] https://crrev.com/cd1a4090579fa3521891c360b63d07f02bd3eb96/chromeos/components/tether/crash_recovery_manager.h

Project Member

Comment 17 by bugdroid1@chromium.org, Oct 20 2017

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

commit bc6efb5d17c3e614b498932b56bb7947bdc38a95
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 20 21:59:37 2017

[CrOS Tether] Add a factory to DisconnectTetheringRequestSender.

This will be used to aid testing in a follow-up CL.

TBR=khorimoto@google.com

(cherry picked from commit 7a7f3d91f1169b381a8dfb85df349efa589b2afe)

Bug:  761532 , 672263
Change-Id: Ifb7b81ab745126aed13abe92e46fe2c77c0bada0
Reviewed-on: https://chromium-review.googlesource.com/724228
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#509605}
Reviewed-on: https://chromium-review.googlesource.com/731608
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#118}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/bc6efb5d17c3e614b498932b56bb7947bdc38a95/chromeos/components/tether/disconnect_tethering_request_sender_impl.cc
[modify] https://crrev.com/bc6efb5d17c3e614b498932b56bb7947bdc38a95/chromeos/components/tether/disconnect_tethering_request_sender_impl.h

Project Member

Comment 18 by bugdroid1@chromium.org, Oct 20 2017

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

commit 089bded79c0ad7111f06275eb0256dd85ba1c795
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 20 22:01:12 2017

[CrOS Tether] Refactor BleScanner.

Now, BleScanner is a base class with a BleScannerImpl implementation and
a FakeBleScanner test double; additionally, BleScannerImpl has a
factory.

This will be used in a follow-up CL to improve the robustness of the
initialization flow.

Note that some other files were edited slightly due to compiler errors
which resulted from the change.

TBR=khorimoto@google.com

(cherry picked from commit 25979bd506b19307211c702b69c112b215a74eef)

Bug:  761532 , 672263
Change-Id: I95bf3686dd1074275db670ef2791a38cb75c7805
Reviewed-on: https://chromium-review.googlesource.com/724269
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#509652}
Reviewed-on: https://chromium-review.googlesource.com/731609
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#119}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/BUILD.gn
[modify] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/ble_connection_manager.h
[modify] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/ble_connection_manager_unittest.cc
[modify] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/ble_scanner.cc
[modify] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/ble_scanner.h
[add] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/ble_scanner_impl.cc
[add] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/ble_scanner_impl.h
[rename] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/ble_scanner_impl_unittest.cc
[modify] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/fake_ble_connection_manager.cc
[add] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/fake_ble_scanner.cc
[add] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/fake_ble_scanner.h
[modify] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/host_scanner_operation.cc
[modify] https://crrev.com/089bded79c0ad7111f06275eb0256dd85ba1c795/chromeos/components/tether/tether_component_impl.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Oct 20 2017

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

commit 3b0991256e07d6ae190ae7a7ebb0c5dd73cdb91b
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 20 22:02:35 2017

[CrOS Tether] Refactor BleAdvertiser.

Now, BleAdvertiser is a base class with a BleAdvertiserImpl
implementation and a FakeBleAdvertiser test double; additionally,
BleAdvertiserImpl has a factory.

This will be used in a follow-up CL to improve the robustness of the
initialization flow.

TBR=khorimoto@google.com

(cherry picked from commit 87ae03b67fec9320d4f0d988dfbcf8d7be6f61e5)

Bug:  761532 , 672263
Change-Id: I99f9dacfb5d6e62fa4580d40126e84d3e6040612
Reviewed-on: https://chromium-review.googlesource.com/724423
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#509811}
Reviewed-on: https://chromium-review.googlesource.com/731610
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#120}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/3b0991256e07d6ae190ae7a7ebb0c5dd73cdb91b/chromeos/components/tether/BUILD.gn
[modify] https://crrev.com/3b0991256e07d6ae190ae7a7ebb0c5dd73cdb91b/chromeos/components/tether/ble_advertiser.cc
[modify] https://crrev.com/3b0991256e07d6ae190ae7a7ebb0c5dd73cdb91b/chromeos/components/tether/ble_advertiser.h
[add] https://crrev.com/3b0991256e07d6ae190ae7a7ebb0c5dd73cdb91b/chromeos/components/tether/ble_advertiser_impl.cc
[add] https://crrev.com/3b0991256e07d6ae190ae7a7ebb0c5dd73cdb91b/chromeos/components/tether/ble_advertiser_impl.h
[rename] https://crrev.com/3b0991256e07d6ae190ae7a7ebb0c5dd73cdb91b/chromeos/components/tether/ble_advertiser_impl_unittest.cc
[modify] https://crrev.com/3b0991256e07d6ae190ae7a7ebb0c5dd73cdb91b/chromeos/components/tether/ble_connection_manager_unittest.cc
[add] https://crrev.com/3b0991256e07d6ae190ae7a7ebb0c5dd73cdb91b/chromeos/components/tether/fake_ble_advertiser.cc
[add] https://crrev.com/3b0991256e07d6ae190ae7a7ebb0c5dd73cdb91b/chromeos/components/tether/fake_ble_advertiser.h
[modify] https://crrev.com/3b0991256e07d6ae190ae7a7ebb0c5dd73cdb91b/chromeos/components/tether/tether_component_impl.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Oct 20 2017

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

commit 2569aaf8099cca2946006757322b198dd6997436
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 20 22:05:14 2017

[CrOS Tether] Refactor CrashRecoveryManager.

Now, CrashRecoveryManager is a pure virtual base class with a
CrashRecoveryManagerImpl implementation and a FakeCrashRecoveryManager
test double.

This will be used in a follow-up CL to improve the robustness of the
initialization flow.

TBR=khorimoto@google.com

(cherry picked from commit 2c09b844a2147e68d714c488cb8deca2100b9ccb)

Bug:  761532 , 672263
Change-Id: I104a24e8c580bf7e5b767b7ab0af1aa7cd8f3f3d
Reviewed-on: https://chromium-review.googlesource.com/727184
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#510201}
Reviewed-on: https://chromium-review.googlesource.com/731613
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#121}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/2569aaf8099cca2946006757322b198dd6997436/chromeos/components/tether/BUILD.gn
[modify] https://crrev.com/2569aaf8099cca2946006757322b198dd6997436/chromeos/components/tether/active_host.h
[modify] https://crrev.com/2569aaf8099cca2946006757322b198dd6997436/chromeos/components/tether/crash_recovery_manager.h
[rename] https://crrev.com/2569aaf8099cca2946006757322b198dd6997436/chromeos/components/tether/crash_recovery_manager_impl.cc
[add] https://crrev.com/2569aaf8099cca2946006757322b198dd6997436/chromeos/components/tether/crash_recovery_manager_impl.h
[rename] https://crrev.com/2569aaf8099cca2946006757322b198dd6997436/chromeos/components/tether/crash_recovery_manager_impl_unittest.cc
[add] https://crrev.com/2569aaf8099cca2946006757322b198dd6997436/chromeos/components/tether/fake_crash_recovery_manager.cc
[add] https://crrev.com/2569aaf8099cca2946006757322b198dd6997436/chromeos/components/tether/fake_crash_recovery_manager.h
[modify] https://crrev.com/2569aaf8099cca2946006757322b198dd6997436/chromeos/components/tether/tether_component_impl.cc

Project Member

Comment 21 by bugdroid1@chromium.org, Oct 20 2017

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

commit 315b7cac137dd5b170188bf7cd216ec07de82bff
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 20 22:07:37 2017

[CrOS Tether] Refactor HostScanScheduler.

Now, HostScanScheduler is a pure virtual base class with a
HostScanSchedulerImpl implementation and a FakeHostScanScheduler
test double.

This will be used in a follow-up CL to improve the robustness of the
initialization flow.

TBR=khorimoto@google.com

(cherry picked from commit 4b9ab834cbdc1e6d4ec03a1f855dbba53f0a46f0)

Bug:  761532 , 672263
Change-Id: I75b8d81e56a0a82a9f2c7e29d1aab2d5c38ca4c4
Reviewed-on: https://chromium-review.googlesource.com/727267
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#510450}
Reviewed-on: https://chromium-review.googlesource.com/731618
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#123}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/315b7cac137dd5b170188bf7cd216ec07de82bff/chromeos/components/tether/BUILD.gn
[add] https://crrev.com/315b7cac137dd5b170188bf7cd216ec07de82bff/chromeos/components/tether/fake_host_scan_scheduler.cc
[add] https://crrev.com/315b7cac137dd5b170188bf7cd216ec07de82bff/chromeos/components/tether/fake_host_scan_scheduler.h
[modify] https://crrev.com/315b7cac137dd5b170188bf7cd216ec07de82bff/chromeos/components/tether/host_scan_scheduler.h
[rename] https://crrev.com/315b7cac137dd5b170188bf7cd216ec07de82bff/chromeos/components/tether/host_scan_scheduler_impl.cc
[add] https://crrev.com/315b7cac137dd5b170188bf7cd216ec07de82bff/chromeos/components/tether/host_scan_scheduler_impl.h
[rename] https://crrev.com/315b7cac137dd5b170188bf7cd216ec07de82bff/chromeos/components/tether/host_scan_scheduler_impl_unittest.cc
[modify] https://crrev.com/315b7cac137dd5b170188bf7cd216ec07de82bff/chromeos/components/tether/tether_component_impl.cc

Status: Fixed (was: Started)
Project Member

Comment 23 by bugdroid1@chromium.org, Oct 20 2017

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

commit 4f514f182c4eb524e8fa4788c9c89f1862811a33
Author: Kyle Horimoto <khorimoto@google.com>
Date: Fri Oct 20 22:11:31 2017

[CrOS Tether] Refactor the asynchronous shutdown flow.

Previously, this flow had two major flaws:
(1) It required that each time a top-level object was created, the
    developer had to remember explicitly to delete the object at the
    correct time. This was an easy source of bugs which caused several
    crashes (e.g.,  crbug.com/762318 ).
(2) The implementation was written in such a way that it was impossible
    to unit-test. There was a lot of core code in the initialization and
    shutdown flows which was completely untested, which made it
    difficult to make any edits to the relevant files with any certainty
    that the flow would not break.

This refactor creates two new classes which hold the core objects for
the Tether component: AsynchronousShutdownObjectContainer and
SynchronousShutdownObjectContainer. When the Tether component is to be
shut down, it immediately deletes the synchronous shutdown objects and
asks the asynchronous shutdown objects to begin the shutdown flow; once
that is complete, the entire component is considered shut down.

TBR=khorimoto@google.com

(cherry picked from commit 61e2b127cd21425344f7841cc75d4e15a6de7658)

Bug:  761532 , 672263
Change-Id: I1b000a4147cf7076c1ef7bb1d6781c72a1dc64f9
Reviewed-on: https://chromium-review.googlesource.com/727026
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#510489}
Reviewed-on: https://chromium-review.googlesource.com/731644
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#124}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/BUILD.gn
[add] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/asynchronous_shutdown_object_container.h
[add] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/asynchronous_shutdown_object_container_impl.cc
[add] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/asynchronous_shutdown_object_container_impl.h
[add] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/asynchronous_shutdown_object_container_impl_unittest.cc
[modify] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/crash_recovery_manager_impl.cc
[modify] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/crash_recovery_manager_impl.h
[add] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/fake_asynchronous_shutdown_object_container.cc
[add] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/fake_asynchronous_shutdown_object_container.h
[add] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/fake_synchronous_shutdown_object_container.cc
[add] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/fake_synchronous_shutdown_object_container.h
[add] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/synchronous_shutdown_object_container.h
[add] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/synchronous_shutdown_object_container_impl.cc
[add] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/synchronous_shutdown_object_container_impl.h
[modify] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/tether_component_impl.cc
[modify] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/tether_component_impl.h
[modify] https://crrev.com/4f514f182c4eb524e8fa4788c9c89f1862811a33/chromeos/components/tether/tether_component_impl_unittest.cc

Sign in to add a comment