Previously, the DisplayLink would never start until a software draw was done at least once. With force-compositing-mode, the software draw was never done, causing tabs to lockup indefinitely waiting for the swap acknowledgement. This fix ensures that the DisplayLink is running immediately.

The Cocoa setUpGState callback was required to avoid "invalid drawable" errors when setView was called on the NSOpenGLContext.

Another deadlock cause could be the GpuChannel route getting removed while the renderer is blocked on a FlushSync. For this, a method has been added to GpuCommandBufferStub to unblock/cleanup that is called from GpuChannel before the IPC route is removed.

Trace events have been added in places that will help debug related issues in the future.

BUG=84343
TEST=launch with --force-compositing-mode; open about:flags; in the same tab, open ycombinator.com and it should not hang.

Review URL: http://codereview.chromium.org/6993043

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@88877 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 134 insertions, 57 deletions

diff --git a/chrome/browser/renderer_host/accelerated_plugin_view_mac.h b/chrome/browser/renderer_host/accelerated_plugin_view_mac.h
index 597ac65..3bbda3b 100644
--- a/chrome/browser/renderer_host/accelerated_plugin_view_mac.h
+++ b/chrome/browser/renderer_host/accelerated_plugin_view_mac.h
@@ -76,7 +76,6 @@ class RenderWidgetHostViewMac;
 
 - (id)initWithRenderWidgetHostViewMac:(RenderWidgetHostViewMac*)r
                          pluginHandle:(gfx::PluginWindowHandle)pluginHandle;
-- (void)drawView;
 
 // Sets the list of rectangles that should show the web page, rather than the
 // accelerated plugin. This is used to simulate the iframe-based trick that web
diff --git a/chrome/browser/renderer_host/accelerated_plugin_view_mac.mm b/chrome/browser/renderer_host/accelerated_plugin_view_mac.mm
index 82638b6..18652a7 100644
--- a/chrome/browser/renderer_host/accelerated_plugin_view_mac.mm
+++ b/chrome/browser/renderer_host/accelerated_plugin_view_mac.mm
@@ -15,27 +15,53 @@
 @implementation AcceleratedPluginView
 @synthesize cachedSize = cachedSize_;
 
+- (void)drawView {
+  TRACE_EVENT1("browser", "AcceleratedPluginViewMac::drawView",
+      "frameNum", swapBuffersCount_);
+
+  if (renderWidgetHostView_) {
+    // TODO(thakis): Pixel or view coordinates for size?
+    renderWidgetHostView_->DrawAcceleratedSurfaceInstance(
+        cglContext_, pluginHandle_, [self cachedSize]);
+  }
+
+  CGLFlushDrawable(cglContext_);
+  CGLSetCurrentContext(0);
+}
+
+// Note: cglContext_ lock must be held during this call.
+- (void)ackSwapBuffers:(uint64)swapBuffersCount {
+  if (swapBuffersCount > acknowledgedSwapBuffersCount_) {
+    acknowledgedSwapBuffersCount_ = swapBuffersCount;
+    bool sendAck = (rendererId_ != 0 || routeId_ != 0);
+    if (sendAck && renderWidgetHostView_) {
+      renderWidgetHostView_->AcknowledgeSwapBuffers(
+          rendererId_,
+          routeId_,
+          gpuHostId_,
+          acknowledgedSwapBuffersCount_);
+    }
+  }
+}
+
 - (CVReturn)getFrameForTime:(const CVTimeStamp*)outputTime {
+  TRACE_EVENT0("browser", "AcceleratedPluginView::getFrameForTime");
   // There is no autorelease pool when this method is called because it will be
   // called from a background thread.
   base::mac::ScopedNSAutoreleasePool pool;
 
-  bool sendAck = (rendererId_ != 0 || routeId_ != 0);
   uint64 currentSwapBuffersCount = swapBuffersCount_;
   if (currentSwapBuffersCount == acknowledgedSwapBuffersCount_) {
     return kCVReturnSuccess;
   }
 
+  // Called on a background thread. Synchronized via the CGL context lock.
+  CGLLockContext(cglContext_);
+
   [self drawView];
+  [self ackSwapBuffers:currentSwapBuffersCount];
 
-  acknowledgedSwapBuffersCount_ = currentSwapBuffersCount;
-  if (sendAck && renderWidgetHostView_) {
-    renderWidgetHostView_->AcknowledgeSwapBuffers(
-        rendererId_,
-        routeId_,
-        gpuHostId_,
-        acknowledgedSwapBuffersCount_);
-  }
+  CGLUnlockContext(cglContext_);
 
   return kCVReturnSuccess;
 }
