Defer window.close(), window.resizeTo(), and window.moveTo()

actions by posting a task back to the message loop before
notifying the RenderWidgetHost to perform these operations.
Otherwise, the JS code races with the browser to use the
modified window.

BUG=http://crbug.com/6377
BUG=http://crbug.com/6192

Also, because window resizing is asynchronous between the renderer
and browser, added a renderer-side cache of "pending window rect"
to ChromeClientImpl.  Before the change, calling setWindowRect() would
schedule a call to the RenderViewHost to do the work, but a subsequent
call to windowRect() would return the current size, as through the
call to setWindowRect() had never happened.  We cache a pending size
and then schedule a task on the message loop to clear the cached size.
This allows javascript which writes and reads the WindowRect sizes to
work, and once we come out of JS and return to the message loop, we'll 
go back to asking the RenderViewHost for the true values.

This pending_window_size is my only concern about this patch.  It does
pass all layout tests, and the ChromeClientImpl code is executed in the
test_shell.exe code path.

BUG=http://crbug.com/835

TEST=LayoutTests have tests which cover all of these actions.  The
problem is that we run layout tests using the test_shell, not Chrome.

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

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

4 files changed, 71 insertions, 4 deletions

diff --git a/chrome/renderer/render_widget.cc b/chrome/renderer/render_widget.cc
index 5bed201..d6da3c1 100644
--- a/chrome/renderer/render_widget.cc
+++ b/chrome/renderer/render_widget.cc
@@ -605,12 +605,21 @@ void RenderWidget::Blur(WebWidget* webwidget) {
   Send(new ViewHostMsg_Blur(routing_id_));
 }
 
