Revert "Fix: Safely delete browsers on shutdown"
This reverts commit 0883dc5859694ef5dbcab5006f9c8c81c8358028.
Reason for revert:
LUCI Bisection has identified this change as the cause of a test failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/test-analysis/b/5703485266853888
Sample build with failed test: https://ci.chromium.org/b/8725020453320881169
Affected test(s):
[ninja://chrome/test:browser_tests/TabManagerTest.DiscardTabsWithOccludedWindow/RetainedWebContents](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2Fchrome%2Ftest:browser_tests%2FTabManagerTest.DiscardTabsWithOccludedWindow%2FRetainedWebContents?q=VHash%3Addee393e1fa4b060)
If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Ftest-analysis%2Fb%2F5703485266853888&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F6179704&type=BUG
Original change's description:
> Fix: Safely delete browsers on shutdown
>
> The current code for deleting browsers on shutdown is not safe because closing a browser window may close other browser windows (e.g., popin feature). The current approach of copying the list of browsers before deleting is not enough to ensure safety.
>
> This change introduces an observer to keep the list of browsers up to date, ensuring that browsers are deleted safely.
>
> Bug: 388400226
>
> Change-Id: I158a22170bfbaf614fdd884d57ddd4dc653490c4
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6179704
> Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1409687}
>
Bug: 388400226
Change-Id: Ia9f36e75cc081a9ea43a36742ae9f37a90fa397a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6192304
Reviewed-by: Solomon Kinard <solomonkinard@chromium.org>
Owners-Override: Solomon Kinard <solomonkinard@google.com>
Commit-Queue: Solomon Kinard <solomonkinard@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1409956}
diff --git a/chrome/browser/lifetime/browser_close_manager.cc b/chrome/browser/lifetime/browser_close_manager.cc
index ffa9f0ce..577ebc5f 100644
--- a/chrome/browser/lifetime/browser_close_manager.cc
+++ b/chrome/browser/lifetime/browser_close_manager.cc
@@ -37,38 +37,6 @@
namespace {
-// Make a copy of the BrowserList and watch for any calls to AddBrowser or
-// RemoveBrowser. This class allows a safe iteration over the list assuming
-// that removing some Browser instance may remove another pending Browser
-// instance.
-class BrowserListIterator : public BrowserListObserver {
- public:
- BrowserListIterator() {
- base::ranges::copy(*BrowserList::GetInstance(),
- std::inserter(browsers_, browsers_.begin()));
- BrowserList::GetInstance()->AddObserver(this);
- }
- BrowserListIterator(const BrowserListIterator&) = delete;
- BrowserListIterator(BrowserListIterator&&) = delete;
- ~BrowserListIterator() override {
- BrowserList::GetInstance()->RemoveObserver(this);
- }
-
- void OnBrowserAdded(Browser* browser) override { browsers_.insert(browser); }
- void OnBrowserRemoved(Browser* browser) override { browsers_.erase(browser); }
- bool IsEmpty() const { return browsers_.empty(); }
-
- Browser* Pop() {
- Browser* browser = *browsers_.begin();
- browsers_.erase(browsers_.begin());
- DCHECK(base::Contains(*BrowserList::GetInstance(), browser));
- return browser;
- }
-
- private:
- std::set<Browser*> browsers_;
-};
-
// Navigates a browser window for |profile|, creating one if necessary, to the
// downloads page if there are downloads in progress for |profile|.
void ShowInProgressDownloads(Profile* profile) {
@@ -214,12 +182,13 @@
// Make a copy of the BrowserList to simplify the case where we need to
// destroy a Browser during the loop.
- BrowserListIterator browser_list_copy;
+ std::vector<Browser*> browser_list_copy;
+ base::ranges::copy(*BrowserList::GetInstance(),
+ std::back_inserter(browser_list_copy));
bool ignore_unload_handlers = browser_shutdown::ShouldIgnoreUnloadHandlers();
- while (!browser_list_copy.IsEmpty()) {
- Browser* browser = browser_list_copy.Pop();
+ for (auto* browser : browser_list_copy) {
browser->window()->Close();
if (ignore_unload_handlers) {
// This path is hit during logoff/power-down. It could be the case that