@@ -93,35 +119,34 @@ static CVReturn DrawOneAcceleratedPluginCallback(
 
     // Set up a display link to do OpenGL rendering on a background thread.
     CVDisplayLinkCreateWithActiveCGDisplays(&displayLink_);
+    CVDisplayLinkSetOutputCallback(
+        displayLink_, &DrawOneAcceleratedPluginCallback, self);
+    CVDisplayLinkSetCurrentCGDisplayFromOpenGLContext(
+        displayLink_, cglContext_, cglPixelFormat_);
+
+    [[NSNotificationCenter defaultCenter]
+         addObserver:self
+            selector:@selector(globalFrameDidChange:)
+                name:NSViewGlobalFrameDidChangeNotification
+              object:self];
   }
   return self;
 }
 
 - (void)dealloc {
   CVDisplayLinkRelease(displayLink_);
+
+  // Ack pending swaps (if any):
+  CGLLockContext(cglContext_);
+  [self ackSwapBuffers:swapBuffersCount_];
+  CGLUnlockContext(cglContext_);
+
   if (renderWidgetHostView_)
     renderWidgetHostView_->DeallocFakePluginWindowHandle(pluginHandle_);
   [[NSNotificationCenter defaultCenter] removeObserver:self];
   [super dealloc];
 }
 
-- (void)drawView {
-  TRACE_EVENT1("browser", "AcceleratedPluginViewMac::drawView",
-      "frameNum", swapBuffersCount_);
-  // Called on a background thread. Synchronized via the CGL context lock.
-  CGLLockContext(cglContext_);
-
-  if (renderWidgetHostView_) {
-    // TODO(thakis): Pixel or view coordinates for size?
-    renderWidgetHostView_->DrawAcceleratedSurfaceInstance(
-        cglContext_, pluginHandle_, [self cachedSize]);
-  }
-
-  CGLFlushDrawable(cglContext_);
-  CGLSetCurrentContext(0);
-  CGLUnlockContext(cglContext_);
-}
-
 - (void)setCutoutRects:(NSArray*)cutout_rects {
   cutoutRects_.reset([cutout_rects copy]);
 }
@@ -141,6 +166,15 @@ static CVReturn DrawOneAcceleratedPluginCallback(
     gpuHostId_ = gpuHostId;
     swapBuffersCount_ = count;
   }
