Ensure that connection/disconnection callbacks are invoked during shutdown |
||||||||||||
Issue descriptionWhen the user requests a connection/disconnection, NetworkConnectionHandler passes a success and error callback to the Tether component. If a connection or disconnection is in progress when component shuts down, we do not ensure that the callback is actually invoked. The following situation is possible: (1) UI requests a connection. (2) NetworkConnectionHandler calls into the Tether component and passes success/error callbacks. (3) Tether component is shut down. The class that holds the callback is destroyed. (4) The callback is never invoked. This is an edge case, but is entirely possible if the user disables Mobile data during a connection/disconnection attempt. To prevent this issue, in NetworkConnectionHandlerDelegate, instead of passing callbacks directly to TetherConnector and TetherDisconnector, I will create a wrapper callback and pass that onward, keeping track of the original callbacks internally. Then, when one of the wrapper callbacks is invoked, I will find the original callback and invoke that. To handle the shutdown case described above, in NetworkConnectionHandlerDelegate's destructor, I will check to see if there are any outstanding requests and invoke the error callback on them if they exist.
,
Sep 8 2017
,
Sep 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4c259e0c548b54ee9c7455bc4848d48e2144ed21 commit 4c259e0c548b54ee9c7455bc4848d48e2144ed21 Author: Kyle Horimoto <khorimoto@google.com> Date: Fri Sep 08 22:33:06 2017 [CrOS Tether] Invoke any pending connection callbacks on shutdown. This ensures that there is no possibility for pending network connection or disconnection requests when shutdown occurs. Leaving these pending could leave the network stack in a bad state. Bug: 761541 , 672263 Change-Id: Ic67e2a79024e1ac81ee719447128ce6fcbc5a7ac Reviewed-on: https://chromium-review.googlesource.com/656570 Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Cr-Commit-Position: refs/heads/master@{#500714} [modify] https://crrev.com/4c259e0c548b54ee9c7455bc4848d48e2144ed21/chromeos/components/tether/network_connection_handler_tether_delegate.cc [modify] https://crrev.com/4c259e0c548b54ee9c7455bc4848d48e2144ed21/chromeos/components/tether/network_connection_handler_tether_delegate.h [modify] https://crrev.com/4c259e0c548b54ee9c7455bc4848d48e2144ed21/chromeos/components/tether/network_connection_handler_tether_delegate_unittest.cc
,
Sep 8 2017
,
Sep 8 2017
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5bd3afa6ec7f9b9b2e033071a4d82dca744fbb2b commit 5bd3afa6ec7f9b9b2e033071a4d82dca744fbb2b Author: Ben Wells <benwells@chromium.org> Date: Mon Sep 11 01:06:49 2017 Revert "[CrOS Tether] Invoke any pending connection callbacks on shutdown." This reverts commit 4c259e0c548b54ee9c7455bc4848d48e2144ed21. Reason for revert: Caused heap-use-after-free on Linux Chromium OS ASan LSan Tests Link to first broken build: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/23536 Sample of failure output: SUMMARY: AddressSanitizer: heap-use-after-free chromeos/network/network_connection_handler.cc:81:20 in chromeos::NetworkConnectionHandler::SetTetherDelegate(chromeos::NetworkConnectionHandler::TetherDelegate*) Original change's description: > [CrOS Tether] Invoke any pending connection callbacks on shutdown. > > This ensures that there is no possibility for pending network connection > or disconnection requests when shutdown occurs. Leaving these pending > could leave the network stack in a bad state. > > Bug: 761541 , 672263 > Change-Id: Ic67e2a79024e1ac81ee719447128ce6fcbc5a7ac > Reviewed-on: https://chromium-review.googlesource.com/656570 > Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> > Reviewed-by: Ryan Hansberry <hansberry@chromium.org> > Cr-Commit-Position: refs/heads/master@{#500714} TBR=khorimoto@chromium.org,hansberry@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 761541 , 672263 Change-Id: I48c65bcfadf9b5503245bedae75e443445c103e0 Reviewed-on: https://chromium-review.googlesource.com/659377 Reviewed-by: Ben Wells <benwells@chromium.org> Commit-Queue: Ben Wells <benwells@chromium.org> Cr-Commit-Position: refs/heads/master@{#500820} [modify] https://crrev.com/5bd3afa6ec7f9b9b2e033071a4d82dca744fbb2b/chromeos/components/tether/network_connection_handler_tether_delegate.cc [modify] https://crrev.com/5bd3afa6ec7f9b9b2e033071a4d82dca744fbb2b/chromeos/components/tether/network_connection_handler_tether_delegate.h [modify] https://crrev.com/5bd3afa6ec7f9b9b2e033071a4d82dca744fbb2b/chromeos/components/tether/network_connection_handler_tether_delegate_unittest.cc
,
Sep 11 2017
approving merge to M61 and M62. Please merge to M62 first and post validation merge to M61.
,
Sep 11 2017
,
Sep 11 2017
,
Sep 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7dd2ddd7ec7cad212416fc5294fe56bc6e5a4c9f commit 7dd2ddd7ec7cad212416fc5294fe56bc6e5a4c9f Author: Kyle Horimoto <khorimoto@google.com> Date: Tue Sep 12 21:13:29 2017 [CrOS Tether] Invoke any pending connection callbacks on shutdown. This ensures that there is no possibility for pending network connection or disconnection requests when shutdown occurs. Leaving these pending could leave the network stack in a bad state. The original CL landed as [1] and was reverted as [2] due to an ASAN bot failure. I've uploaded the original CL as patch 1 on this CL, and my fix as patch 2. [1] https://chromium-review.googlesource.com/c/chromium/src/+/656570 [2] https://chromium-review.googlesource.com/c/chromium/src/+/659377 Bug: 761541 , 672263 Change-Id: I3e9beb0ad62fb92ca202f391229dbc46c9110b18 Reviewed-on: https://chromium-review.googlesource.com/663673 Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Commit-Queue: Ryan Hansberry <hansberry@chromium.org> Cr-Commit-Position: refs/heads/master@{#501397} [modify] https://crrev.com/7dd2ddd7ec7cad212416fc5294fe56bc6e5a4c9f/chromeos/components/tether/network_connection_handler_tether_delegate.cc [modify] https://crrev.com/7dd2ddd7ec7cad212416fc5294fe56bc6e5a4c9f/chromeos/components/tether/network_connection_handler_tether_delegate.h [modify] https://crrev.com/7dd2ddd7ec7cad212416fc5294fe56bc6e5a4c9f/chromeos/components/tether/network_connection_handler_tether_delegate_unittest.cc
,
Sep 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7103446dd312eb173850ff6c7425cfbe12803c39 commit 7103446dd312eb173850ff6c7425cfbe12803c39 Author: Kyle Horimoto <khorimoto@google.com> Date: Tue Sep 12 21:35:08 2017 [CrOS Tether] Invoke any pending connection callbacks on shutdown. This ensures that there is no possibility for pending network connection or disconnection requests when shutdown occurs. Leaving these pending could leave the network stack in a bad state. The original CL landed as [1] and was reverted as [2] due to an ASAN bot failure. I've uploaded the original CL as patch 1 on this CL, and my fix as patch 2. [1] https://chromium-review.googlesource.com/c/chromium/src/+/656570 [2] https://chromium-review.googlesource.com/c/chromium/src/+/659377 TBR=khorimoto@google.com (cherry picked from commit 7dd2ddd7ec7cad212416fc5294fe56bc6e5a4c9f) Bug: 761541 , 672263 Change-Id: I3e9beb0ad62fb92ca202f391229dbc46c9110b18 Reviewed-on: https://chromium-review.googlesource.com/663673 Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Commit-Queue: Ryan Hansberry <hansberry@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#501397} Reviewed-on: https://chromium-review.googlesource.com/663856 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#186} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/7103446dd312eb173850ff6c7425cfbe12803c39/chromeos/components/tether/network_connection_handler_tether_delegate.cc [modify] https://crrev.com/7103446dd312eb173850ff6c7425cfbe12803c39/chromeos/components/tether/network_connection_handler_tether_delegate.h [modify] https://crrev.com/7103446dd312eb173850ff6c7425cfbe12803c39/chromeos/components/tether/network_connection_handler_tether_delegate_unittest.cc
,
Sep 12 2017
,
Sep 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3543b71e6b438a8e245f8bb0dfea6b7eae7f7a52 commit 3543b71e6b438a8e245f8bb0dfea6b7eae7f7a52 Author: Kyle Horimoto <khorimoto@google.com> Date: Tue Sep 12 21:36:51 2017 [CrOS Tether] Invoke any pending connection callbacks on shutdown. This ensures that there is no possibility for pending network connection or disconnection requests when shutdown occurs. Leaving these pending could leave the network stack in a bad state. The original CL landed as [1] and was reverted as [2] due to an ASAN bot failure. I've uploaded the original CL as patch 1 on this CL, and my fix as patch 2. [1] https://chromium-review.googlesource.com/c/chromium/src/+/656570 [2] https://chromium-review.googlesource.com/c/chromium/src/+/659377 TBR=khorimoto@google.com (cherry picked from commit 7dd2ddd7ec7cad212416fc5294fe56bc6e5a4c9f) Bug: 761541 , 672263 Change-Id: I3e9beb0ad62fb92ca202f391229dbc46c9110b18 Reviewed-on: https://chromium-review.googlesource.com/663673 Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Commit-Queue: Ryan Hansberry <hansberry@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#501397} Reviewed-on: https://chromium-review.googlesource.com/664237 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#1176} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/3543b71e6b438a8e245f8bb0dfea6b7eae7f7a52/chromeos/components/tether/network_connection_handler_tether_delegate.cc [modify] https://crrev.com/3543b71e6b438a8e245f8bb0dfea6b7eae7f7a52/chromeos/components/tether/network_connection_handler_tether_delegate.h [modify] https://crrev.com/3543b71e6b438a8e245f8bb0dfea6b7eae7f7a52/chromeos/components/tether/network_connection_handler_tether_delegate_unittest.cc
,
Jan 22 2018
,
Jan 23 2018
|
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by khorimoto@chromium.org
, Sep 1 2017