Revert "base: Don't keep running tasks after nested run loop quits on iOS"
This reverts commit 3ce69a991fd9ca03030738e324a1833c02462da0.
Reason for revert:
Sheriff suspects this broke a handful of menu tests on macOS 10.12:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.12%20Tests/17564
e.g.:
WebViewTest.TestContextMenu
Original change's description:
> base: Don't keep running tasks after nested run loop quits on iOS
>
> The CoreFoundation message pump (MessagePumpCFRunLoop) scheduled an extra call
> to Delegate::DoWork in the following scenario:
>
> 1. Start a run loop and schedule DoWork.
> 2. In DoWork, start a second run nested run loop which quits immediately.
> 3. After the nested run loop quits, schedule another DoWork which quits the
> original run loop.
>
> After step #3, the message pump would call DoWork again because the nested
> run loop triggered the execution of the nesting deferred work source.
>
> This patch fixes the issue by checking whether the further work is allowed
> before calling DoWork. Note that this check can't be done just for the nested
> deferred work source, because in the flow above that source is what triggers
> the call to DoWork in step #3, i.e., the run loop hasn't quit yet at that point.
>
> The patch also includes a fix to
> WebContentsViewMacInteractiveTest.SelectMenuLifetime; the test had the
> following sequence in it (while a select menu is open):
>
> 1. Quit an outer run loop.
> 2. Post a task onto an inner run loop to continue the test.
>
> The assumption here was that a select menu causes an inner run loop to
> be active while the menu is open. This wasn't strictly correct: the
> inner run loop is a native (CoreFoundation) run loop as opposed to a
> base::RunLoop, which can't be explicitly exited with RunLoop::Quit().
> This means that the Quit() in step #1 affects the same run loop as the
> PostTask in step #2, i.e., the task is prevented from running.
>
> This patch changes the test to only quit the test run loop after the
> posted task (which closes the select menu) has run.
>
> Bug: 891670
> Change-Id: I6fe21289205664c4e1b1469547495667c753f56d
> Reviewed-on: https://chromium-review.googlesource.com/c/1373754
> Reviewed-by: Mark Mentovai <mark@chromium.org>
> Commit-Queue: Sami Kyöstilä <skyostil@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#615967}
TBR=skyostil@chromium.org,mark@chromium.org
Change-Id: If6e3b032ae40a391df56b62ae78f0bc1ed8d93f6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 891670
Reviewed-on: https://chromium-review.googlesource.com/c/1375382
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616297}diff --git a/base/message_loop/message_pump_mac.mm b/base/message_loop/message_pump_mac.mm
index a3ffbcf..ba39d57 100644
--- a/base/message_loop/message_pump_mac.mm
+++ b/base/message_loop/message_pump_mac.mm
@@ -472,8 +472,6 @@
delegateless_work_ = true;
return false;
}
- if (!keep_running())
- return false;
// The NSApplication-based run loop only drains the autorelease pool at each
// UI event (NSEvent). The autorelease pool is not drained for each
diff --git a/base/message_loop/message_pump_unittest.cc b/base/message_loop/message_pump_unittest.cc
index ba2a566..0cde468 100644
--- a/base/message_loop/message_pump_unittest.cc
+++ b/base/message_loop/message_pump_unittest.cc
@@ -8,9 +8,7 @@
#include "testing/gtest/include/gtest/gtest.h"
using ::testing::_;
-using ::testing::AnyNumber;
using ::testing::Invoke;
-using ::testing::Return;
namespace base {
namespace {
@@ -52,39 +50,6 @@
message_pump_->Run(&delegate);
}
-TEST_P(MessagePumpTest, QuitStopsWorkWithNestedRunLoop) {
- testing::InSequence sequence;
- testing::StrictMock<MockMessagePumpDelegate> delegate;
- testing::StrictMock<MockMessagePumpDelegate> nested_delegate;
-
- // We first schedule a call to DoWork, which runs a nested run loop. After the
- // nested loop exits, we schedule another DoWork which quits the outer
- // (original) run loop. The test verifies that there are no extra calls to
- // DoWork after the outer loop quits.
- EXPECT_CALL(delegate, DoWork).WillOnce(Invoke([&] {
- message_pump_->ScheduleWork();
- message_pump_->Run(&nested_delegate);
- message_pump_->ScheduleWork();
- return false;
- }));
- EXPECT_CALL(nested_delegate, DoWork).WillOnce(Invoke([&] {
- // Quit the nested run loop.
- message_pump_->Quit();
- return false;
- }));
- EXPECT_CALL(delegate, DoDelayedWork(_)).WillOnce(Return(false));
- // The outer pump may or may not trigger idle work at this point.
- EXPECT_CALL(delegate, DoIdleWork()).Times(AnyNumber());
- EXPECT_CALL(delegate, DoWork).WillOnce(Invoke([&] {
- // Quit the original run loop.
- message_pump_->Quit();
- return false;
- }));
-
- message_pump_->ScheduleWork();
- message_pump_->Run(&delegate);
-}
-
INSTANTIATE_TEST_CASE_P(,
MessagePumpTest,
::testing::Values(MessageLoop::TYPE_DEFAULT,
diff --git a/chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm b/chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm
index 0209a3b..d6ab72f 100644
--- a/chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm
+++ b/chrome/browser/ui/cocoa/tab_contents/web_contents_view_mac_interactive_uitest.mm
@@ -6,7 +6,6 @@
#import "base/mac/scoped_nsobject.h"
#include "base/run_loop.h"
-#include "base/test/bind_test_util.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -42,17 +41,20 @@
usingBlock:^(NSNotification* notification) {
first_item.reset(
[[[[notification object] itemAtIndex:0] title] copy]);
+ // The nested run loop runs next. Ensure the outer run loop
+ // exits once the inner run loop quits.
+ outer_run_loop_for_block->Quit();
+
// We can't cancel tracking until after
// NSMenuDidBeginTrackingNotification is processed (i.e. after
// this block returns). So post a task to run on the inner run
// loop which will close the tab (and cancel tracking in
- // ~PopupMenuHelper()) and quit the outer run loop to continue
- // the test.
+ // ~PopupMenuHelper()).
base::ThreadTaskRunnerHandle::Get()->PostTask(
- FROM_HERE, base::BindLambdaForTesting([&] {
- browser()->tab_strip_model()->CloseWebContentsAt(1, 0);
- outer_run_loop_for_block->Quit();
- }));
+ FROM_HERE,
+ base::Bind(
+ base::IgnoreResult(&TabStripModel::CloseWebContentsAt),
+ base::Unretained(browser()->tab_strip_model()), 1, 0));
}];
// Send a space key to open the <select>.