+  // If this view is not visible, we have to ack now. Otherwise,
+  // the ack will not be sent until the tab is made visible, which means it will
+  // look like the tab is loading (busy cursor) until it is clicked.
+  if (![self window]) {
+    // Ack pending swaps (if any):
+    CGLLockContext(cglContext_);
+    [self ackSwapBuffers:swapBuffersCount_];
+    CGLUnlockContext(cglContext_);
+  }
 }
 
 - (void)onRenderWidgetHostViewGone {
@@ -148,6 +182,8 @@ static CVReturn DrawOneAcceleratedPluginCallback(
     return;
 
   CGLLockContext(cglContext_);
+  // Ack pending swaps (if any):
+  [self ackSwapBuffers:swapBuffersCount_];
   // Deallocate the plugin handle while we still can.
   renderWidgetHostView_->DeallocFakePluginWindowHandle(pluginHandle_);
   renderWidgetHostView_ = NULL;
@@ -155,6 +191,7 @@ static CVReturn DrawOneAcceleratedPluginCallback(
 }
 
 - (void)drawRect:(NSRect)rect {
+  TRACE_EVENT0("browser", "AcceleratedPluginView::drawRect");
   const NSRect* dirtyRects;
   int dirtyRectCount;
   [self getRectsBeingDrawn:&dirtyRects count:&dirtyRectCount];
@@ -190,7 +227,9 @@ static CVReturn DrawOneAcceleratedPluginCallback(
     NSRectFillList(dirtyRects, dirtyRectCount);
   }
 
+  CGLLockContext(cglContext_);
   [self drawView];
+  CGLUnlockContext(cglContext_);
 }
 
 - (void)rightMouseDown:(NSEvent*)event {
@@ -225,43 +264,52 @@ static CVReturn DrawOneAcceleratedPluginCallback(
   }
 }
 
+- (void)prepareForGLRendering {
+  TRACE_EVENT0("browser", "AcceleratedPluginView::prepareForGLRendering");
+
+  // If we're using OpenGL, make sure it is connected.
+  if ([glContext_ view] != self) {
+    [glContext_ setView:self];
+  }
+}
+
 - (void)renewGState {
+  TRACE_EVENT0("browser", "AcceleratedPluginView::renewGState");
   // Synchronize with window server to avoid flashes or corrupt drawing.
   [[self window] disableScreenUpdatesUntilFlush];
   [self globalFrameDidChange:nil];
   [super renewGState];
 }
 
+- (void)setUpGState {
+  TRACE_EVENT0("browser", "AcceleratedPluginView::setUpGState");
+  [self prepareForGLRendering];
+}
+
+// TODO(jbates): is lockFocus needed anymore? it seems redundant with
+//               setUpGState in traces.
 - (void)lockFocus {
+  TRACE_EVENT0("browser", "AcceleratedPluginView::lockFocus");
   [super lockFocus];
-
-  // If we're using OpenGL, make sure it is connected and that the display link
-  // is running.
-  if ([glContext_ view] != self) {
-    [glContext_ setView:self];
-
-    [[NSNotificationCenter defaultCenter]
-         addObserver:self
-            selector:@selector(globalFrameDidChange:)
-                name:NSViewGlobalFrameDidChangeNotification
-              object:self];
-    CVDisplayLinkSetOutputCallback(
-        displayLink_, &DrawOneAcceleratedPluginCallback, self);
-    CVDisplayLinkSetCurrentCGDisplayFromOpenGLContext(
-        displayLink_, cglContext_, cglPixelFormat_);
-    CVDisplayLinkStart(displayLink_);
-  }
+  [self prepareForGLRendering];
   [glContext_ makeCurrentContext];
 }
 
 - (void)viewWillMoveToWindow:(NSWindow*)newWindow {
+  TRACE_EVENT1("browser", "AcceleratedPluginView::viewWillMoveToWindow",
+               "newWindow", newWindow);
   // Stop the display link thread while the view is not visible.
   if (newWindow) {
-    if (displayLink_ && !CVDisplayLinkIsRunning(displayLink_))
+    if (!CVDisplayLinkIsRunning(displayLink_))
       CVDisplayLinkStart(displayLink_);
   } else {
-    if (displayLink_ && CVDisplayLinkIsRunning(displayLink_))
+    if (CVDisplayLinkIsRunning(displayLink_))
       CVDisplayLinkStop(displayLink_);
+
+    // Ack pending swaps (if any):
+    CGLLockContext(cglContext_);
+    [self ackSwapBuffers:swapBuffersCount_];
+    CGLUnlockContext(cglContext_);
   }
 
   // Inform the window hosting this accelerated view that it needs to be
@@ -275,6 +323,7 @@ static CVReturn DrawOneAcceleratedPluginCallback(
 }
 
 - (void)viewDidHide {
+  TRACE_EVENT0("browser", "AcceleratedPluginView::viewDidHide");
   [super viewDidHide];
 
   if ([[self window] respondsToSelector:@selector(underlaySurfaceRemoved)]) {
@@ -283,6 +332,7 @@ static CVReturn DrawOneAcceleratedPluginCallback(
 }
 
 - (void)viewDidUnhide {
+  TRACE_EVENT0("browser", "AcceleratedPluginView::viewDidUnhide");
   [super viewDidUnhide];
 
   if ([[self window] respondsToSelector:@selector(underlaySurfaceRemoved)]) {
@@ -297,6 +347,7 @@ static CVReturn DrawOneAcceleratedPluginCallback(
 }
 
 - (void)setFrameSize:(NSSize)newSize {
+  TRACE_EVENT0("browser", "AcceleratedPluginView::newSize");
   [self setCachedSize:newSize];
   [super setFrameSize:newSize];
 }
@@ -316,6 +367,7 @@ static CVReturn DrawOneAcceleratedPluginCallback(
 }
 
 - (void)viewDidMoveToSuperview {
+  TRACE_EVENT0("browser", "AcceleratedPluginView::viewDidMoveToSuperview");
   if (![self superview])
     [self onRenderWidgetHostViewGone];
 }
diff --git a/content/common/gpu/gpu_channel.cc b/content/common/gpu/gpu_channel.cc
index b7a8349..81563c5 100644
--- a/content/common/gpu/gpu_channel.cc
+++ b/content/common/gpu/gpu_channel.cc
@@ -9,6 +9,7 @@
 #include "content/common/gpu/gpu_channel.h"
 
 #include "base/command_line.h"
+#include "base/debug/trace_event.h"
 #include "base/process_util.h"
 #include "base/string_util.h"
 #include "content/common/child_process.h"
@@ -254,6 +255,8 @@ void GpuChannel::OnCreateOffscreenCommandBuffer(
       0, 0, watchdog_));
   router_.AddRoute(*route_id, stub.get());
   stubs_.AddWithID(stub.release(), *route_id);
+  TRACE_EVENT1("gpu", "GpuChannel::OnCreateOffscreenCommandBuffer",
+               "route_id", route_id);
 #else
   *route_id = MSG_ROUTING_NONE;
 #endif
@@ -261,7 +264,14 @@ void GpuChannel::OnCreateOffscreenCommandBuffer(
 
 void GpuChannel::OnDestroyCommandBuffer(int32 route_id) {
 #if defined(ENABLE_GPU)
+  TRACE_EVENT1("gpu", "GpuChannel::OnDestroyCommandBuffer",
+               "route_id", route_id);
   if (router_.ResolveRoute(route_id)) {
+    GpuCommandBufferStub* stub = stubs_.Lookup(route_id);
+    // In case the renderer is currently blocked waiting for a sync reply from
+    // the stub, allow the stub to clean up and unblock pending messages here:
+    if (stub != NULL)
+      stub->CommandBufferWasDestroyed();
     router_.RemoveRoute(route_id);
     stubs_.Remove(route_id);
   }
diff --git a/content/common/gpu/gpu_command_buffer_stub.cc b/content/common/gpu/gpu_command_buffer_stub.cc
index 6b780be..a677337 100644
--- a/content/common/gpu/gpu_command_buffer_stub.cc
+++ b/content/common/gpu/gpu_command_buffer_stub.cc
@@ -393,25 +393,35 @@ void GpuCommandBufferStub::SwapBuffersCallback() {
 
 void GpuCommandBufferStub::AcceleratedSurfaceBuffersSwapped(
     uint64 swap_buffers_count) {
-  TRACE_EVENT0("gpu",
-               "GpuCommandBufferStub::AcceleratedSurfaceBuffersSwapped");
+  TRACE_EVENT1("gpu",
+               "GpuCommandBufferStub::AcceleratedSurfaceBuffersSwapped",
+               "frame", swap_buffers_count);
 
   // Multiple swapbuffers may get consolidated together into a single
   // AcceleratedSurfaceBuffersSwapped call. Since OnSwapBuffers expects to be
   // called one time for every swap, make up the difference here.
   uint64 delta = swap_buffers_count -
       scheduler_->acknowledged_swap_buffers_count();
-
   scheduler_->set_acknowledged_swap_buffers_count(swap_buffers_count);
 
-  for(uint64 i = 0; i < delta; i++)
+  for(uint64 i = 0; i < delta; i++) {
     OnSwapBuffers();
-
-  // Wake up the GpuScheduler to start doing work again.
-  scheduler_->SetScheduled(true);
+    // Wake up the GpuScheduler to start doing work again.
+    scheduler_->SetScheduled(true);
+  }
 }
 #endif  // defined(OS_MACOSX)
 
+void GpuCommandBufferStub::CommandBufferWasDestroyed() {
+  TRACE_EVENT0("gpu", "GpuCommandBufferStub::CommandBufferWasDestroyed");
+  // In case the renderer is currently blocked waiting for a sync reply from
+  // the stub, this method allows us to cleanup and unblock pending messages.
+  while (!scheduler_->IsScheduled())
+    scheduler_->SetScheduled(true);
+  // Handle any deferred messages now that the scheduler is scheduled.
+  HandleDeferredMessages();
+}
+
 void GpuCommandBufferStub::ResizeCallback(gfx::Size size) {
   if (handle_ == gfx::kNullPluginWindow) {
     scheduler_->decoder()->ResizeOffscreenFrameBuffer(size);
diff --git a/content/common/gpu/gpu_command_buffer_stub.h b/content/common/gpu/gpu_command_buffer_stub.h
index 7e10a83..99c2464 100644
--- a/content/common/gpu/gpu_command_buffer_stub.h
+++ b/content/common/gpu/gpu_command_buffer_stub.h
@@ -77,6 +77,10 @@ class GpuCommandBufferStub
   void AcceleratedSurfaceBuffersSwapped(uint64 swap_buffers_count);
 #endif  // defined(OS_MACOSX)
 
+  // Called when the command buffer was destroyed, and the stub should now
+  // unblock itself and handle pending messages.
+  void CommandBufferWasDestroyed();
+
  private:
   // Message handlers:
   void OnInitialize(base::SharedMemoryHandle ring_buffer,
diff --git a/gpu/command_buffer/service/gpu_scheduler.cc b/gpu/command_buffer/service/gpu_scheduler.cc
index 53fcfa2..f6d0607 100644
--- a/gpu/command_buffer/service/gpu_scheduler.cc
+++ b/gpu/command_buffer/service/gpu_scheduler.cc
@@ -137,7 +137,7 @@ const unsigned int kMaxOutstandingSwapBuffersCallsPerOnscreenContext = 1;
 #endif
 
 void GpuScheduler::PutChanged(bool sync) {
-  TRACE_EVENT0("gpu", "GpuScheduler:PutChanged");
+  TRACE_EVENT1("gpu", "GpuScheduler:PutChanged", "this", this);
   CommandBuffer::State state = command_buffer_->GetState();
   parser_->set_put(state.put_offset);
 
@@ -148,7 +148,7 @@ void GpuScheduler::PutChanged(bool sync) {
 }
 
 void GpuScheduler::ProcessCommands() {
-  TRACE_EVENT0("gpu", "GpuScheduler:ProcessCommands");
+  TRACE_EVENT1("gpu", "GpuScheduler:ProcessCommands", "this", this);
   CommandBuffer::State state = command_buffer_->GetState();
   if (state.error != error::kNoError)
     return;
@@ -173,6 +173,7 @@ void GpuScheduler::ProcessCommands() {
   if (do_rate_limiting &&
       swap_buffers_count_ - acknowledged_swap_buffers_count_ >=
       kMaxOutstandingSwapBuffersCallsPerOnscreenContext) {
+    TRACE_EVENT0("gpu", "EarlyOut_OSX_Throttle");
     // Stop doing work on this command buffer. In the GPU process,
     // receipt of the GpuMsg_AcceleratedSurfaceBuffersSwappedACK
     // message causes ProcessCommands to be scheduled again.
@@ -226,8 +227,9 @@ void GpuScheduler::ProcessCommands() {
 }
 
 void GpuScheduler::SetScheduled(bool scheduled) {
-  TRACE_EVENT2("gpu", "GpuScheduler:SetScheduled", "scheduled", scheduled,
-               "unscheduled_count_", unscheduled_count_);
+  TRACE_EVENT2("gpu", "GpuScheduler:SetScheduled", "this", this,
+               "new unscheduled_count_",
+               unscheduled_count_ + (scheduled? -1 : 1));
   if (scheduled) {
     --unscheduled_count_;
     DCHECK_GE(unscheduled_count_, 0);