+void RenderWidget::DoDeferredClose() {
+  Send(new ViewHostMsg_Close(routing_id_));
+}
+
 void RenderWidget::CloseWidgetSoon(WebWidget* webwidget) {
   // If a page calls window.close() twice, we'll end up here twice, but that's
   // OK.  It is safe to send multiple Close messages.
 
-  // Ask the RenderWidgetHost to initiate close.
-  Send(new ViewHostMsg_Close(routing_id_));
+  // Ask the RenderWidgetHost to initiate close.  We could be called from deep
+  // in Javascript.  If we ask the RendwerWidgetHost to close now, the window
+  // could be closed before the JS finishes executing.  So instead, post a
+  // message back to the message loop, which won't run until the JS is
+  // complete, and then the Close message can be sent.
+  MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(
+      this, &RenderWidget::DoDeferredClose));
 }
 
 void RenderWidget::GenerateFullRepaint() {
@@ -630,7 +639,7 @@ void RenderWidget::GetWindowRect(WebWidget* webwidget, WebRect* result) {
   *result = rect;
 }
 
-void RenderWidget::SetWindowRect(WebWidget* webwidget, const WebRect& pos) {
+void RenderWidget::DoDeferredSetWindowRect(const WebRect& pos) {
   if (did_show_) {
     Send(new ViewHostMsg_RequestMove(routing_id_, pos));
   } else {
@@ -638,6 +647,16 @@ void RenderWidget::SetWindowRect(WebWidget* webwidget, const WebRect& pos) {
   }
 }
 
+void RenderWidget::SetWindowRect(WebWidget* webwidget, const WebRect& pos) {
+  // We could be called from deep in Javascript.  If we ask the
+  // RenderWidgetHost to resize now, the window could be resized while the
+  // JS is still running.  This causes a number of interesting side effects
+  // when manipulating window sizes from Javascript.  Avoid races by posting
+  // a message back to the MessageLoop to do it later.
+  MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(
+      this, &RenderWidget::DoDeferredSetWindowRect, pos));
+}
+
 void RenderWidget::GetRootWindowRect(WebWidget* webwidget, WebRect* result) {
   gfx::Rect rect;
   Send(new ViewHostMsg_GetRootWindowRect(routing_id_, host_window_, &rect));
diff --git a/chrome/renderer/render_widget.h b/chrome/renderer/render_widget.h
index 62777e8..eb3575a 100644
--- a/chrome/renderer/render_widget.h
+++ b/chrome/renderer/render_widget.h
@@ -24,6 +24,10 @@
 class RenderThreadBase;
 struct WebPluginGeometry;
 
+namespace WebKit {
+struct WebRect;
+}
+
 // RenderWidget provides a communication bridge between a WebWidget and
 // a RenderWidgetHost, the latter of which lives in a different process.
 class RenderWidget : public IPC::Channel::Listener,
@@ -119,6 +123,8 @@ class RenderWidget : public IPC::Channel::Listener,
 
   void DoDeferredPaint();
   void DoDeferredScroll();
+  void DoDeferredClose();
+  void DoDeferredSetWindowRect(const WebKit::WebRect& pos);
 
   // This method is called immediately after PaintRect but before the
   // corresponding paint or scroll message is send to the widget host.
diff --git a/webkit/glue/chrome_client_impl.cc b/webkit/glue/chrome_client_impl.cc
index 573a3b2..2a6eedc 100644
--- a/webkit/glue/chrome_client_impl.cc
+++ b/webkit/glue/chrome_client_impl.cc
@@ -29,6 +29,7 @@ MSVC_POP_WARNING();
 #include "webkit/glue/chrome_client_impl.h"
 
 #include "base/logging.h"
+#include "base/message_loop.h"
 #include "base/gfx/rect.h"
 #include "googleurl/src/gurl.h"
 #include "third_party/WebKit/WebKit/chromium/public/WebInputEvent.h"
@@ -82,7 +83,8 @@ ChromeClientImpl::ChromeClientImpl(WebViewImpl* webview)
       scrollbars_visible_(true),
       menubar_visible_(true),
       resizable_(true),
-      ignore_next_set_cursor_(false) {
+      ignore_next_set_cursor_(false),
+      ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)) {
 }
 
 ChromeClientImpl::~ChromeClientImpl() {
@@ -92,17 +94,47 @@ void ChromeClientImpl::chromeDestroyed() {
   delete this;
 }
 
+void ChromeClientImpl::ClearPendingWindowRect() {
+  has_pending_window_rect_ = false;
+}
+
+void ChromeClientImpl::SetPendingWindowRect(const WebCore::FloatRect& rect) {
+  // Because we may be asynchronously attached to our delegate (where we
+  // do the real SetWindowRect()/GetWindowRect()), we keep a pending
+  // window rect size.  If JS code sets the WindowRect, and then calls
+  // GetWindowRect() before the delegate has asyncronously handled the
+  // Set, the pending rect makes sure that JS gets the right values.
+  //
+  // TODO(mbelshe): There is still a potential race condition here.  Currently
+  // this cache of the pending_window_rect_ is maintained until we return to
+  // the message loop.  We could make the browser send an ACK when it processes
+  // the SetWindowRect change.
+  pending_window_rect_ = rect;
+  if (!has_pending_window_rect_) {
+    // Create a task to clear the pending rect as soon as we
+    // go through our message loop once.
+    MessageLoop::current()->PostTask(FROM_HERE,
+        task_factory_.NewRunnableMethod(
+        &ChromeClientImpl::ClearPendingWindowRect));
+    has_pending_window_rect_ = true;
+  }
+}
+
 void ChromeClientImpl::setWindowRect(const WebCore::FloatRect& r) {
   WebViewDelegate* delegate = webview_->delegate();
   if (delegate) {
     WebCore::IntRect ir(r);
     delegate->SetWindowRect(webview_,
                             gfx::Rect(ir.x(), ir.y(), ir.width(), ir.height()));
+    SetPendingWindowRect(r);
   }
 }
 
 WebCore::FloatRect ChromeClientImpl::windowRect() {
   WebRect rect;
+  if (has_pending_window_rect_)
+    return pending_window_rect_;
+
   if (webview_->delegate()) {
     webview_->delegate()->GetRootWindowRect(webview_, &rect);
   } else {
diff --git a/webkit/glue/chrome_client_impl.h b/webkit/glue/chrome_client_impl.h
index e372f7c..194dd62 100644
--- a/webkit/glue/chrome_client_impl.h
+++ b/webkit/glue/chrome_client_impl.h
@@ -6,6 +6,7 @@
 #define WEBKIT_GLUE_CHROME_CLIENT_IMPL_H_
 
 #include "base/compiler_specific.h"
+#include "base/task.h"
 
 MSVC_PUSH_WARNING_LEVEL(0);
 #include "ChromeClientChromium.h"
@@ -135,6 +136,9 @@ class ChromeClientImpl : public WebCore::ChromeClientChromium {
   virtual WebCore::HTMLParserQuirks* createHTMLParserQuirks() { return 0; }
 
  private:
+  void ClearPendingWindowRect();
+  void SetPendingWindowRect(const WebCore::FloatRect& r);
+
   WebViewImpl* webview_;  // weak pointer
   bool toolbars_visible_;
   bool statusbar_visible_;
@@ -143,6 +147,12 @@ class ChromeClientImpl : public WebCore::ChromeClientChromium {
   bool resizable_;
   // Set to true if the next SetCursor is to be ignored.
   bool ignore_next_set_cursor_;
+  // While we are waiting for the browser to update window sizes,
+  // we track the pending size temporarily.
+  bool has_pending_window_rect_;
+  WebCore::FloatRect pending_window_rect_;
+
+  ScopedRunnableMethodFactory<ChromeClientImpl> task_factory_;
 };
 
 #endif  // WEBKIT_GLUE_CHROME_CLIENT_IMPL_H_