Commit cfa89ca5 authored by Mikel Astiz's avatar Mikel Astiz Committed by Commit Bot

Fix mac pumps potentially doing nothing in DoRun()

Prior to this patch, DoRun() in MessagePumpNSRunLoop and
MessagePumpNSApplication didn't handle well the case where keep_running_
is initially false.

This situation reproduces in the convoluted scenario of deferred quits
that involve yet another nested loop being started exactly between the
quit request and its deferred execution, which is actually exercised in
some browser tests.

The proposed solution mimics what other analogous pumps do.

Bug: 787551
Change-Id: Ida8c93f9bc189477e5ad16f86e88ac1613160ba2
Reviewed-on: https://chromium-review.googlesource.com/782563Reviewed-by: default avatarMike Pinkerton <pinkerton@chromium.org>
Reviewed-by: default avatarMark Mentovai <mark@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521709}
parent 112c4ff7
...@@ -8,6 +8,7 @@ ...@@ -8,6 +8,7 @@
#include <limits> #include <limits>
#include "base/auto_reset.h"
#include "base/logging.h" #include "base/logging.h"
#include "base/mac/call_with_eh_frame.h" #include "base/mac/call_with_eh_frame.h"
#include "base/mac/scoped_cftyperef.h" #include "base/mac/scoped_cftyperef.h"
...@@ -719,13 +720,13 @@ MessagePumpNSRunLoop::~MessagePumpNSRunLoop() { ...@@ -719,13 +720,13 @@ MessagePumpNSRunLoop::~MessagePumpNSRunLoop() {
} }
void MessagePumpNSRunLoop::DoRun(Delegate* delegate) { void MessagePumpNSRunLoop::DoRun(Delegate* delegate) {
AutoReset<bool> auto_reset_keep_running(&keep_running_, true);
while (keep_running_) { while (keep_running_) {
// NSRunLoop manages autorelease pools itself. // NSRunLoop manages autorelease pools itself.
[[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode
beforeDate:[NSDate distantFuture]]; beforeDate:[NSDate distantFuture]];
} }
keep_running_ = true;
} }
void MessagePumpNSRunLoop::Quit() { void MessagePumpNSRunLoop::Quit() {
...@@ -789,6 +790,7 @@ MessagePumpNSApplication::~MessagePumpNSApplication() { ...@@ -789,6 +790,7 @@ MessagePumpNSApplication::~MessagePumpNSApplication() {
} }
void MessagePumpNSApplication::DoRun(Delegate* delegate) { void MessagePumpNSApplication::DoRun(Delegate* delegate) {
AutoReset<bool> auto_reset_keep_running(&keep_running_, true);
bool last_running_own_loop_ = running_own_loop_; bool last_running_own_loop_ = running_own_loop_;
// NSApp must be initialized by calling: // NSApp must be initialized by calling:
...@@ -815,7 +817,6 @@ void MessagePumpNSApplication::DoRun(Delegate* delegate) { ...@@ -815,7 +817,6 @@ void MessagePumpNSApplication::DoRun(Delegate* delegate) {
[NSApp sendEvent:event]; [NSApp sendEvent:event];
} }
} }
keep_running_ = true;
} }
running_own_loop_ = last_running_own_loop_; running_own_loop_ = last_running_own_loop_;
......
...@@ -7,7 +7,6 @@ ...@@ -7,7 +7,6 @@
#include <memory> #include <memory>
#include "base/mac/scoped_nsobject.h" #include "base/mac/scoped_nsobject.h"
#include "base/message_loop/message_pump_mac.h"
#import "ui/gfx/test/ui_cocoa_test_helper.h" #import "ui/gfx/test/ui_cocoa_test_helper.h"
// This class runs an animation for exactly two frames then end it. // This class runs an animation for exactly two frames then end it.
...@@ -15,7 +14,6 @@ ...@@ -15,7 +14,6 @@
: NSObject<NSAnimationDelegate> { : NSObject<NSAnimationDelegate> {
@private @private
CGFloat frameCount_; CGFloat frameCount_;
std::unique_ptr<base::MessagePumpNSRunLoop> message_pump_;
} }
- (void)runAnimation:(NSAnimation*)animation; - (void)runAnimation:(NSAnimation*)animation;
...@@ -24,12 +22,6 @@ ...@@ -24,12 +22,6 @@
@implementation ConstrainedWindowAnimationTestDelegate @implementation ConstrainedWindowAnimationTestDelegate
- (id)init {
if ((self = [super init]))
message_pump_.reset(new base::MessagePumpNSRunLoop);
return self;
}
- (float)animation:(NSAnimation*)animation - (float)animation:(NSAnimation*)animation
valueForProgress:(NSAnimationProgress)progress { valueForProgress:(NSAnimationProgress)progress {
++frameCount_; ++frameCount_;
...@@ -40,7 +32,6 @@ ...@@ -40,7 +32,6 @@
- (void)animationDidEnd:(NSAnimation*)animation { - (void)animationDidEnd:(NSAnimation*)animation {
EXPECT_EQ(2, frameCount_); EXPECT_EQ(2, frameCount_);
message_pump_->Quit();
} }
- (void)runAnimation:(NSAnimation*)animation { - (void)runAnimation:(NSAnimation*)animation {
...@@ -49,7 +40,7 @@ ...@@ -49,7 +40,7 @@
[animation setDuration:600]; [animation setDuration:600];
[animation setDelegate:self]; [animation setDelegate:self];
[animation startAnimation]; [animation startAnimation];
message_pump_->Run(NULL); EXPECT_EQ(2, frameCount_);
} }
@end @end
...@@ -57,7 +48,7 @@ ...@@ -57,7 +48,7 @@
class ConstrainedWindowAnimationTest : public ui::CocoaTest { class ConstrainedWindowAnimationTest : public ui::CocoaTest {
protected: protected:
ConstrainedWindowAnimationTest() : CocoaTest() { ConstrainedWindowAnimationTest() : CocoaTest() {
delegate_.reset([[ConstrainedWindowAnimationTestDelegate alloc] init]); delegate_.reset([ConstrainedWindowAnimationTestDelegate alloc]);
} }
base::scoped_nsobject<ConstrainedWindowAnimationTestDelegate> delegate_; base::scoped_nsobject<ConstrainedWindowAnimationTestDelegate> delegate_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment