Ensure task exclusion in BrowsingDataRemover |
|||||
Issue descriptionBrowsingDataRemover currently only supports one removal task at a time. Executing two tasks at the same time can cause undefined behavior. This can be improved using a task scheduler which will ensure that removal tasks called in parallel will be serialized. See this public document for more details about the current state and the proposal: https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/
,
Jul 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/09df8fd82d462a314bde355b8aba7a221f1f3bbc commit 09df8fd82d462a314bde355b8aba7a221f1f3bbc Author: msramek <msramek@chromium.org> Date: Tue Jul 26 11:10:19 2016 Fix a memory leak in BrowsingDataFilterBuilder BrowsingDataFilterBuilder is an abstract class that has a non-virtual destructor. This is a mistake - since the class and its subclasses are non-copyable, they're typically handled by pointers; calling delete on such a pointer will only call the base class destructor, not those of the subclasses, and thus leak any attributes of the subclasses. This was spotted by ASan in the context of the CL implementing the task scheduler for BrowsingDataRemover. When a task was finished processing, its destructor did not correctly destroy the contained BrowsingDataFilterBuilder. BUG= 630327 Review-Url: https://codereview.chromium.org/2185453004 Cr-Commit-Position: refs/heads/master@{#407765} [modify] https://crrev.com/09df8fd82d462a314bde355b8aba7a221f1f3bbc/chrome/browser/browsing_data/browsing_data_filter_builder.h [modify] https://crrev.com/09df8fd82d462a314bde355b8aba7a221f1f3bbc/chrome/browser/browsing_data/origin_filter_builder.h [modify] https://crrev.com/09df8fd82d462a314bde355b8aba7a221f1f3bbc/chrome/browser/browsing_data/registrable_domain_filter_builder.h
,
Jul 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/00117ff9b264b662930f8e10683fe2a0f7abef3f commit 00117ff9b264b662930f8e10683fe2a0f7abef3f Author: msramek <msramek@chromium.org> Date: Thu Jul 28 12:58:03 2016 Deprecate the CallbackSubscription in BrowsingDataRemover BrowsingDataRemover currently supports two notification systems: 1. BrowsingDataRemover::Observer 2. BrowsingDataRemover::CallbackSubscription Remove #2 in favor of #1. The NotificationDetails class which is returned by #2, and used by tests to determine the parameters of the last call to BrowsingDataRemover, is replaced by a public API of BrowsingDataRemover exposing the same values. The consolidation of notifications will simplify the implementation of task scheduler for removal tasks, as described in https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/ BUG= 630327 Review-Url: https://codereview.chromium.org/2171383002 Cr-Commit-Position: refs/heads/master@{#408378} [modify] https://crrev.com/00117ff9b264b662930f8e10683fe2a0f7abef3f/chrome/browser/browsing_data/browsing_data_remover.cc [modify] https://crrev.com/00117ff9b264b662930f8e10683fe2a0f7abef3f/chrome/browser/browsing_data/browsing_data_remover.h [modify] https://crrev.com/00117ff9b264b662930f8e10683fe2a0f7abef3f/chrome/browser/browsing_data/browsing_data_remover_unittest.cc [modify] https://crrev.com/00117ff9b264b662930f8e10683fe2a0f7abef3f/chrome/browser/extensions/api/browsing_data/browsing_data_test.cc
,
Aug 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6f166831372df3044052eda2459982671c24ea5e commit 6f166831372df3044052eda2459982671c24ea5e Author: msramek <msramek@chromium.org> Date: Fri Aug 05 08:44:28 2016 Implement a task scheduler for BrowsingDataRemover BrowsingDataRemover now processes each Remove() call by adding a task to a queue; the tasks are then processed serially. Design doc and discussion: https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/ Changes in this CL: 1. Add a RemovalTask struct that will represent an enqueued task. 2. Since BrowsingDataFilterBuilder and its subclasses are not copyable, change the interface so that it's passed in a unique_ptr (it must be owned by a RemovalTask so that it outlives the Remove() call). 3. Split the Remove() and RemoveWithFilter() interfaces into two versions, one with an observer and one without. Thus, RemoveAndReply() and RemoveWithFilterAndReply() are added. They must explicitly specify a *single* observer that will receive a callback from that particular task. 4. Despite the change #3, every observer must still explicitly register and unregister itself. This is needed so that if a caller is destroyed before its scheduled tasks are executed, it can unregister itself and we know that we should *not* call it back. 5. The recently added OnBrowsingDataRemoving() call which communicates a busy / idle state of BrowsingDataRemover still calls all registered observers. 6. Updated the tests with the new interface. To consider in follow-up CLs: - Use a base::Callback instead of an Observer pointer (see the discussion in the design doc), or use a task handles whose destruction will cancel the task. - Consider changing BrowsingDataFilterBuilder to be copyable. This is still not needed in production, but could simplify tests. TBR=mmenke@chromium.org,engedy@chromium.org,meacer@chromium.org,johnme@chromium.org,stevenjb@chromium.org BUG= 630327 Review-Url: https://codereview.chromium.org/2175703002 Cr-Commit-Position: refs/heads/master@{#410016} [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/android/preferences/pref_service_bridge.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/android/signin/signin_manager_android.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/browsing_data/browsing_data_remover.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/browsing_data/browsing_data_remover.h [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/browsing_data/browsing_data_remover_browsertest.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/browsing_data/browsing_data_remover_unittest.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/chromeos/profiles/profile_helper.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/extensions/api/browsing_data/browsing_data_api.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/net/sdch_browsertest.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/prerender/prerender_browsertest.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/profile_resetter/profile_resetter.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/push_messaging/push_messaging_browsertest.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/ssl/chrome_ssl_host_state_delegate_test.cc [modify] https://crrev.com/6f166831372df3044052eda2459982671c24ea5e/chrome/browser/ui/webui/options/clear_browser_data_handler.cc
,
Sep 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a946e92eaa55cb6da801ada49dfdd51d786fa06d commit a946e92eaa55cb6da801ada49dfdd51d786fa06d Author: msramek <msramek@chromium.org> Date: Thu Sep 01 10:38:26 2016 Make ClearBrowsingDataHandler only observe its own removal task In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler was made to observe whether BrowsingDataRemover has any removal task in progress and disable the "Clear data" button if that is the case. This is no longer necessary. BrowsingDataRemover is nowadays able to accept a new removal task at any time and call back (only) the associated observer when the task is done. Callers no longer need to be aware of the possibility of other removal tasks running. We still disable the "Clear data" button while we execute the removal task started by pressing that button - this serves as a feedback to the user that their removal task has indeed been executed, i.e. that pressing the button had an effect. However, opening another instance of a dialog in a different tab (i.e. tied to a different handler) allows the user to execute another parallel removal task if they wish to do so. Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from BrowsingDataRemover::Observer, as the previous behavior of ClearBrowsingDataHandler was its only usecase. In practice, this CL is mostly a partial revert of https://codereview.chromium.org/2133893002/ BUG= 630327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2240883002 Cr-Commit-Position: refs/heads/master@{#415925} [modify] https://crrev.com/a946e92eaa55cb6da801ada49dfdd51d786fa06d/chrome/browser/browsing_data/browsing_data_remover.cc [modify] https://crrev.com/a946e92eaa55cb6da801ada49dfdd51d786fa06d/chrome/browser/browsing_data/browsing_data_remover.h [modify] https://crrev.com/a946e92eaa55cb6da801ada49dfdd51d786fa06d/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js [modify] https://crrev.com/a946e92eaa55cb6da801ada49dfdd51d786fa06d/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js [modify] https://crrev.com/a946e92eaa55cb6da801ada49dfdd51d786fa06d/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc [modify] https://crrev.com/a946e92eaa55cb6da801ada49dfdd51d786fa06d/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h
,
Sep 1 2016
The last one of these CLs (comment #5) reconciles the logic used by the material design Clear Browsing Data dialog (chrome://md-settings/clearBrowserData) to track the clearing tasks with the sequentialization of tasks in BrowsingDataRemover. In the state before this CL, the dialog would stay open until all running tasks are finished, not only the task initiated by the user. In the current world, it's probably not a problem yet - it's not yet likely that several tasks would be executed at the same time, and the old non-material CBD dialog did not even protect itself from this case. However, it's still a bug that has appeared in M54, so I would consider merging it.
,
Sep 2 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 2 2016
Please merge your change to M54 (branch: 2840) before 5:00 PM PST Monday [09/05] if you would like to make it to M54 Beta promotion on Thursday [09/08].
,
Sep 5 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b768f65f0df5bda94f61b45035c83793678c8e46 commit b768f65f0df5bda94f61b45035c83793678c8e46 Author: Martin Sramek <msramek@chromium.org> Date: Mon Sep 05 09:26:46 2016 Make ClearBrowsingDataHandler only observe its own removal task In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler was made to observe whether BrowsingDataRemover has any removal task in progress and disable the "Clear data" button if that is the case. This is no longer necessary. BrowsingDataRemover is nowadays able to accept a new removal task at any time and call back (only) the associated observer when the task is done. Callers no longer need to be aware of the possibility of other removal tasks running. We still disable the "Clear data" button while we execute the removal task started by pressing that button - this serves as a feedback to the user that their removal task has indeed been executed, i.e. that pressing the button had an effect. However, opening another instance of a dialog in a different tab (i.e. tied to a different handler) allows the user to execute another parallel removal task if they wish to do so. Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from BrowsingDataRemover::Observer, as the previous behavior of ClearBrowsingDataHandler was its only usecase. In practice, this CL is mostly a partial revert of https://codereview.chromium.org/2133893002/ BUG= 630327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2240883002 Cr-Commit-Position: refs/heads/master@{#415925} (cherry picked from commit a946e92eaa55cb6da801ada49dfdd51d786fa06d) Review URL: https://codereview.chromium.org/2315443002 . Cr-Commit-Position: refs/branch-heads/2840@{#154} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/browsing_data/browsing_data_remover.cc [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/browsing_data/browsing_data_remover.h [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h
,
Sep 5 2016
We now fully support the serialization of deletion tasks.
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b768f65f0df5bda94f61b45035c83793678c8e46 commit b768f65f0df5bda94f61b45035c83793678c8e46 Author: Martin Sramek <msramek@chromium.org> Date: Mon Sep 05 09:26:46 2016 Make ClearBrowsingDataHandler only observe its own removal task In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler was made to observe whether BrowsingDataRemover has any removal task in progress and disable the "Clear data" button if that is the case. This is no longer necessary. BrowsingDataRemover is nowadays able to accept a new removal task at any time and call back (only) the associated observer when the task is done. Callers no longer need to be aware of the possibility of other removal tasks running. We still disable the "Clear data" button while we execute the removal task started by pressing that button - this serves as a feedback to the user that their removal task has indeed been executed, i.e. that pressing the button had an effect. However, opening another instance of a dialog in a different tab (i.e. tied to a different handler) allows the user to execute another parallel removal task if they wish to do so. Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from BrowsingDataRemover::Observer, as the previous behavior of ClearBrowsingDataHandler was its only usecase. In practice, this CL is mostly a partial revert of https://codereview.chromium.org/2133893002/ BUG= 630327 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2240883002 Cr-Commit-Position: refs/heads/master@{#415925} (cherry picked from commit a946e92eaa55cb6da801ada49dfdd51d786fa06d) Review URL: https://codereview.chromium.org/2315443002 . Cr-Commit-Position: refs/branch-heads/2840@{#154} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/browsing_data/browsing_data_remover.cc [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/browsing_data/browsing_data_remover.h [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_browser_proxy.js [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.js [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc [modify] https://crrev.com/b768f65f0df5bda94f61b45035c83793678c8e46/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h
,
Nov 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/08165e64189a0c081f340b1819ab2e954d5266a1 commit 08165e64189a0c081f340b1819ab2e954d5266a1 Author: msramek <msramek@chromium.org> Date: Wed Nov 23 16:13:37 2016 Remove the "one at a time" restriction from the browsingData API As of https://codereview.chromium.org/2175703002, BrowsingDataRemover supports task exclusion and this is no longer necessary. BUG= 630327 Review-Url: https://codereview.chromium.org/2504943002 Cr-Commit-Position: refs/heads/master@{#434177} [modify] https://crrev.com/08165e64189a0c081f340b1819ab2e954d5266a1/chrome/browser/browsing_data/browsing_data_remover.h [modify] https://crrev.com/08165e64189a0c081f340b1819ab2e954d5266a1/chrome/browser/extensions/api/browsing_data/browsing_data_api.cc [modify] https://crrev.com/08165e64189a0c081f340b1819ab2e954d5266a1/chrome/browser/extensions/api/browsing_data/browsing_data_api.h [modify] https://crrev.com/08165e64189a0c081f340b1819ab2e954d5266a1/chrome/browser/extensions/api/browsing_data/browsing_data_test.cc |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by bugdroid1@chromium.org
, Jul 22 2016